From 0d9856e4cf4fec23e56fccb32344fffd91ea90bc Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Tue, 4 Jun 2024 14:06:21 +0200 Subject: [PATCH 01/11] feat(redis): Support `mget` command in caching functionality --- .../tracing/redis-cache/scenario-ioredis.js | 2 + .../suites/tracing/redis-cache/test.ts | 31 +++- .../node/src/integrations/tracing/redis.ts | 69 +++----- packages/node/src/utils/redisCache.ts | 77 +++++++++ .../test/integrations/tracing/redis.test.ts | 152 ++++++++++++++++++ .../src/utils/parseSpanDescription.ts | 9 +- 6 files changed, 286 insertions(+), 54 deletions(-) create mode 100644 packages/node/src/utils/redisCache.ts create mode 100644 packages/node/test/integrations/tracing/redis.test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js index 22385f81b064..18f06dc67c29 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js @@ -30,6 +30,8 @@ async function run() { await redis.get('test-key'); await redis.get('ioredis-cache:test-key'); await redis.get('ioredis-cache:unavailable-data'); + + await redis.mget('test-key', 'ioredis-cache:test-key', 'ioredis-cache:unavailable-data'); } finally { await redis.disconnect(); } diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts index 3ad860bb72f4..740c85e6d6e0 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -48,11 +48,12 @@ describe('redis cache auto instrumentation', () => { spans: expect.arrayContaining([ // SET expect.objectContaining({ - description: 'set ioredis-cache:test-key [1 other arguments]', + description: 'ioredis-cache:test-key', op: 'cache.put', origin: 'auto.db.otel.redis', data: expect.objectContaining({ - 'db.statement': 'set ioredis-cache:test-key [1 other arguments]', + 'sentry.origin': 'auto.db.otel.redis', + 'db.statement': 'ioredis-cache:test-key [1 other arguments]', 'cache.key': 'ioredis-cache:test-key', 'cache.item_size': 2, 'network.peer.address': 'localhost', @@ -61,10 +62,11 @@ describe('redis cache auto instrumentation', () => { }), // GET expect.objectContaining({ - description: 'get ioredis-cache:test-key', - op: 'cache.get_item', // todo: will be changed to cache.get + description: 'ioredis-cache:test-key', + op: 'cache.get', origin: 'auto.db.otel.redis', data: expect.objectContaining({ + 'sentry.origin': 'auto.db.otel.redis', 'db.statement': 'get ioredis-cache:test-key', 'cache.hit': true, 'cache.key': 'ioredis-cache:test-key', @@ -73,12 +75,13 @@ describe('redis cache auto instrumentation', () => { 'network.peer.port': 6379, }), }), - // GET (unavailable) + // GET (unavailable - no cache hit) expect.objectContaining({ - description: 'get ioredis-cache:unavailable-data', - op: 'cache.get_item', // todo: will be changed to cache.get + description: 'ioredis-cache:unavailable-data', + op: 'cache.get', origin: 'auto.db.otel.redis', data: expect.objectContaining({ + 'sentry.origin': 'auto.db.otel.redis', 'db.statement': 'get ioredis-cache:unavailable-data', 'cache.hit': false, 'cache.key': 'ioredis-cache:unavailable-data', @@ -86,6 +89,20 @@ describe('redis cache auto instrumentation', () => { 'network.peer.port': 6379, }), }), + // MGET + expect.objectContaining({ + description: 'test-key,ioredis-cache:test-key,ioredis-cache:unavailable-data', + op: 'cache.get', + origin: 'auto.db.otel.redis', + data: expect.objectContaining({ + 'sentry.origin': 'auto.db.otel.redis', + 'db.statement': 'mget [3 other arguments]', + 'cache.hit': true, + 'cache.key': 'test-key,ioredis-cache:test-key,ioredis-cache:unavailable-data', + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), ]), }; diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 1379336412f6..0a3924e98412 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -10,32 +10,12 @@ import { } from '@sentry/core'; import type { IntegrationFn } from '@sentry/types'; import { generateInstrumentOnce } from '../../otel/instrument'; - -function keyHasPrefix(key: string, prefixes: string[]): boolean { - return prefixes.some(prefix => key.startsWith(prefix)); -} - -/** Currently, caching only supports 'get' and 'set' commands. More commands will be added (setex, mget, del, expire) */ -function shouldConsiderForCache( - redisCommand: string, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - key: string | number | any[] | Buffer, - prefixes: string[], -): boolean { - return (redisCommand === 'get' || redisCommand === 'set') && typeof key === 'string' && keyHasPrefix(key, prefixes); -} - -function calculateCacheItemSize(response: unknown): number | undefined { - try { - if (Buffer.isBuffer(response)) return response.byteLength; - else if (typeof response === 'string') return response.length; - else if (typeof response === 'number') return response.toString().length; - else if (response === null || response === undefined) return 0; - return JSON.stringify(response).length; - } catch (e) { - return undefined; - } -} +import { + calculateCacheItemSize, + getCacheKeySafely, + getCacheOperation, + shouldConsiderForCache, +} from '../../utils/redisCache'; interface RedisOptions { cachePrefixes?: string[]; @@ -48,11 +28,14 @@ let _redisOptions: RedisOptions = {}; export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { return new IORedisInstrumentation({ responseHook: (span, redisCommand, cmdArgs, response) => { - const key = cmdArgs[0]; + const safeKey = getCacheKeySafely(cmdArgs); span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis'); - if (!_redisOptions?.cachePrefixes || !shouldConsiderForCache(redisCommand, key, _redisOptions.cachePrefixes)) { + if ( + !_redisOptions?.cachePrefixes || + !shouldConsiderForCache(redisCommand, safeKey, _redisOptions.cachePrefixes) + ) { // not relevant for cache return; } @@ -65,26 +48,22 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort }); } + const cacheOperation = getCacheOperation(redisCommand); + + if (!cacheOperation) return; // redis command unsupported as cache operation + const cacheItemSize = calculateCacheItemSize(response); if (cacheItemSize) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); - if (typeof key === 'string') { - switch (redisCommand) { - case 'get': - span.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.get_item', // todo: will be changed to cache.get - [SEMANTIC_ATTRIBUTE_CACHE_KEY]: key, - }); - if (cacheItemSize !== undefined) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); - break; - case 'set': - span.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.put', - [SEMANTIC_ATTRIBUTE_CACHE_KEY]: key, - }); - break; - } - } + if (cacheOperation === 'cache.get' && cacheItemSize !== undefined) + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); + + span.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation, + [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, + }); + + span.updateName(`${safeKey.substring(0, 1024)}...`); }, }); }); diff --git a/packages/node/src/utils/redisCache.ts b/packages/node/src/utils/redisCache.ts new file mode 100644 index 000000000000..bbb7b302f4a8 --- /dev/null +++ b/packages/node/src/utils/redisCache.ts @@ -0,0 +1,77 @@ +import type { CommandArgs as IORedisCommandArgs } from '@opentelemetry/instrumentation-ioredis'; + +export const GET_COMMANDS = ['get', 'mget']; +export const SET_COMMANDS = ['set' /* todo: 'setex' */]; +// todo: del, expire + +/** Determine cache operation based on redis statement */ +export function getCacheOperation( + statement: string, +): 'cache.get' | 'cache.put' | 'cache.remove' | 'cache.flush' | undefined { + const lowercaseStatement = statement.toLowerCase(); + + if (GET_COMMANDS.includes(lowercaseStatement)) { + return 'cache.get'; + } else if (SET_COMMANDS.includes(lowercaseStatement)) { + return 'cache.put'; + } else { + return undefined; + } +} + +function keyHasPrefix(key: string, prefixes: string[]): boolean { + return prefixes.some(prefix => key.startsWith(prefix)); +} + +/** Safely converts a redis key to a string (comma-separated if there are multiple keys) */ +export function getCacheKeySafely(cmdArgs: IORedisCommandArgs): string { + try { + if (cmdArgs.length === 0) { + return ''; + } + + const keys: string[] = []; + + cmdArgs.forEach(arg => { + if (typeof arg === 'string' || typeof arg === 'number') { + keys.push(arg.toString()); + } else if (Buffer.isBuffer(arg)) { + keys.push(arg.toString()); + } else if (Array.isArray(arg)) { + arg.forEach(subArg => { + if (typeof subArg === 'string' || typeof subArg === 'number') { + keys.push(subArg.toString()); + } else if (Buffer.isBuffer(subArg)) { + keys.push(subArg.toString()); + } + }); + } + }); + + return keys.join(','); + } catch (e) { + return ''; + } +} + +/** Determines whether a redis operation should be considered as "cache operation" by checking if a key is prefixed. + * We only support certain commands (such as 'set', 'get', 'mget'). */ +export function shouldConsiderForCache(redisCommand: string, key: string, prefixes: string[]): boolean { + const lowercaseCommand = redisCommand.toLowerCase(); + if (!SET_COMMANDS.includes(lowercaseCommand) && !GET_COMMANDS.includes(lowercaseCommand)) return false; + + return key.split(',').reduce((prev, key) => prev || keyHasPrefix(key, prefixes), false); +} + +/** Calculates size based on the cache response value */ +export function calculateCacheItemSize(response: unknown): number | undefined { + try { + if (Buffer.isBuffer(response)) return response.byteLength; + else if (typeof response === 'string') return response.length; + else if (typeof response === 'number') return response.toString().length; + else if (response === null || response === undefined) return 0; + return JSON.stringify(response).length; + } catch (e) { + return undefined; + } +} diff --git a/packages/node/test/integrations/tracing/redis.test.ts b/packages/node/test/integrations/tracing/redis.test.ts new file mode 100644 index 000000000000..ab226f9e7358 --- /dev/null +++ b/packages/node/test/integrations/tracing/redis.test.ts @@ -0,0 +1,152 @@ +import { + GET_COMMANDS, + SET_COMMANDS, + calculateCacheItemSize, + getCacheKeySafely, + shouldConsiderForCache, +} from '../../../src/utils/redisCache'; + +describe('Redis', () => { + describe('getCacheKeySafely', () => { + it('should return an empty string if there are no command arguments', () => { + const result = getCacheKeySafely([]); + expect(result).toBe(''); + }); + + it('should return a string representation of a single argument', () => { + const cmdArgs = ['key1']; + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe('key1'); + }); + + it('should return a comma-separated string for multiple arguments', () => { + const cmdArgs = ['key1', 'key2', 'key3']; + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe('key1,key2,key3'); + }); + + it('should handle number arguments', () => { + const cmdArgs = [1, 2, 3]; + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe('1,2,3'); + }); + + it('should handle Buffer arguments', () => { + const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe('key1,key2'); + }); + + it('should handle array arguments', () => { + const cmdArgs = [ + ['key1', 'key2'], + ['key3', 'key4'], + ]; + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe('key1,key2,key3,key4'); + }); + + it('should handle mixed type arguments', () => { + const cmdArgs = [Buffer.from('key1'), ['key2', 'key3'], [Buffer.from('key4'), 'key5', 'key6', 7]]; + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe('key1,key2,key3,key4,key5,key6,7'); + }); + + it('should return an empty string if an error occurs', () => { + const cmdArgs = [Symbol('key1')]; // Symbols cannot be converted to a string + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe(''); + }); + }); + + describe('calculateCacheItemSize', () => { + it('should return byte length if response is a Buffer', () => { + const response = Buffer.from('test'); + const result = calculateCacheItemSize(response); + expect(result).toBe(response.byteLength); + }); + + it('should return string length if response is a string', () => { + const response = 'test'; + const result = calculateCacheItemSize(response); + expect(result).toBe(response.length); + }); + + it('should return length of string representation if response is a number', () => { + const response = 1234; + const result = calculateCacheItemSize(response); + expect(result).toBe(response.toString().length); + }); + + it('should return 0 if response is null or undefined', () => { + const response = null; + const result = calculateCacheItemSize(response); + expect(result).toBe(0); + }); + + it('should return length of JSON stringified response if response is an object', () => { + const response = { key: 'value' }; + const result = calculateCacheItemSize(response); + expect(result).toBe(JSON.stringify(response).length); + }); + + it('should return undefined if an error occurs', () => { + const circularObject: { self?: any } = {}; + circularObject.self = circularObject; // This will cause JSON.stringify to throw an error + const result = calculateCacheItemSize(circularObject); + expect(result).toBeUndefined(); + }); + }); + + describe('shouldConsiderForCache', () => { + const prefixes = ['cache:', 'ioredis-cache:']; + + it('should return false for non-cache commands', () => { + const command = 'EXISTS'; + const commandLowercase = 'exists'; + const key = 'cache:test-key'; + const result1 = shouldConsiderForCache(command, key, prefixes); + const result2 = shouldConsiderForCache(commandLowercase, key, prefixes); + expect(result1).toBe(false); + expect(result2).toBe(false); + }); + + it('should return true for cache commands with matching prefix', () => { + const command = 'get'; + const key = 'cache:test-key'; + const result = shouldConsiderForCache(command, key, prefixes); + expect(result).toBe(true); + }); + + it('should return false for cache commands without matching prefix', () => { + const command = 'get'; + const key = 'test-key'; + const result = shouldConsiderForCache(command, key, prefixes); + expect(result).toBe(false); + }); + + it('should return true for multiple keys with at least one matching prefix', () => { + const command = 'mget'; + const key = 'test-key,cache:test-key'; + const result = shouldConsiderForCache(command, key, prefixes); + expect(result).toBe(true); + }); + + it('should return false for multiple keys without any matching prefix', () => { + const command = 'mget'; + const key = 'test-key,test-key2'; + const result = shouldConsiderForCache(command, key, prefixes); + expect(result).toBe(false); + }); + + GET_COMMANDS.concat(SET_COMMANDS).forEach(command => { + it(`should return true for ${command} command with matching prefix`, () => { + const key = 'cache:test-key'; + const result = shouldConsiderForCache(command, key, prefixes); + expect(result).toBe(true); + }); + }); + }); +}); diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 9888e8b5b0dd..2955dc84ca39 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -14,6 +14,7 @@ import { import type { TransactionSource } from '@sentry/types'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; +import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '@sentry/core'; import type { AbstractSpan } from '../types'; import { getSpanKind } from './getSpanKind'; import { spanHasAttributes, spanHasName } from './spanTypes'; @@ -43,9 +44,13 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { return descriptionForHttpMethod({ attributes, name, kind: getSpanKind(span) }, httpMethod); } - // If db.type exists then this is a database call span. + // If db.type exists then this is a database call span (unless OP is cache) const dbSystem = attributes[SEMATTRS_DB_SYSTEM]; - if (dbSystem) { + if ( + dbSystem && + typeof attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] === 'string' && + !attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP].startsWith('cache.') + ) { return descriptionForDbSystem({ attributes, name }); } From 320855bb9f14943de3c8036ce3f05940e5d3450e Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Tue, 4 Jun 2024 14:21:29 +0200 Subject: [PATCH 02/11] fix tests --- packages/opentelemetry/src/utils/parseSpanDescription.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 2955dc84ca39..80bef2c4865a 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -48,8 +48,9 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { const dbSystem = attributes[SEMATTRS_DB_SYSTEM]; if ( dbSystem && - typeof attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] === 'string' && - !attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP].startsWith('cache.') + (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || + (typeof attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] === 'string' && + !attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP].startsWith('cache.'))) ) { return descriptionForDbSystem({ attributes, name }); } From 3ded60f9edb6d536600cbee009503179ce772963 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Tue, 4 Jun 2024 14:58:14 +0200 Subject: [PATCH 03/11] review comments --- .../suites/tracing/redis-cache/test.ts | 4 +-- .../node/src/integrations/tracing/redis.ts | 14 +++++++--- packages/node/src/utils/redisCache.ts | 23 +++++++--------- .../test/integrations/tracing/redis.test.ts | 27 ++++++++++++------- 4 files changed, 40 insertions(+), 28 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts index 740c85e6d6e0..8e1db732cfcc 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -91,14 +91,14 @@ describe('redis cache auto instrumentation', () => { }), // MGET expect.objectContaining({ - description: 'test-key,ioredis-cache:test-key,ioredis-cache:unavailable-data', + description: 'test-key, ioredis-cache:test-key, ioredis-cache:unavailable-data', op: 'cache.get', origin: 'auto.db.otel.redis', data: expect.objectContaining({ 'sentry.origin': 'auto.db.otel.redis', 'db.statement': 'mget [3 other arguments]', 'cache.hit': true, - 'cache.key': 'test-key,ioredis-cache:test-key,ioredis-cache:unavailable-data', + 'cache.key': 'test-key, ioredis-cache:test-key, ioredis-cache:unavailable-data', 'network.peer.address': 'localhost', 'network.peer.port': 6379, }), diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 0a3924e98412..cf4b27a677aa 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -50,20 +50,26 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { const cacheOperation = getCacheOperation(redisCommand); - if (!cacheOperation) return; // redis command unsupported as cache operation + if (!cacheOperation) { + // redis command unsupported as cache operation + return; + } const cacheItemSize = calculateCacheItemSize(response); - if (cacheItemSize) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); + if (cacheItemSize) { + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); + } - if (cacheOperation === 'cache.get' && cacheItemSize !== undefined) + if (cacheOperation === 'cache.get' && cacheItemSize !== undefined) { span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); + } span.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation, [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, }); - span.updateName(`${safeKey.substring(0, 1024)}...`); + span.updateName(safeKey.length > 1024 ? `${safeKey.substring(0, 1024)}...` : safeKey); }, }); }); diff --git a/packages/node/src/utils/redisCache.ts b/packages/node/src/utils/redisCache.ts index bbb7b302f4a8..a3a547204872 100644 --- a/packages/node/src/utils/redisCache.ts +++ b/packages/node/src/utils/redisCache.ts @@ -32,23 +32,20 @@ export function getCacheKeySafely(cmdArgs: IORedisCommandArgs): string { const keys: string[] = []; - cmdArgs.forEach(arg => { - if (typeof arg === 'string' || typeof arg === 'number') { - keys.push(arg.toString()); - } else if (Buffer.isBuffer(arg)) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const processArg = (arg: string | Buffer | number | any[]): void => { + if (typeof arg === 'string' || typeof arg === 'number' || Buffer.isBuffer(arg)) { keys.push(arg.toString()); } else if (Array.isArray(arg)) { - arg.forEach(subArg => { - if (typeof subArg === 'string' || typeof subArg === 'number') { - keys.push(subArg.toString()); - } else if (Buffer.isBuffer(subArg)) { - keys.push(subArg.toString()); - } - }); + arg.forEach(processArg); + } else { + keys.push(''); } - }); + }; + + cmdArgs.forEach(processArg); - return keys.join(','); + return keys.join(', '); } catch (e) { return ''; } diff --git a/packages/node/test/integrations/tracing/redis.test.ts b/packages/node/test/integrations/tracing/redis.test.ts index ab226f9e7358..03c69e78771d 100644 --- a/packages/node/test/integrations/tracing/redis.test.ts +++ b/packages/node/test/integrations/tracing/redis.test.ts @@ -22,19 +22,19 @@ describe('Redis', () => { it('should return a comma-separated string for multiple arguments', () => { const cmdArgs = ['key1', 'key2', 'key3']; const result = getCacheKeySafely(cmdArgs); - expect(result).toBe('key1,key2,key3'); + expect(result).toBe('key1, key2, key3'); }); it('should handle number arguments', () => { const cmdArgs = [1, 2, 3]; const result = getCacheKeySafely(cmdArgs); - expect(result).toBe('1,2,3'); + expect(result).toBe('1, 2, 3'); }); it('should handle Buffer arguments', () => { const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; const result = getCacheKeySafely(cmdArgs); - expect(result).toBe('key1,key2'); + expect(result).toBe('key1, key2'); }); it('should handle array arguments', () => { @@ -43,21 +43,30 @@ describe('Redis', () => { ['key3', 'key4'], ]; const result = getCacheKeySafely(cmdArgs); - expect(result).toBe('key1,key2,key3,key4'); + expect(result).toBe('key1, key2, key3, key4'); }); it('should handle mixed type arguments', () => { - const cmdArgs = [Buffer.from('key1'), ['key2', 'key3'], [Buffer.from('key4'), 'key5', 'key6', 7]]; + const cmdArgs = [Buffer.from('key1'), ['key2', 'key3'], [Buffer.from('key4'), 'key5', 'key6', 7, ['key8']]]; const result = getCacheKeySafely(cmdArgs); - expect(result).toBe('key1,key2,key3,key4,key5,key6,7'); + expect(result).toBe('key1, key2, key3, key4, key5, key6, 7, key8'); }); - it('should return an empty string if an error occurs', () => { - const cmdArgs = [Symbol('key1')]; // Symbols cannot be converted to a string + it('should handle nested arrays in arguments', () => { + const cmdArgs = [ + ['key1', 'key2'], + ['key3', 'key4', ['key5', ['key6']]], + ]; + const result = getCacheKeySafely(cmdArgs); + expect(result).toBe('key1, key2, key3, key4, key5, key6'); + }); + + it('should return if the arg type is not supported', () => { + const cmdArgs = [Symbol('key1')]; // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore const result = getCacheKeySafely(cmdArgs); - expect(result).toBe(''); + expect(result).toBe(''); }); }); From e4618cb7ee5df48b95c9433b4b2145af91e87bfa Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Tue, 4 Jun 2024 17:43:17 +0200 Subject: [PATCH 04/11] review comments --- .../suites/tracing/redis-cache/test.ts | 2 +- packages/node/src/utils/redisCache.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts index 8e1db732cfcc..1d3151286294 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -53,7 +53,7 @@ describe('redis cache auto instrumentation', () => { origin: 'auto.db.otel.redis', data: expect.objectContaining({ 'sentry.origin': 'auto.db.otel.redis', - 'db.statement': 'ioredis-cache:test-key [1 other arguments]', + 'db.statement': 'set ioredis-cache:test-key [1 other arguments]', 'cache.key': 'ioredis-cache:test-key', 'cache.item_size': 2, 'network.peer.address': 'localhost', diff --git a/packages/node/src/utils/redisCache.ts b/packages/node/src/utils/redisCache.ts index a3a547204872..b30bd1ead2c5 100644 --- a/packages/node/src/utils/redisCache.ts +++ b/packages/node/src/utils/redisCache.ts @@ -54,8 +54,9 @@ export function getCacheKeySafely(cmdArgs: IORedisCommandArgs): string { /** Determines whether a redis operation should be considered as "cache operation" by checking if a key is prefixed. * We only support certain commands (such as 'set', 'get', 'mget'). */ export function shouldConsiderForCache(redisCommand: string, key: string, prefixes: string[]): boolean { - const lowercaseCommand = redisCommand.toLowerCase(); - if (!SET_COMMANDS.includes(lowercaseCommand) && !GET_COMMANDS.includes(lowercaseCommand)) return false; + if (!getCacheOperation(redisCommand)) { + return false; + } return key.split(',').reduce((prev, key) => prev || keyHasPrefix(key, prefixes), false); } From 78147818417b53e3630502c7731150273b561dc2 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Wed, 5 Jun 2024 10:18:26 +0200 Subject: [PATCH 05/11] fix single arg commands --- .../node/src/integrations/tracing/redis.ts | 2 +- packages/node/src/utils/redisCache.ts | 22 +++++--- .../test/integrations/tracing/redis.test.ts | 56 +++++++++++++------ 3 files changed, 54 insertions(+), 26 deletions(-) diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index cf4b27a677aa..3b942f2aefbf 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -28,7 +28,7 @@ let _redisOptions: RedisOptions = {}; export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { return new IORedisInstrumentation({ responseHook: (span, redisCommand, cmdArgs, response) => { - const safeKey = getCacheKeySafely(cmdArgs); + const safeKey = getCacheKeySafely(redisCommand, cmdArgs); span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis'); diff --git a/packages/node/src/utils/redisCache.ts b/packages/node/src/utils/redisCache.ts index b30bd1ead2c5..c105a65abc50 100644 --- a/packages/node/src/utils/redisCache.ts +++ b/packages/node/src/utils/redisCache.ts @@ -1,5 +1,7 @@ import type { CommandArgs as IORedisCommandArgs } from '@opentelemetry/instrumentation-ioredis'; +const SINGLE_ARG_COMMANDS = ['get', 'set', 'setex']; + export const GET_COMMANDS = ['get', 'mget']; export const SET_COMMANDS = ['set' /* todo: 'setex' */]; // todo: del, expire @@ -24,28 +26,32 @@ function keyHasPrefix(key: string, prefixes: string[]): boolean { } /** Safely converts a redis key to a string (comma-separated if there are multiple keys) */ -export function getCacheKeySafely(cmdArgs: IORedisCommandArgs): string { +export function getCacheKeySafely(redisCommand: string, cmdArgs: IORedisCommandArgs): string { try { if (cmdArgs.length === 0) { return ''; } - const keys: string[] = []; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const joinArgsWithComma = (acc: string, currArg: string | Buffer | number | any[]): string => + acc.length === 0 ? processArg(currArg) : `${acc}, ${processArg(currArg)}`; // eslint-disable-next-line @typescript-eslint/no-explicit-any - const processArg = (arg: string | Buffer | number | any[]): void => { + const processArg = (arg: string | Buffer | number | any[]): string => { if (typeof arg === 'string' || typeof arg === 'number' || Buffer.isBuffer(arg)) { - keys.push(arg.toString()); + return arg.toString(); } else if (Array.isArray(arg)) { - arg.forEach(processArg); + return arg.reduce(joinArgsWithComma, ''); } else { - keys.push(''); + return ''; } }; - cmdArgs.forEach(processArg); + if (SINGLE_ARG_COMMANDS.includes(redisCommand) && cmdArgs.length > 0) { + return processArg(cmdArgs[0]); + } - return keys.join(', '); + return cmdArgs.reduce(joinArgsWithComma, ''); } catch (e) { return ''; } diff --git a/packages/node/test/integrations/tracing/redis.test.ts b/packages/node/test/integrations/tracing/redis.test.ts index 03c69e78771d..579d3d5d9e87 100644 --- a/packages/node/test/integrations/tracing/redis.test.ts +++ b/packages/node/test/integrations/tracing/redis.test.ts @@ -7,33 +7,55 @@ import { } from '../../../src/utils/redisCache'; describe('Redis', () => { - describe('getCacheKeySafely', () => { + describe('getCacheKeySafely (single arg)', () => { it('should return an empty string if there are no command arguments', () => { - const result = getCacheKeySafely([]); + const result = getCacheKeySafely('get', []); expect(result).toBe(''); }); it('should return a string representation of a single argument', () => { const cmdArgs = ['key1']; - const result = getCacheKeySafely(cmdArgs); + const result = getCacheKeySafely('get', cmdArgs); expect(result).toBe('key1'); }); - it('should return a comma-separated string for multiple arguments', () => { - const cmdArgs = ['key1', 'key2', 'key3']; - const result = getCacheKeySafely(cmdArgs); - expect(result).toBe('key1, key2, key3'); + it('should return only the key for multiple arguments', () => { + const cmdArgs = ['key1', 'the-value']; + const result = getCacheKeySafely('get', cmdArgs); + expect(result).toBe('key1'); }); it('should handle number arguments', () => { - const cmdArgs = [1, 2, 3]; - const result = getCacheKeySafely(cmdArgs); - expect(result).toBe('1, 2, 3'); + const cmdArgs = [1, 'the-value']; + const result = getCacheKeySafely('get', cmdArgs); + expect(result).toBe('1'); + }); + + it('should handle Buffer arguments', () => { + const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; + const result = getCacheKeySafely('get', cmdArgs); + expect(result).toBe('key1'); + }); + + it('should return if the arg type is not supported', () => { + const cmdArgs = [Symbol('key1')]; + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const result = getCacheKeySafely('get', cmdArgs); + expect(result).toBe(''); + }); + }); + + describe('getCacheKeySafely (multiple args)', () => { + it('should return a comma-separated string for multiple arguments with mget command', () => { + const cmdArgs = ['key1', 'key2', 'key3']; + const result = getCacheKeySafely('mget', cmdArgs); + expect(result).toBe('key1, key2, key3'); }); it('should handle Buffer arguments', () => { const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; - const result = getCacheKeySafely(cmdArgs); + const result = getCacheKeySafely('mget', cmdArgs); expect(result).toBe('key1, key2'); }); @@ -42,22 +64,22 @@ describe('Redis', () => { ['key1', 'key2'], ['key3', 'key4'], ]; - const result = getCacheKeySafely(cmdArgs); + const result = getCacheKeySafely('mget', cmdArgs); expect(result).toBe('key1, key2, key3, key4'); }); it('should handle mixed type arguments', () => { const cmdArgs = [Buffer.from('key1'), ['key2', 'key3'], [Buffer.from('key4'), 'key5', 'key6', 7, ['key8']]]; - const result = getCacheKeySafely(cmdArgs); + const result = getCacheKeySafely('mget', cmdArgs); expect(result).toBe('key1, key2, key3, key4, key5, key6, 7, key8'); }); - it('should handle nested arrays in arguments', () => { + it('should handle nested arrays with mixed types in arguments', () => { const cmdArgs = [ ['key1', 'key2'], - ['key3', 'key4', ['key5', ['key6']]], + ['key3', 'key4', [Buffer.from('key5'), ['key6']]], ]; - const result = getCacheKeySafely(cmdArgs); + const result = getCacheKeySafely('mget', cmdArgs); expect(result).toBe('key1, key2, key3, key4, key5, key6'); }); @@ -65,7 +87,7 @@ describe('Redis', () => { const cmdArgs = [Symbol('key1')]; // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore - const result = getCacheKeySafely(cmdArgs); + const result = getCacheKeySafely('mget', cmdArgs); expect(result).toBe(''); }); }); From d0f8f84eb349c3e3c037ad478582f4035a94b261 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Wed, 5 Jun 2024 11:49:10 +0200 Subject: [PATCH 06/11] change if condition --- .../opentelemetry/src/utils/parseSpanDescription.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 80bef2c4865a..86f91213d3ea 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -14,7 +14,7 @@ import { import type { TransactionSource } from '@sentry/types'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_CACHE_HIT } from '@sentry/core'; import type { AbstractSpan } from '../types'; import { getSpanKind } from './getSpanKind'; import { spanHasAttributes, spanHasName } from './spanTypes'; @@ -44,14 +44,9 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { return descriptionForHttpMethod({ attributes, name, kind: getSpanKind(span) }, httpMethod); } - // If db.type exists then this is a database call span (unless OP is cache) + // If db.type exists then this is a database call span (unless it is cache) const dbSystem = attributes[SEMATTRS_DB_SYSTEM]; - if ( - dbSystem && - (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || - (typeof attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] === 'string' && - !attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP].startsWith('cache.'))) - ) { + if (dbSystem && typeof attributes[SEMANTIC_ATTRIBUTE_CACHE_HIT] !== 'boolean') { return descriptionForDbSystem({ attributes, name }); } From d9e29ee97faaca5aaf307d4bed8cadf21b3feba1 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Wed, 5 Jun 2024 12:26:35 +0200 Subject: [PATCH 07/11] set cache hit for all get commands --- packages/node/src/integrations/tracing/redis.ts | 3 ++- .../opentelemetry/src/utils/parseSpanDescription.ts | 11 ++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 3b942f2aefbf..8425cfd80825 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -11,6 +11,7 @@ import { import type { IntegrationFn } from '@sentry/types'; import { generateInstrumentOnce } from '../../otel/instrument'; import { + GET_COMMANDS, calculateCacheItemSize, getCacheKeySafely, getCacheOperation, @@ -60,7 +61,7 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); } - if (cacheOperation === 'cache.get' && cacheItemSize !== undefined) { + if (GET_COMMANDS.includes(redisCommand) && cacheItemSize !== undefined) { span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); } diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 86f91213d3ea..78fc009ec195 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -14,7 +14,7 @@ import { import type { TransactionSource } from '@sentry/types'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; -import { SEMANTIC_ATTRIBUTE_CACHE_HIT } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '@sentry/core'; import type { AbstractSpan } from '../types'; import { getSpanKind } from './getSpanKind'; import { spanHasAttributes, spanHasName } from './spanTypes'; @@ -44,9 +44,14 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { return descriptionForHttpMethod({ attributes, name, kind: getSpanKind(span) }, httpMethod); } - // If db.type exists then this is a database call span (unless it is cache) const dbSystem = attributes[SEMATTRS_DB_SYSTEM]; - if (dbSystem && typeof attributes[SEMANTIC_ATTRIBUTE_CACHE_HIT] !== 'boolean') { + const opIsCache = + typeof attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] === 'string' && + attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP].startsWith('cache.'); + + // If db.type exists then this is a database call span + // If the Redis DB is used as a cache, the span description should not be changed + if (dbSystem && (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !opIsCache)) { return descriptionForDbSystem({ attributes, name }); } From 144cbfc84a8e0e0b8003679b8aea11d61c1b2ada Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Wed, 5 Jun 2024 13:28:15 +0200 Subject: [PATCH 08/11] accept arrays in item size calculation --- packages/node/src/utils/redisCache.ts | 27 +++++++---- .../test/integrations/tracing/redis.test.ts | 46 +++++++++++++++++++ 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/packages/node/src/utils/redisCache.ts b/packages/node/src/utils/redisCache.ts index c105a65abc50..8c211949c10d 100644 --- a/packages/node/src/utils/redisCache.ts +++ b/packages/node/src/utils/redisCache.ts @@ -69,13 +69,22 @@ export function shouldConsiderForCache(redisCommand: string, key: string, prefix /** Calculates size based on the cache response value */ export function calculateCacheItemSize(response: unknown): number | undefined { - try { - if (Buffer.isBuffer(response)) return response.byteLength; - else if (typeof response === 'string') return response.length; - else if (typeof response === 'number') return response.toString().length; - else if (response === null || response === undefined) return 0; - return JSON.stringify(response).length; - } catch (e) { - return undefined; - } + const getSize = (value: unknown): number | undefined => { + try { + if (Buffer.isBuffer(value)) return value.byteLength; + else if (typeof value === 'string') return value.length; + else if (typeof value === 'number') return value.toString().length; + else if (value === null || value === undefined) return 0; + return JSON.stringify(value).length; + } catch (e) { + return undefined; + } + }; + + return Array.isArray(response) + ? response.reduce((acc: number | undefined, curr) => { + const size = getSize(curr); + return typeof size === 'number' ? (acc !== undefined ? acc + size : size) : acc; + }, 0) + : getSize(response); } diff --git a/packages/node/test/integrations/tracing/redis.test.ts b/packages/node/test/integrations/tracing/redis.test.ts index 579d3d5d9e87..38685c6034e7 100644 --- a/packages/node/test/integrations/tracing/redis.test.ts +++ b/packages/node/test/integrations/tracing/redis.test.ts @@ -180,4 +180,50 @@ describe('Redis', () => { }); }); }); + + describe('calculateCacheItemSize', () => { + it('should return byte length for Buffer input', () => { + const buffer = Buffer.from('test'); + const result = calculateCacheItemSize(buffer); + expect(result).toBe(4); + }); + + it('should return string length for string input', () => { + const str = 'test'; + const result = calculateCacheItemSize(str); + expect(result).toBe(4); + }); + + it('should return number length for number input', () => { + const num = 1234; + const result = calculateCacheItemSize(num); + expect(result).toBe(4); + }); + + it('should return 0 for null or undefined input', () => { + const resultForNull = calculateCacheItemSize(null); + const resultForUndefined = calculateCacheItemSize(undefined); + expect(resultForNull).toBe(0); + expect(resultForUndefined).toBe(0); + }); + + it('should return total size for array input', () => { + const arr = ['test', Buffer.from('test'), 1234]; + const result = calculateCacheItemSize(arr); + expect(result).toBe(12); + }); + + it('should return JSON string length for object input', () => { + const obj = { key: 'value' }; + const result = calculateCacheItemSize(obj); + expect(result).toBe(15); + }); + + it('should return undefined for circular objects', () => { + const circularObject: { self?: any } = {}; + circularObject.self = circularObject; // This creates a circular reference + const result = calculateCacheItemSize(circularObject); + expect(result).toBeUndefined(); + }); + }); }); From 38c3ec9495dd95ca614e5a83bb9d2822ffe8d234 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Wed, 5 Jun 2024 14:02:21 +0200 Subject: [PATCH 09/11] test something --- packages/node/src/integrations/tracing/redis.ts | 16 +++++++++------- packages/node/src/utils/redisCache.ts | 4 ++-- .../src/utils/parseSpanDescription.ts | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 8425cfd80825..de2a51f1949b 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -33,7 +33,10 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis'); + const cacheOperation = getCacheOperation(redisCommand); + if ( + !cacheOperation || !_redisOptions?.cachePrefixes || !shouldConsiderForCache(redisCommand, safeKey, _redisOptions.cachePrefixes) ) { @@ -49,14 +52,11 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort }); } - const cacheOperation = getCacheOperation(redisCommand); - - if (!cacheOperation) { - // redis command unsupported as cache operation - return; - } - + // eslint-disable-next-line no-console + console.log('response', response); const cacheItemSize = calculateCacheItemSize(response); + // eslint-disable-next-line no-console + console.log('cacheItemSize', cacheItemSize); if (cacheItemSize) { span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); } @@ -71,6 +71,8 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { }); span.updateName(safeKey.length > 1024 ? `${safeKey.substring(0, 1024)}...` : safeKey); + // eslint-disable-next-line no-console + console.log('span',span); }, }); }); diff --git a/packages/node/src/utils/redisCache.ts b/packages/node/src/utils/redisCache.ts index 8c211949c10d..1f1aca35438a 100644 --- a/packages/node/src/utils/redisCache.ts +++ b/packages/node/src/utils/redisCache.ts @@ -8,9 +8,9 @@ export const SET_COMMANDS = ['set' /* todo: 'setex' */]; /** Determine cache operation based on redis statement */ export function getCacheOperation( - statement: string, + command: string, ): 'cache.get' | 'cache.put' | 'cache.remove' | 'cache.flush' | undefined { - const lowercaseStatement = statement.toLowerCase(); + const lowercaseStatement = command.toLowerCase(); if (GET_COMMANDS.includes(lowercaseStatement)) { return 'cache.get'; diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 78fc009ec195..74f97d56fbfc 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -52,7 +52,7 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { // If db.type exists then this is a database call span // If the Redis DB is used as a cache, the span description should not be changed if (dbSystem && (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !opIsCache)) { - return descriptionForDbSystem({ attributes, name }); + // return descriptionForDbSystem({ attributes, name }); } // If rpc.service exists then this is a rpc call span. From f61a939efe8f6fa84732dcc383618f96c0bdb8fe Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 6 Jun 2024 10:15:32 +0200 Subject: [PATCH 10/11] add flatten array util --- packages/utils/src/array.ts | 19 ++++++++++ packages/utils/src/index.ts | 1 + packages/utils/test/array.test.ts | 58 +++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 packages/utils/src/array.ts create mode 100644 packages/utils/test/array.test.ts diff --git a/packages/utils/src/array.ts b/packages/utils/src/array.ts new file mode 100644 index 000000000000..15f08ba541b8 --- /dev/null +++ b/packages/utils/src/array.ts @@ -0,0 +1,19 @@ +export type NestedArray = Array | T>; + +/** Flattens a multi-dimensional array */ +export function flatten(input: NestedArray): T[] { + const result: T[] = []; + + const flattenHelper = (input: NestedArray): void => { + input.forEach((el: T | NestedArray) => { + if (Array.isArray(el)) { + flattenHelper(el as NestedArray); + } else { + result.push(el as T); + } + }); + }; + + flattenHelper(input); + return result; +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 433e29a68d79..822d150dfde1 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -1,4 +1,5 @@ export * from './aggregate-errors'; +export * from './array'; export * from './browser'; export * from './dsn'; export * from './error'; diff --git a/packages/utils/test/array.test.ts b/packages/utils/test/array.test.ts new file mode 100644 index 000000000000..ebf702508585 --- /dev/null +++ b/packages/utils/test/array.test.ts @@ -0,0 +1,58 @@ +import type { NestedArray } from '../src/array'; +import { flatten } from '../src/array'; + +describe('flatten', () => { + it('should return the same array when input is a flat array', () => { + const input = [1, 2, 3, 4]; + const expected = [1, 2, 3, 4]; + expect(flatten(input)).toEqual(expected); + }); + + it('should flatten a nested array of numbers', () => { + const input = [[1, 2, [3]], 4]; + const expected = [1, 2, 3, 4]; + expect(flatten(input)).toEqual(expected); + }); + + it('should flatten a nested array of strings', () => { + const input = [ + ['Hello', 'World'], + ['How', 'Are', 'You'], + ]; + const expected = ['Hello', 'World', 'How', 'Are', 'You']; + expect(flatten(input)).toEqual(expected); + }); + + it('should flatten a nested array of objects', () => { + const input: NestedArray<{ a: number; b?: number } | { b: number; a?: number }> = [ + [{ a: 1 }, { b: 2 }], + [{ a: 3 }, { b: 4 }], + ]; + const expected = [{ a: 1 }, { b: 2 }, { a: 3 }, { b: 4 }]; + expect(flatten(input)).toEqual(expected); + }); + + it('should flatten a mixed type array', () => { + const input: NestedArray = [['a', { b: 2 }, 'c'], 'd']; + const expected = ['a', { b: 2 }, 'c', 'd']; + expect(flatten(input)).toEqual(expected); + }); + + it('should flatten a deeply nested array', () => { + const input = [1, [2, [3, [4, [5]]]]]; + const expected = [1, 2, 3, 4, 5]; + expect(flatten(input)).toEqual(expected); + }); + + it('should return an empty array when input is empty', () => { + const input: any[] = []; + const expected: any[] = []; + expect(flatten(input)).toEqual(expected); + }); + + it('should return the same array when input is a flat array', () => { + const input = [1, 'a', { b: 2 }, 'c', 3]; + const expected = [1, 'a', { b: 2 }, 'c', 3]; + expect(flatten(input)).toEqual(expected); + }); +}); From 2007358c591e5f31bfe486af5c8c6fe7104af87f Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 6 Jun 2024 10:15:56 +0200 Subject: [PATCH 11/11] create array key --- .../suites/tracing/redis-cache/test.ts | 8 ++--- .../node/src/integrations/tracing/redis.ts | 12 +++---- packages/node/src/utils/redisCache.ts | 27 +++++++------- .../test/integrations/tracing/redis.test.ts | 36 +++++++++---------- .../src/utils/parseSpanDescription.ts | 2 +- 5 files changed, 41 insertions(+), 44 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts index 1d3151286294..141c6f8174b1 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -54,7 +54,7 @@ describe('redis cache auto instrumentation', () => { data: expect.objectContaining({ 'sentry.origin': 'auto.db.otel.redis', 'db.statement': 'set ioredis-cache:test-key [1 other arguments]', - 'cache.key': 'ioredis-cache:test-key', + 'cache.key': ['ioredis-cache:test-key'], 'cache.item_size': 2, 'network.peer.address': 'localhost', 'network.peer.port': 6379, @@ -69,7 +69,7 @@ describe('redis cache auto instrumentation', () => { 'sentry.origin': 'auto.db.otel.redis', 'db.statement': 'get ioredis-cache:test-key', 'cache.hit': true, - 'cache.key': 'ioredis-cache:test-key', + 'cache.key': ['ioredis-cache:test-key'], 'cache.item_size': 10, 'network.peer.address': 'localhost', 'network.peer.port': 6379, @@ -84,7 +84,7 @@ describe('redis cache auto instrumentation', () => { 'sentry.origin': 'auto.db.otel.redis', 'db.statement': 'get ioredis-cache:unavailable-data', 'cache.hit': false, - 'cache.key': 'ioredis-cache:unavailable-data', + 'cache.key': ['ioredis-cache:unavailable-data'], 'network.peer.address': 'localhost', 'network.peer.port': 6379, }), @@ -98,7 +98,7 @@ describe('redis cache auto instrumentation', () => { 'sentry.origin': 'auto.db.otel.redis', 'db.statement': 'mget [3 other arguments]', 'cache.hit': true, - 'cache.key': 'test-key, ioredis-cache:test-key, ioredis-cache:unavailable-data', + 'cache.key': ['test-key', 'ioredis-cache:test-key', 'ioredis-cache:unavailable-data'], 'network.peer.address': 'localhost', 'network.peer.port': 6379, }), diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index de2a51f1949b..7424215ee726 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -36,6 +36,7 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { const cacheOperation = getCacheOperation(redisCommand); if ( + !safeKey || !cacheOperation || !_redisOptions?.cachePrefixes || !shouldConsiderForCache(redisCommand, safeKey, _redisOptions.cachePrefixes) @@ -52,11 +53,8 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort }); } - // eslint-disable-next-line no-console - console.log('response', response); const cacheItemSize = calculateCacheItemSize(response); - // eslint-disable-next-line no-console - console.log('cacheItemSize', cacheItemSize); + if (cacheItemSize) { span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); } @@ -70,9 +68,9 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, }); - span.updateName(safeKey.length > 1024 ? `${safeKey.substring(0, 1024)}...` : safeKey); - // eslint-disable-next-line no-console - console.log('span',span); + const spanDescription = safeKey.join(', '); + + span.updateName(spanDescription.length > 1024 ? `${spanDescription.substring(0, 1024)}...` : spanDescription); }, }); }); diff --git a/packages/node/src/utils/redisCache.ts b/packages/node/src/utils/redisCache.ts index 1f1aca35438a..4064252c8e7a 100644 --- a/packages/node/src/utils/redisCache.ts +++ b/packages/node/src/utils/redisCache.ts @@ -1,4 +1,5 @@ import type { CommandArgs as IORedisCommandArgs } from '@opentelemetry/instrumentation-ioredis'; +import { flatten } from '@sentry/utils'; const SINGLE_ARG_COMMANDS = ['get', 'set', 'setex']; @@ -26,24 +27,20 @@ function keyHasPrefix(key: string, prefixes: string[]): boolean { } /** Safely converts a redis key to a string (comma-separated if there are multiple keys) */ -export function getCacheKeySafely(redisCommand: string, cmdArgs: IORedisCommandArgs): string { +export function getCacheKeySafely(redisCommand: string, cmdArgs: IORedisCommandArgs): string[] | undefined { try { if (cmdArgs.length === 0) { - return ''; + return undefined; } // eslint-disable-next-line @typescript-eslint/no-explicit-any - const joinArgsWithComma = (acc: string, currArg: string | Buffer | number | any[]): string => - acc.length === 0 ? processArg(currArg) : `${acc}, ${processArg(currArg)}`; - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const processArg = (arg: string | Buffer | number | any[]): string => { + const processArg = (arg: string | Buffer | number | any[]): string[] => { if (typeof arg === 'string' || typeof arg === 'number' || Buffer.isBuffer(arg)) { - return arg.toString(); + return [arg.toString()]; } else if (Array.isArray(arg)) { - return arg.reduce(joinArgsWithComma, ''); + return flatten(arg.map(arg => processArg(arg))); } else { - return ''; + return ['']; } }; @@ -51,20 +48,22 @@ export function getCacheKeySafely(redisCommand: string, cmdArgs: IORedisCommandA return processArg(cmdArgs[0]); } - return cmdArgs.reduce(joinArgsWithComma, ''); + return flatten(cmdArgs.map(arg => processArg(arg))); } catch (e) { - return ''; + return undefined; } } /** Determines whether a redis operation should be considered as "cache operation" by checking if a key is prefixed. * We only support certain commands (such as 'set', 'get', 'mget'). */ -export function shouldConsiderForCache(redisCommand: string, key: string, prefixes: string[]): boolean { +export function shouldConsiderForCache(redisCommand: string, key: string[], prefixes: string[]): boolean { if (!getCacheOperation(redisCommand)) { return false; } - return key.split(',').reduce((prev, key) => prev || keyHasPrefix(key, prefixes), false); + return key.reduce((prev, key) => { + return prev || keyHasPrefix(key, prefixes); + }, false); } /** Calculates size based on the cache response value */ diff --git a/packages/node/test/integrations/tracing/redis.test.ts b/packages/node/test/integrations/tracing/redis.test.ts index 38685c6034e7..307991f24a73 100644 --- a/packages/node/test/integrations/tracing/redis.test.ts +++ b/packages/node/test/integrations/tracing/redis.test.ts @@ -10,31 +10,31 @@ describe('Redis', () => { describe('getCacheKeySafely (single arg)', () => { it('should return an empty string if there are no command arguments', () => { const result = getCacheKeySafely('get', []); - expect(result).toBe(''); + expect(result).toBe(undefined); }); it('should return a string representation of a single argument', () => { const cmdArgs = ['key1']; const result = getCacheKeySafely('get', cmdArgs); - expect(result).toBe('key1'); + expect(result).toStrictEqual(['key1']); }); it('should return only the key for multiple arguments', () => { const cmdArgs = ['key1', 'the-value']; const result = getCacheKeySafely('get', cmdArgs); - expect(result).toBe('key1'); + expect(result).toStrictEqual(['key1']); }); it('should handle number arguments', () => { const cmdArgs = [1, 'the-value']; const result = getCacheKeySafely('get', cmdArgs); - expect(result).toBe('1'); + expect(result).toStrictEqual(['1']); }); it('should handle Buffer arguments', () => { const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; const result = getCacheKeySafely('get', cmdArgs); - expect(result).toBe('key1'); + expect(result).toStrictEqual(['key1']); }); it('should return if the arg type is not supported', () => { @@ -42,7 +42,7 @@ describe('Redis', () => { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore const result = getCacheKeySafely('get', cmdArgs); - expect(result).toBe(''); + expect(result).toStrictEqual(['']); }); }); @@ -50,13 +50,13 @@ describe('Redis', () => { it('should return a comma-separated string for multiple arguments with mget command', () => { const cmdArgs = ['key1', 'key2', 'key3']; const result = getCacheKeySafely('mget', cmdArgs); - expect(result).toBe('key1, key2, key3'); + expect(result).toStrictEqual(['key1', 'key2', 'key3']); }); it('should handle Buffer arguments', () => { const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; const result = getCacheKeySafely('mget', cmdArgs); - expect(result).toBe('key1, key2'); + expect(result).toStrictEqual(['key1', 'key2']); }); it('should handle array arguments', () => { @@ -65,13 +65,13 @@ describe('Redis', () => { ['key3', 'key4'], ]; const result = getCacheKeySafely('mget', cmdArgs); - expect(result).toBe('key1, key2, key3, key4'); + expect(result).toStrictEqual(['key1', 'key2', 'key3', 'key4']); }); it('should handle mixed type arguments', () => { const cmdArgs = [Buffer.from('key1'), ['key2', 'key3'], [Buffer.from('key4'), 'key5', 'key6', 7, ['key8']]]; const result = getCacheKeySafely('mget', cmdArgs); - expect(result).toBe('key1, key2, key3, key4, key5, key6, 7, key8'); + expect(result).toStrictEqual(['key1', 'key2', 'key3', 'key4', 'key5', 'key6', '7', 'key8']); }); it('should handle nested arrays with mixed types in arguments', () => { @@ -80,7 +80,7 @@ describe('Redis', () => { ['key3', 'key4', [Buffer.from('key5'), ['key6']]], ]; const result = getCacheKeySafely('mget', cmdArgs); - expect(result).toBe('key1, key2, key3, key4, key5, key6'); + expect(result).toStrictEqual(['key1', 'key2', 'key3', 'key4', 'key5', 'key6']); }); it('should return if the arg type is not supported', () => { @@ -88,7 +88,7 @@ describe('Redis', () => { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore const result = getCacheKeySafely('mget', cmdArgs); - expect(result).toBe(''); + expect(result).toStrictEqual(['']); }); }); @@ -137,7 +137,7 @@ describe('Redis', () => { it('should return false for non-cache commands', () => { const command = 'EXISTS'; const commandLowercase = 'exists'; - const key = 'cache:test-key'; + const key = ['cache:test-key']; const result1 = shouldConsiderForCache(command, key, prefixes); const result2 = shouldConsiderForCache(commandLowercase, key, prefixes); expect(result1).toBe(false); @@ -146,35 +146,35 @@ describe('Redis', () => { it('should return true for cache commands with matching prefix', () => { const command = 'get'; - const key = 'cache:test-key'; + const key = ['cache:test-key']; const result = shouldConsiderForCache(command, key, prefixes); expect(result).toBe(true); }); it('should return false for cache commands without matching prefix', () => { const command = 'get'; - const key = 'test-key'; + const key = ['test-key']; const result = shouldConsiderForCache(command, key, prefixes); expect(result).toBe(false); }); it('should return true for multiple keys with at least one matching prefix', () => { const command = 'mget'; - const key = 'test-key,cache:test-key'; + const key = ['test-key', 'cache:test-key']; const result = shouldConsiderForCache(command, key, prefixes); expect(result).toBe(true); }); it('should return false for multiple keys without any matching prefix', () => { const command = 'mget'; - const key = 'test-key,test-key2'; + const key = ['test-key', 'test-key2']; const result = shouldConsiderForCache(command, key, prefixes); expect(result).toBe(false); }); GET_COMMANDS.concat(SET_COMMANDS).forEach(command => { it(`should return true for ${command} command with matching prefix`, () => { - const key = 'cache:test-key'; + const key = ['cache:test-key']; const result = shouldConsiderForCache(command, key, prefixes); expect(result).toBe(true); }); diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 74f97d56fbfc..78fc009ec195 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -52,7 +52,7 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { // If db.type exists then this is a database call span // If the Redis DB is used as a cache, the span description should not be changed if (dbSystem && (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !opIsCache)) { - // return descriptionForDbSystem({ attributes, name }); + return descriptionForDbSystem({ attributes, name }); } // If rpc.service exists then this is a rpc call span.