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..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 @@ -48,12 +48,13 @@ 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({ + '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, @@ -61,27 +62,43 @@ 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', + 'cache.key': ['ioredis-cache:test-key'], 'cache.item_size': 10, 'network.peer.address': 'localhost', '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', + 'cache.key': ['ioredis-cache:unavailable-data'], + 'network.peer.address': 'localhost', + '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..7424215ee726 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -10,32 +10,13 @@ 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 { + GET_COMMANDS, + calculateCacheItemSize, + getCacheKeySafely, + getCacheOperation, + shouldConsiderForCache, +} from '../../utils/redisCache'; interface RedisOptions { cachePrefixes?: string[]; @@ -48,11 +29,18 @@ let _redisOptions: RedisOptions = {}; export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { return new IORedisInstrumentation({ responseHook: (span, redisCommand, cmdArgs, response) => { - const key = cmdArgs[0]; + const safeKey = getCacheKeySafely(redisCommand, cmdArgs); span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis'); - if (!_redisOptions?.cachePrefixes || !shouldConsiderForCache(redisCommand, key, _redisOptions.cachePrefixes)) { + const cacheOperation = getCacheOperation(redisCommand); + + if ( + !safeKey || + !cacheOperation || + !_redisOptions?.cachePrefixes || + !shouldConsiderForCache(redisCommand, safeKey, _redisOptions.cachePrefixes) + ) { // not relevant for cache return; } @@ -66,25 +54,23 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => { } 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 (cacheItemSize) { + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); + } + + if (GET_COMMANDS.includes(redisCommand) && cacheItemSize !== undefined) { + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); } + + span.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation, + [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, + }); + + 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 new file mode 100644 index 000000000000..4064252c8e7a --- /dev/null +++ b/packages/node/src/utils/redisCache.ts @@ -0,0 +1,89 @@ +import type { CommandArgs as IORedisCommandArgs } from '@opentelemetry/instrumentation-ioredis'; +import { flatten } from '@sentry/utils'; + +const SINGLE_ARG_COMMANDS = ['get', 'set', 'setex']; + +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( + command: string, +): 'cache.get' | 'cache.put' | 'cache.remove' | 'cache.flush' | undefined { + const lowercaseStatement = command.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(redisCommand: string, cmdArgs: IORedisCommandArgs): string[] | undefined { + try { + if (cmdArgs.length === 0) { + return undefined; + } + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const processArg = (arg: string | Buffer | number | any[]): string[] => { + if (typeof arg === 'string' || typeof arg === 'number' || Buffer.isBuffer(arg)) { + return [arg.toString()]; + } else if (Array.isArray(arg)) { + return flatten(arg.map(arg => processArg(arg))); + } else { + return ['']; + } + }; + + if (SINGLE_ARG_COMMANDS.includes(redisCommand) && cmdArgs.length > 0) { + return processArg(cmdArgs[0]); + } + + return flatten(cmdArgs.map(arg => processArg(arg))); + } catch (e) { + 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 { + if (!getCacheOperation(redisCommand)) { + return false; + } + + return key.reduce((prev, key) => { + return prev || keyHasPrefix(key, prefixes); + }, false); +} + +/** Calculates size based on the cache response value */ +export function calculateCacheItemSize(response: unknown): number | 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 new file mode 100644 index 000000000000..307991f24a73 --- /dev/null +++ b/packages/node/test/integrations/tracing/redis.test.ts @@ -0,0 +1,229 @@ +import { + GET_COMMANDS, + SET_COMMANDS, + calculateCacheItemSize, + getCacheKeySafely, + shouldConsiderForCache, +} from '../../../src/utils/redisCache'; + +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(undefined); + }); + + it('should return a string representation of a single argument', () => { + const cmdArgs = ['key1']; + const result = getCacheKeySafely('get', cmdArgs); + 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).toStrictEqual(['key1']); + }); + + it('should handle number arguments', () => { + const cmdArgs = [1, 'the-value']; + const result = getCacheKeySafely('get', cmdArgs); + expect(result).toStrictEqual(['1']); + }); + + it('should handle Buffer arguments', () => { + const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; + const result = getCacheKeySafely('get', cmdArgs); + expect(result).toStrictEqual(['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).toStrictEqual(['']); + }); + }); + + 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).toStrictEqual(['key1', 'key2', 'key3']); + }); + + it('should handle Buffer arguments', () => { + const cmdArgs = [Buffer.from('key1'), Buffer.from('key2')]; + const result = getCacheKeySafely('mget', cmdArgs); + expect(result).toStrictEqual(['key1', 'key2']); + }); + + it('should handle array arguments', () => { + const cmdArgs = [ + ['key1', 'key2'], + ['key3', 'key4'], + ]; + const result = getCacheKeySafely('mget', cmdArgs); + 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).toStrictEqual(['key1', 'key2', 'key3', 'key4', 'key5', 'key6', '7', 'key8']); + }); + + it('should handle nested arrays with mixed types in arguments', () => { + const cmdArgs = [ + ['key1', 'key2'], + ['key3', 'key4', [Buffer.from('key5'), ['key6']]], + ]; + const result = getCacheKeySafely('mget', cmdArgs); + expect(result).toStrictEqual(['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('mget', cmdArgs); + expect(result).toStrictEqual(['']); + }); + }); + + 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); + }); + }); + }); + + 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(); + }); + }); +}); diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 9888e8b5b0dd..78fc009ec195 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,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. const dbSystem = attributes[SEMATTRS_DB_SYSTEM]; - if (dbSystem) { + 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 }); } 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); + }); +});