-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(redis-cache): Create cache-span with prefixed keys (get/set commands) #12070
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
497f196
feat(redis-cache): Create cache-span with prefixed keys
s1gr1d f98a4a7
set item size to undefined on error
s1gr1d 473e4f2
Merge branch 'develop' into sig/redis-cache-prefix
s1gr1d e433396
rename startSpan
s1gr1d 5cf9d80
fix test
s1gr1d 1a33f3d
fix test
mydea 3707dac
Merge branch 'develop' into sig/redis-cache-prefix
s1gr1d File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
9 changes: 9 additions & 0 deletions
9
dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
version: '3.9' | ||
|
||
services: | ||
db: | ||
image: redis:latest | ||
restart: always | ||
container_name: integration-tests-redis | ||
ports: | ||
- '6379:6379' |
41 changes: 41 additions & 0 deletions
41
dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
const { loggingTransport } = require('@sentry-internal/node-integration-tests'); | ||
const Sentry = require('@sentry/node'); | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
release: '1.0', | ||
tracesSampleRate: 1.0, | ||
transport: loggingTransport, | ||
integrations: [Sentry.redisIntegration({ cachePrefixes: ['ioredis-cache:'] })], | ||
}); | ||
|
||
// Stop the process from exiting before the transaction is sent | ||
setInterval(() => {}, 1000); | ||
|
||
const Redis = require('ioredis'); | ||
|
||
const redis = new Redis({ port: 6379 }); | ||
|
||
async function run() { | ||
await Sentry.startSpan( | ||
{ | ||
name: 'Test Span', | ||
op: 'test-span', | ||
}, | ||
async () => { | ||
try { | ||
await redis.set('test-key', 'test-value'); | ||
await redis.set('ioredis-cache:test-key', 'test-value'); | ||
|
||
await redis.get('test-key'); | ||
await redis.get('ioredis-cache:test-key'); | ||
await redis.get('ioredis-cache:unavailable-data'); | ||
} finally { | ||
await redis.disconnect(); | ||
} | ||
}, | ||
); | ||
} | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
run(); |
92 changes: 92 additions & 0 deletions
92
dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; | ||
|
||
describe('redis auto instrumentation', () => { | ||
afterAll(() => { | ||
cleanupChildProcesses(); | ||
}); | ||
|
||
test('should not add cache spans when key is not prefixed', done => { | ||
const EXPECTED_TRANSACTION = { | ||
transaction: 'Test Span', | ||
spans: expect.arrayContaining([ | ||
expect.objectContaining({ | ||
description: 'set test-key [1 other arguments]', | ||
op: 'db', | ||
data: expect.objectContaining({ | ||
'sentry.op': 'db', | ||
'db.system': 'redis', | ||
'net.peer.name': 'localhost', | ||
'net.peer.port': 6379, | ||
'db.statement': 'set test-key [1 other arguments]', | ||
}), | ||
}), | ||
expect.objectContaining({ | ||
description: 'get test-key', | ||
op: 'db', | ||
data: expect.objectContaining({ | ||
'sentry.op': 'db', | ||
'db.system': 'redis', | ||
'net.peer.name': 'localhost', | ||
'net.peer.port': 6379, | ||
'db.statement': 'get test-key', | ||
}), | ||
}), | ||
]), | ||
}; | ||
|
||
createRunner(__dirname, 'scenario-ioredis.js') | ||
.withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] }) | ||
.expect({ transaction: EXPECTED_TRANSACTION }) | ||
.start(done); | ||
}); | ||
|
||
test('should create cache spans for prefixed keys', done => { | ||
const EXPECTED_TRANSACTION = { | ||
transaction: 'Test Span', | ||
spans: expect.arrayContaining([ | ||
// SET | ||
expect.objectContaining({ | ||
description: 'set ioredis-cache:test-key [1 other arguments]', | ||
op: 'cache.put', | ||
data: expect.objectContaining({ | ||
'db.statement': 'set ioredis-cache:test-key [1 other arguments]', | ||
'cache.key': 'ioredis-cache:test-key', | ||
'cache.item_size': 2, | ||
'network.peer.address': 'localhost', | ||
'network.peer.port': 6379, | ||
}), | ||
}), | ||
// GET | ||
expect.objectContaining({ | ||
description: 'get ioredis-cache:test-key', | ||
op: 'cache.get_item', // todo: will be changed to cache.get | ||
data: expect.objectContaining({ | ||
'db.statement': 'get ioredis-cache:test-key', | ||
'cache.hit': true, | ||
'cache.key': 'ioredis-cache:test-key', | ||
'cache.item_size': 10, | ||
'network.peer.address': 'localhost', | ||
'network.peer.port': 6379, | ||
}), | ||
}), | ||
// GET (unavailable) | ||
expect.objectContaining({ | ||
description: 'get ioredis-cache:unavailable-data', | ||
op: 'cache.get_item', // todo: will be changed to cache.get | ||
data: expect.objectContaining({ | ||
'db.statement': 'get ioredis-cache:unavailable-data', | ||
'cache.hit': false, | ||
'cache.key': 'ioredis-cache:unavailable-data', | ||
'network.peer.address': 'localhost', | ||
'network.peer.port': 6379, | ||
}), | ||
}), | ||
]), | ||
}; | ||
|
||
createRunner(__dirname, 'scenario-ioredis.js') | ||
.withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] }) | ||
.expect({ transaction: EXPECTED_TRANSACTION }) | ||
.start(done); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of interest, are we using something non-standard here or is redis emitting something non-standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redis seems to be using the old standard as there was a change to those params: open-telemetry/opentelemetry-specification#3199
We are using those: https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, got it, makes sense - maybe we can leave a comment like this in the code so we can potentially revisit this when we update the redis instrumentation at some point :)