review comments

Author: s1gr1d

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,
           }),

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);
     },
   });
 });

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('<unknown>');
       }
-    });
+    };
+
+    cmdArgs.forEach(processArg);
 
-    return keys.join(',');
+    return keys.join(', ');
   } catch (e) {
     return '';
   }

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 <unknown> 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('<unknown>');
     });
   });