feat(redis-cache): Create cache-span with prefixed keys (get/set commands) (#12070)

s1gr1d · mydea · web-flow · commit 257bcb05ddc8 · 2024-05-16T16:34:17.000+02:00
Populates the OTel span with cache attributes. Currently, `get` and
`set` commands are considered.

---------

Co-authored-by: Francesco Novy &lt;francesco.novy@sentry.io&gt;
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
@@ -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
@@ -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 Span',
+      op: 'test-span',
+    },
+    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
@@ -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 Span',
+      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 Span',
+      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:unavailable-data',
+            '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/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
@@ -7,12 +7,13 @@ 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]',
           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
@@ -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
@@ -1,14 +1,89 @@
 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 | 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;
+  }
+}
+
+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;
+            }
+
+            // 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) {
+              span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort });
+            }
+
+            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;
+              }
+            }
+          },
+        }),
         // todo: implement them gradually
         // new LegacyRedisInstrumentation({}),
         // new RedisInstrumentation({}),
