From 497f196388f8c8e3d8bcb253b513942dd16c3284 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 16 May 2024 14:10:14 +0200 Subject: [PATCH 1/5] feat(redis-cache): Create cache-span with prefixed keys --- .../tracing/redis-cache/docker-compose.yml | 9 ++ .../tracing/redis-cache/scenario-ioredis.js | 41 +++++++++ .../suites/tracing/redis-cache/test.ts | 92 +++++++++++++++++++ .../suites/tracing/redis/test.ts | 2 + packages/core/src/semanticAttributes.ts | 6 ++ .../node/src/integrations/tracing/redis.ts | 77 +++++++++++++++- 6 files changed, 224 insertions(+), 3 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml b/dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml new file mode 100644 index 000000000000..164d5977e33d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml @@ -0,0 +1,9 @@ +version: '3.9' + +services: + db: + image: redis:latest + restart: always + container_name: integration-tests-redis + ports: + - '6379:6379' 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 new file mode 100644 index 000000000000..cd2b38db7a5b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js @@ -0,0 +1,41 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + integrations: [Sentry.redisIntegration({ cachePrefixes: ['ioredis-cache:'] })], +}); + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +const Redis = require('ioredis'); + +const redis = new Redis({ port: 6379 }); + +async function run() { + await Sentry.startSpan( + { + name: 'Test Transaction', + op: 'transaction', + }, + async () => { + try { + await redis.set('test-key', 'test-value'); + await redis.set('ioredis-cache:test-key', 'test-value'); + + await redis.get('test-key'); + await redis.get('ioredis-cache:test-key'); + await redis.get('ioredis-cache:unavailable-data'); + } finally { + await redis.disconnect(); + } + }, + ); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); 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 new file mode 100644 index 000000000000..158f44cf7d71 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -0,0 +1,92 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('redis auto instrumentation', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + test('should not add cache spans when key is not prefixed', done => { + const EXPECTED_TRANSACTION = { + transaction: 'Test Transaction', + spans: expect.arrayContaining([ + expect.objectContaining({ + description: 'set test-key [1 other arguments]', + op: 'db', + data: expect.objectContaining({ + 'sentry.op': 'db', + 'db.system': 'redis', + 'net.peer.name': 'localhost', + 'net.peer.port': 6379, + 'db.statement': 'set test-key [1 other arguments]', + }), + }), + expect.objectContaining({ + description: 'get test-key', + op: 'db', + data: expect.objectContaining({ + 'sentry.op': 'db', + 'db.system': 'redis', + 'net.peer.name': 'localhost', + 'net.peer.port': 6379, + 'db.statement': 'get test-key', + }), + }), + ]), + }; + + createRunner(__dirname, 'scenario-ioredis.js') + .withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); + + test('should create cache spans for prefixed keys', done => { + const EXPECTED_TRANSACTION = { + transaction: 'Test Transaction', + spans: expect.arrayContaining([ + // SET + expect.objectContaining({ + description: 'set ioredis-cache:test-key [1 other arguments]', + op: 'cache.put', + data: expect.objectContaining({ + 'db.statement': 'set ioredis-cache:test-key [1 other arguments]', + 'cache.key': 'ioredis-cache:test-key', + 'cache.item_size': 2, + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + // GET + expect.objectContaining({ + description: 'get ioredis-cache:test-key', + op: 'cache.get_item', // todo: will be changed to cache.get + data: expect.objectContaining({ + 'db.statement': 'get ioredis-cache:test-key', + 'cache.hit': true, + 'cache.key': 'ioredis-cache:test-key', + 'cache.item_size': 10, + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + // GET (unavailable) + expect.objectContaining({ + description: 'get ioredis-cache:unavailable-data', + op: 'cache.get_item', // todo: will be changed to cache.get + data: expect.objectContaining({ + 'db.statement': 'get ioredis-cache:unavailable-data', + 'cache.hit': false, + 'cache.key': 'ioredis-cache:test-key', + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + ]), + }; + + createRunner(__dirname, 'scenario-ioredis.js') + .withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis/test.ts index fd441201cebc..4d7850aa1f43 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis/test.ts @@ -13,6 +13,7 @@ describe('redis auto instrumentation', () => { description: 'set test-key [1 other arguments]', op: 'db', data: expect.objectContaining({ + 'sentry.op': 'db', 'db.system': 'redis', 'net.peer.name': 'localhost', 'net.peer.port': 6379, @@ -23,6 +24,7 @@ describe('redis auto instrumentation', () => { description: 'get test-key', op: 'db', data: expect.objectContaining({ + 'sentry.op': 'db', 'db.system': 'redis', 'net.peer.name': 'localhost', 'net.peer.port': 6379, diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts index 8f46de21a1ca..2c268110854c 100644 --- a/packages/core/src/semanticAttributes.ts +++ b/packages/core/src/semanticAttributes.ts @@ -35,3 +35,9 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE = 'sentry.measurement_v export const SEMANTIC_ATTRIBUTE_PROFILE_ID = 'sentry.profile_id'; export const SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME = 'sentry.exclusive_time'; + +export const SEMANTIC_ATTRIBUTE_CACHE_HIT = 'cache.hit'; + +export const SEMANTIC_ATTRIBUTE_CACHE_KEY = 'cache.key'; + +export const SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE = 'cache.item_size'; diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 8e1eebb83b6a..24d07d87acc3 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -1,14 +1,85 @@ import { IORedisInstrumentation } from '@opentelemetry/instrumentation-ioredis'; -import { defineIntegration } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_CACHE_HIT, + SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, + SEMANTIC_ATTRIBUTE_CACHE_KEY, + SEMANTIC_ATTRIBUTE_SENTRY_OP, + defineIntegration, + spanToJSON, +} from '@sentry/core'; import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; -const _redisIntegration = (() => { +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 { + 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 0; + } +} + +interface RedisOptions { + cachePrefixes?: string[]; +} + +const _redisIntegration = ((options?: RedisOptions) => { return { name: 'Redis', setupOnce() { addOpenTelemetryInstrumentation([ - new IORedisInstrumentation({}), + new IORedisInstrumentation({ + responseHook: (span, redisCommand, cmdArgs, response) => { + const key = cmdArgs[0]; + + if (!options?.cachePrefixes || !shouldConsiderForCache(redisCommand, key, options.cachePrefixes)) { + // not relevant for cache + return; + } + + const networkPeerAddress = spanToJSON(span).data?.['net.peer.name']; + const networkPeerPort = spanToJSON(span).data?.['net.peer.port']; + span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort }); + + const cacheItemSize = calculateCacheItemSize(response); + 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', + [SEMANTIC_ATTRIBUTE_CACHE_KEY]: key, + [SEMANTIC_ATTRIBUTE_CACHE_HIT]: cacheItemSize > 0, + }); + break; + case 'set': + span.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.put', + [SEMANTIC_ATTRIBUTE_CACHE_KEY]: key, + }); + break; + } + } + }, + }), // todo: implement them gradually // new LegacyRedisInstrumentation({}), // new RedisInstrumentation({}), From f98a4a75d428d497511634313e5ac727179b8474 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 16 May 2024 15:04:19 +0200 Subject: [PATCH 2/5] set item size to undefined on error --- packages/node/src/integrations/tracing/redis.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 24d07d87acc3..2139963f6552 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -24,7 +24,7 @@ function shouldConsiderForCache( return (redisCommand === 'get' || redisCommand === 'set') && typeof key === 'string' && keyHasPrefix(key, prefixes); } -function calculateCacheItemSize(response: unknown): number { +function calculateCacheItemSize(response: unknown): number | undefined { try { if (Buffer.isBuffer(response)) return response.byteLength; else if (typeof response === 'string') return response.length; @@ -32,7 +32,7 @@ function calculateCacheItemSize(response: unknown): number { else if (response === null || response === undefined) return 0; return JSON.stringify(response).length; } catch (e) { - return 0; + return undefined; } } @@ -56,19 +56,21 @@ const _redisIntegration = ((options?: RedisOptions) => { const networkPeerAddress = spanToJSON(span).data?.['net.peer.name']; const networkPeerPort = spanToJSON(span).data?.['net.peer.port']; - span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort }); + if (networkPeerPort && networkPeerAddress) { + span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort }); + } const cacheItemSize = calculateCacheItemSize(response); - span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); + 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', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.get_item', // todo: will be changed to cache.get [SEMANTIC_ATTRIBUTE_CACHE_KEY]: key, - [SEMANTIC_ATTRIBUTE_CACHE_HIT]: cacheItemSize > 0, }); + if (cacheItemSize !== undefined) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); break; case 'set': span.setAttributes({ From e43339629c9234aa4b13b59ce6d8b4409ee68eb2 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 16 May 2024 15:09:34 +0200 Subject: [PATCH 3/5] rename startSpan --- .../suites/tracing/redis-cache/scenario-ioredis.js | 4 ++-- .../node-integration-tests/suites/tracing/redis-cache/test.ts | 2 +- .../suites/tracing/redis/scenario-ioredis.js | 4 ++-- .../node-integration-tests/suites/tracing/redis/test.ts | 2 +- packages/node/src/integrations/tracing/redis.ts | 2 ++ 5 files changed, 8 insertions(+), 6 deletions(-) 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 cd2b38db7a5b..22385f81b064 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 @@ -19,8 +19,8 @@ const redis = new Redis({ port: 6379 }); async function run() { await Sentry.startSpan( { - name: 'Test Transaction', - op: 'transaction', + name: 'Test Span', + op: 'test-span', }, async () => { try { 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 158f44cf7d71..fea54b4a69d1 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 @@ -7,7 +7,7 @@ describe('redis auto instrumentation', () => { test('should not add cache spans when key is not prefixed', done => { const EXPECTED_TRANSACTION = { - transaction: 'Test Transaction', + transaction: 'Test Span', spans: expect.arrayContaining([ expect.objectContaining({ description: 'set test-key [1 other arguments]', diff --git a/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js b/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js index 52df06b2a386..4c325c3a6c21 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js +++ b/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js @@ -18,8 +18,8 @@ const redis = new Redis({ port: 6379 }); async function run() { await Sentry.startSpan( { - name: 'Test Transaction', - op: 'transaction', + name: 'Test Span', + op: 'test-span', }, async () => { try { diff --git a/dev-packages/node-integration-tests/suites/tracing/redis/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis/test.ts index 4d7850aa1f43..604b2751f05b 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis/test.ts @@ -7,7 +7,7 @@ describe('redis auto instrumentation', () => { test('should auto-instrument `ioredis` package when using redis.set() and redis.get()', done => { const EXPECTED_TRANSACTION = { - transaction: 'Test Transaction', + transaction: 'Test Span', spans: expect.arrayContaining([ expect.objectContaining({ description: 'set test-key [1 other arguments]', diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 2139963f6552..f9309b2de33e 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -54,6 +54,8 @@ const _redisIntegration = ((options?: RedisOptions) => { return; } + // otel/ioredis seems to be using the old standard, as there was a change to those params: https://github.com/open-telemetry/opentelemetry-specification/issues/3199 + // We are using params based on the docs: https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/ const networkPeerAddress = spanToJSON(span).data?.['net.peer.name']; const networkPeerPort = spanToJSON(span).data?.['net.peer.port']; if (networkPeerPort && networkPeerAddress) { From 5cf9d8096992f67e8d38da214be36d69ac5e310a Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Thu, 16 May 2024 15:42:13 +0200 Subject: [PATCH 4/5] fix test --- .../node-integration-tests/suites/tracing/redis-cache/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 fea54b4a69d1..8a65c140b535 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 @@ -42,7 +42,7 @@ describe('redis auto instrumentation', () => { test('should create cache spans for prefixed keys', done => { const EXPECTED_TRANSACTION = { - transaction: 'Test Transaction', + transaction: 'Test Span', spans: expect.arrayContaining([ // SET expect.objectContaining({ From 1a33f3d171f3692ad89295597e303fe47792f231 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 16:07:06 +0200 Subject: [PATCH 5/5] fix test --- .../node-integration-tests/suites/tracing/redis-cache/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8a65c140b535..0c2beaf7d4c8 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 @@ -76,7 +76,7 @@ describe('redis auto instrumentation', () => { data: expect.objectContaining({ 'db.statement': 'get ioredis-cache:unavailable-data', 'cache.hit': false, - 'cache.key': 'ioredis-cache:test-key', + 'cache.key': 'ioredis-cache:unavailable-data', 'network.peer.address': 'localhost', 'network.peer.port': 6379, }),