-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Add maxCacheKeyLength to Redis integration (remove truncation)
#18045
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
Changes from 1 commit
7f030e8
7b87870
1bb037f
f32141b
dc0f0cc
0ae8180
29bdf86
778e047
d0c9e8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,14 +24,32 @@ import { | |
| } from '../../utils/redisCache'; | ||
|
|
||
| interface RedisOptions { | ||
| /** | ||
| * Define cache prefixes for cache keys that should be captured as a cache span. | ||
| * | ||
| * Setting this to, for example, `['user:']` will capture cache keys that start with `user:`. | ||
| */ | ||
| cachePrefixes?: string[]; | ||
| /** | ||
| * Maximum length of the cache key added to the span description. If the key exceeds this length, it will be truncated. | ||
| * | ||
| * By default, the full cache key is used. | ||
| */ | ||
| maxCacheKeyLength?: number; | ||
| } | ||
|
|
||
| const INTEGRATION_NAME = 'Redis'; | ||
|
|
||
| let _redisOptions: RedisOptions = {}; | ||
| /* Only exported for testing purposes */ | ||
| export let _redisOptions: RedisOptions = {}; | ||
|
|
||
| const cacheResponseHook: RedisResponseCustomAttributeFunction = (span: Span, redisCommand, cmdArgs, response) => { | ||
| /* Only exported for testing purposes */ | ||
| export const cacheResponseHook: RedisResponseCustomAttributeFunction = ( | ||
| span: Span, | ||
| redisCommand, | ||
| cmdArgs, | ||
| response, | ||
| ) => { | ||
| span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis'); | ||
|
|
||
| const safeKey = getCacheKeySafely(redisCommand, cmdArgs); | ||
|
|
@@ -70,9 +88,12 @@ const cacheResponseHook: RedisResponseCustomAttributeFunction = (span: Span, red | |
| [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, | ||
| }); | ||
|
|
||
| // todo: change to string[] once EAP supports it | ||
| const spanDescription = safeKey.join(', '); | ||
|
|
||
| span.updateName(truncate(spanDescription, 1024)); | ||
| span.updateName( | ||
| _redisOptions.maxCacheKeyLength ? truncate(spanDescription, _redisOptions.maxCacheKeyLength) : spanDescription, | ||
| ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a new comment here since the previous one was marked as outdated. Setting
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the default behavior of our |
||
| }; | ||
|
|
||
| const instrumentIORedis = generateInstrumentOnce(`${INTEGRATION_NAME}.IORedis`, () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { _redisOptions, cacheResponseHook } from '../../../src/integrations/tracing/redis'; | ||
| import { | ||
| calculateCacheItemSize, | ||
| GET_COMMANDS, | ||
|
|
@@ -8,6 +9,82 @@ import { | |
| } from '../../../src/utils/redisCache'; | ||
|
|
||
| describe('Redis', () => { | ||
| describe('cacheResponseHook', () => { | ||
| let mockSpan: any; | ||
| let originalRedisOptions: any; | ||
|
|
||
| beforeEach(() => { | ||
| mockSpan = { | ||
| setAttribute: vi.fn(), | ||
| setAttributes: vi.fn(), | ||
| updateName: vi.fn(), | ||
| spanContext: () => ({ spanId: 'test-span-id', traceId: 'test-trace-id' }), | ||
| }; | ||
|
|
||
| originalRedisOptions = { ..._redisOptions }; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| // Reset redis options by clearing all properties first, then restoring original ones | ||
| Object.keys(_redisOptions).forEach(key => delete (_redisOptions as any)[key]); | ||
| Object.assign(_redisOptions, originalRedisOptions); | ||
| }); | ||
|
|
||
| describe('early returns', () => { | ||
| it.each([ | ||
| { desc: 'no args', cmd: 'get', args: [], response: 'test' }, | ||
| { desc: 'unsupported command', cmd: 'exists', args: ['key'], response: 'test' }, | ||
| { desc: 'no cache prefixes', cmd: 'get', args: ['key'], response: 'test', options: {} }, | ||
| { desc: 'non-matching prefix', cmd: 'get', args: ['key'], response: 'test', options: { cachePrefixes: ['c'] } }, | ||
| ])('should always set sentry.origin but return early when $desc', ({ cmd, args, response, options = {} }) => { | ||
| vi.clearAllMocks(); | ||
|
||
| Object.assign(_redisOptions, options); | ||
|
|
||
| cacheResponseHook(mockSpan, cmd, args, response); | ||
|
|
||
| expect(mockSpan.setAttribute).toHaveBeenCalledWith('sentry.origin', 'auto.db.otel.redis'); | ||
| expect(mockSpan.setAttributes).not.toHaveBeenCalled(); | ||
| expect(mockSpan.updateName).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
Comment on lines
35
to
50
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added some general tests for the handler, as there were none for this behavior so far. |
||
|
|
||
| describe('span name truncation', () => { | ||
| beforeEach(() => { | ||
| Object.assign(_redisOptions, { cachePrefixes: ['cache:'] }); | ||
| }); | ||
|
|
||
| it('should not truncate span name when maxCacheKeyLength is not set', () => { | ||
| cacheResponseHook( | ||
| mockSpan, | ||
| 'mget', | ||
| ['cache:very-long-key-name', 'cache:very-long-key-name-2', 'cache:very-long-key-name-3'], | ||
| 'value', | ||
| ); | ||
|
|
||
| expect(mockSpan.updateName).toHaveBeenCalledWith( | ||
| 'cache:very-long-key-name, cache:very-long-key-name-2, cache:very-long-key-name-3', | ||
| ); | ||
| }); | ||
|
|
||
| it('should truncate span name when maxCacheKeyLength is set', () => { | ||
| Object.assign(_redisOptions, { maxCacheKeyLength: 10 }); | ||
|
|
||
| cacheResponseHook(mockSpan, 'get', ['cache:very-long-key-name'], 'value'); | ||
|
|
||
| expect(mockSpan.updateName).toHaveBeenCalledWith('cache:very...'); | ||
| }); | ||
|
|
||
| it('should truncate multiple keys joined with commas', () => { | ||
| Object.assign(_redisOptions, { maxCacheKeyLength: 20 }); | ||
|
|
||
| cacheResponseHook(mockSpan, 'mget', ['cache:key1', 'cache:key2', 'cache:key3'], ['val1', 'val2', 'val3']); | ||
|
|
||
| expect(mockSpan.updateName).toHaveBeenCalledWith('cache:key1, cache:ke...'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getCacheKeySafely (single arg)', () => { | ||
| it('should return an empty string if there are no command arguments', () => { | ||
| const result = getCacheKeySafely('get', []); | ||
|
|
@@ -26,7 +103,7 @@ describe('Redis', () => { | |
| expect(result).toStrictEqual(['key1']); | ||
| }); | ||
|
|
||
| it('should return only the key for multiple arguments', () => { | ||
| it('should return only the first key for commands that only accept a singe key (get)', () => { | ||
| const cmdArgs = ['key1', 'the-value']; | ||
| const result = getCacheKeySafely('get', cmdArgs); | ||
| expect(result).toStrictEqual(['key1']); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.