Skip to content

Commit 8bdf16e

Browse files
s1gr1dbillyvg
s1gr1d
authored and
billyvg
committed
Merge pull request #12348 from getsentry/sig/ioredis-statements
feat(redis): Support `mget` command in caching functionality
2 parents bc64f58 + 1ae3157 commit 8bdf16e

File tree

9 files changed

+464
-57
lines changed

9 files changed

+464
-57
lines changed

‎dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ async function run() {
3030
await redis.get('test-key');
3131
await redis.get('ioredis-cache:test-key');
3232
await redis.get('ioredis-cache:unavailable-data');
33+
34+
await redis.mget('test-key', 'ioredis-cache:test-key', 'ioredis-cache:unavailable-data');
3335
} finally {
3436
await redis.disconnect();
3537
}

‎dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,40 +48,57 @@ describe('redis cache auto instrumentation', () => {
4848
spans: expect.arrayContaining([
4949
// SET
5050
expect.objectContaining({
51-
description: 'set ioredis-cache:test-key [1 other arguments]',
51+
description: 'ioredis-cache:test-key',
5252
op: 'cache.put',
5353
origin: 'auto.db.otel.redis',
5454
data: expect.objectContaining({
55+
'sentry.origin': 'auto.db.otel.redis',
5556
'db.statement': 'set ioredis-cache:test-key [1 other arguments]',
56-
'cache.key': 'ioredis-cache:test-key',
57+
'cache.key': ['ioredis-cache:test-key'],
5758
'cache.item_size': 2,
5859
'network.peer.address': 'localhost',
5960
'network.peer.port': 6379,
6061
}),
6162
}),
6263
// GET
6364
expect.objectContaining({
64-
description: 'get ioredis-cache:test-key',
65-
op: 'cache.get_item', // todo: will be changed to cache.get
65+
description: 'ioredis-cache:test-key',
66+
op: 'cache.get',
6667
origin: 'auto.db.otel.redis',
6768
data: expect.objectContaining({
69+
'sentry.origin': 'auto.db.otel.redis',
6870
'db.statement': 'get ioredis-cache:test-key',
6971
'cache.hit': true,
70-
'cache.key': 'ioredis-cache:test-key',
72+
'cache.key': ['ioredis-cache:test-key'],
7173
'cache.item_size': 10,
7274
'network.peer.address': 'localhost',
7375
'network.peer.port': 6379,
7476
}),
7577
}),
76-
// GET (unavailable)
78+
// GET (unavailable - no cache hit)
7779
expect.objectContaining({
78-
description: 'get ioredis-cache:unavailable-data',
79-
op: 'cache.get_item', // todo: will be changed to cache.get
80+
description: 'ioredis-cache:unavailable-data',
81+
op: 'cache.get',
8082
origin: 'auto.db.otel.redis',
8183
data: expect.objectContaining({
84+
'sentry.origin': 'auto.db.otel.redis',
8285
'db.statement': 'get ioredis-cache:unavailable-data',
8386
'cache.hit': false,
84-
'cache.key': 'ioredis-cache:unavailable-data',
87+
'cache.key': ['ioredis-cache:unavailable-data'],
88+
'network.peer.address': 'localhost',
89+
'network.peer.port': 6379,
90+
}),
91+
}),
92+
// MGET
93+
expect.objectContaining({
94+
description: 'test-key, ioredis-cache:test-key, ioredis-cache:unavailable-data',
95+
op: 'cache.get',
96+
origin: 'auto.db.otel.redis',
97+
data: expect.objectContaining({
98+
'sentry.origin': 'auto.db.otel.redis',
99+
'db.statement': 'mget [3 other arguments]',
100+
'cache.hit': true,
101+
'cache.key': ['test-key', 'ioredis-cache:test-key', 'ioredis-cache:unavailable-data'],
85102
'network.peer.address': 'localhost',
86103
'network.peer.port': 6379,
87104
}),

‎packages/node/src/integrations/tracing/redis.ts

Lines changed: 32 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -10,32 +10,13 @@ import {
1010
} from '@sentry/core';
1111
import type { IntegrationFn } from '@sentry/types';
1212
import { generateInstrumentOnce } from '../../otel/instrument';
13-
14-
function keyHasPrefix(key: string, prefixes: string[]): boolean {
15-
return prefixes.some(prefix => key.startsWith(prefix));
16-
}
17-
18-
/** Currently, caching only supports 'get' and 'set' commands. More commands will be added (setex, mget, del, expire) */
19-
function shouldConsiderForCache(
20-
redisCommand: string,
21-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
22-
key: string | number | any[] | Buffer,
23-
prefixes: string[],
24-
): boolean {
25-
return (redisCommand === 'get' || redisCommand === 'set') && typeof key === 'string' && keyHasPrefix(key, prefixes);
26-
}
27-
28-
function calculateCacheItemSize(response: unknown): number | undefined {
29-
try {
30-
if (Buffer.isBuffer(response)) return response.byteLength;
31-
else if (typeof response === 'string') return response.length;
32-
else if (typeof response === 'number') return response.toString().length;
33-
else if (response === null || response === undefined) return 0;
34-
return JSON.stringify(response).length;
35-
} catch (e) {
36-
return undefined;
37-
}
38-
}
13+
import {
14+
GET_COMMANDS,
15+
calculateCacheItemSize,
16+
getCacheKeySafely,
17+
getCacheOperation,
18+
shouldConsiderForCache,
19+
} from '../../utils/redisCache';
3920

4021
interface RedisOptions {
4122
cachePrefixes?: string[];
@@ -48,11 +29,18 @@ let _redisOptions: RedisOptions = {};
4829
export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => {
4930
return new IORedisInstrumentation({
5031
responseHook: (span, redisCommand, cmdArgs, response) => {
51-
const key = cmdArgs[0];
32+
const safeKey = getCacheKeySafely(redisCommand, cmdArgs);
5233

5334
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis');
5435

55-
if (!_redisOptions?.cachePrefixes || !shouldConsiderForCache(redisCommand, key, _redisOptions.cachePrefixes)) {
36+
const cacheOperation = getCacheOperation(redisCommand);
37+
38+
if (
39+
!safeKey ||
40+
!cacheOperation ||
41+
!_redisOptions?.cachePrefixes ||
42+
!shouldConsiderForCache(redisCommand, safeKey, _redisOptions.cachePrefixes)
43+
) {
5644
// not relevant for cache
5745
return;
5846
}
@@ -66,25 +54,23 @@ export const instrumentRedis = generateInstrumentOnce(INTEGRATION_NAME, () => {
6654
}
6755

6856
const cacheItemSize = calculateCacheItemSize(response);
69-
if (cacheItemSize) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize);
70-
71-
if (typeof key === 'string') {
72-
switch (redisCommand) {
73-
case 'get':
74-
span.setAttributes({
75-
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.get_item', // todo: will be changed to cache.get
76-
[SEMANTIC_ATTRIBUTE_CACHE_KEY]: key,
77-
});
78-
if (cacheItemSize !== undefined) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0);
79-
break;
80-
case 'set':
81-
span.setAttributes({
82-
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.put',
83-
[SEMANTIC_ATTRIBUTE_CACHE_KEY]: key,
84-
});
85-
break;
86-
}
57+
58+
if (cacheItemSize) {
59+
span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize);
60+
}
61+
62+
if (GET_COMMANDS.includes(redisCommand) && cacheItemSize !== undefined) {
63+
span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0);
8764
}
65+
66+
span.setAttributes({
67+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation,
68+
[SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey,
69+
});
70+
71+
const spanDescription = safeKey.join(', ');
72+
73+
span.updateName(spanDescription.length > 1024 ? `${spanDescription.substring(0, 1024)}...` : spanDescription);
8874
},
8975
});
9076
});

‎packages/node/src/utils/redisCache.ts

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
import type { CommandArgs as IORedisCommandArgs } from '@opentelemetry/instrumentation-ioredis';
2+
import { flatten } from '@sentry/utils';
3+
4+
const SINGLE_ARG_COMMANDS = ['get', 'set', 'setex'];
5+
6+
export const GET_COMMANDS = ['get', 'mget'];
7+
export const SET_COMMANDS = ['set' /* todo: 'setex' */];
8+
// todo: del, expire
9+
10+
/** Determine cache operation based on redis statement */
11+
export function getCacheOperation(
12+
command: string,
13+
): 'cache.get' | 'cache.put' | 'cache.remove' | 'cache.flush' | undefined {
14+
const lowercaseStatement = command.toLowerCase();
15+
16+
if (GET_COMMANDS.includes(lowercaseStatement)) {
17+
return 'cache.get';
18+
} else if (SET_COMMANDS.includes(lowercaseStatement)) {
19+
return 'cache.put';
20+
} else {
21+
return undefined;
22+
}
23+
}
24+
25+
function keyHasPrefix(key: string, prefixes: string[]): boolean {
26+
return prefixes.some(prefix => key.startsWith(prefix));
27+
}
28+
29+
/** Safely converts a redis key to a string (comma-separated if there are multiple keys) */
30+
export function getCacheKeySafely(redisCommand: string, cmdArgs: IORedisCommandArgs): string[] | undefined {
31+
try {
32+
if (cmdArgs.length === 0) {
33+
return undefined;
34+
}
35+
36+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
37+
const processArg = (arg: string | Buffer | number | any[]): string[] => {
38+
if (typeof arg === 'string' || typeof arg === 'number' || Buffer.isBuffer(arg)) {
39+
return [arg.toString()];
40+
} else if (Array.isArray(arg)) {
41+
return flatten(arg.map(arg => processArg(arg)));
42+
} else {
43+
return ['<unknown>'];
44+
}
45+
};
46+
47+
if (SINGLE_ARG_COMMANDS.includes(redisCommand) && cmdArgs.length > 0) {
48+
return processArg(cmdArgs[0]);
49+
}
50+
51+
return flatten(cmdArgs.map(arg => processArg(arg)));
52+
} catch (e) {
53+
return undefined;
54+
}
55+
}
56+
57+
/** Determines whether a redis operation should be considered as "cache operation" by checking if a key is prefixed.
58+
* We only support certain commands (such as 'set', 'get', 'mget'). */
59+
export function shouldConsiderForCache(redisCommand: string, key: string[], prefixes: string[]): boolean {
60+
if (!getCacheOperation(redisCommand)) {
61+
return false;
62+
}
63+
64+
return key.reduce((prev, key) => {
65+
return prev || keyHasPrefix(key, prefixes);
66+
}, false);
67+
}
68+
69+
/** Calculates size based on the cache response value */
70+
export function calculateCacheItemSize(response: unknown): number | undefined {
71+
const getSize = (value: unknown): number | undefined => {
72+
try {
73+
if (Buffer.isBuffer(value)) return value.byteLength;
74+
else if (typeof value === 'string') return value.length;
75+
else if (typeof value === 'number') return value.toString().length;
76+
else if (value === null || value === undefined) return 0;
77+
return JSON.stringify(value).length;
78+
} catch (e) {
79+
return undefined;
80+
}
81+
};
82+
83+
return Array.isArray(response)
84+
? response.reduce((acc: number | undefined, curr) => {
85+
const size = getSize(curr);
86+
return typeof size === 'number' ? (acc !== undefined ? acc + size : size) : acc;
87+
}, 0)
88+
: getSize(response);
89+
}

0 commit comments

Comments
 (0)