-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(node): use diagnostics_channel for redis >= 5.12.0 #20573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
b22b6ca
0f7e6f6
f88ffb0
4a0b785
992feac
a57c3bf
5e6ea30
340e44a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| version: '3.9' | ||
|
|
||
| services: | ||
| db: | ||
| image: redis:latest | ||
| restart: always | ||
| container_name: integration-tests-redis-dc | ||
| ports: | ||
| - '6379:6379' | ||
| healthcheck: | ||
| test: ['CMD-SHELL', 'redis-cli ping | grep -q PONG'] | ||
| interval: 2s | ||
| timeout: 3s | ||
| retries: 30 | ||
| start_period: 5s |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| 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: ['dc-cache:'] })], | ||
| }); | ||
|
|
||
| // Stop the process from exiting before the transaction is sent | ||
| setInterval(() => {}, 1000); | ||
|
|
||
| async function run() { | ||
| // Yield a microtick so the DC subscriber (deferred via Promise.resolve().then) | ||
| // is registered before node-redis eagerly creates its native TracingChannels on require(). | ||
| await Promise.resolve(); | ||
|
|
||
| const { createClient } = require('redis-5'); | ||
| const redisClient = await createClient({ socket: { host: '127.0.0.1', port: 6379 } }).connect(); | ||
|
|
||
| await Sentry.startSpan( | ||
| { | ||
| name: 'Test Span Redis 5 DC', | ||
| op: 'test-span-redis-5-dc', | ||
| }, | ||
| async () => { | ||
| try { | ||
| await redisClient.set('dc-test-key', 'test-value'); | ||
| await redisClient.set('dc-cache:test-key', 'test-value'); | ||
|
|
||
| await redisClient.set('dc-cache:test-key-ex', 'test-value', { EX: 10 }); | ||
|
|
||
| await redisClient.get('dc-test-key'); | ||
| await redisClient.get('dc-cache:test-key'); | ||
| await redisClient.get('dc-cache:unavailable-data'); | ||
|
|
||
| await redisClient.mGet(['dc-test-key', 'dc-cache:test-key', 'dc-cache:unavailable-data']); | ||
| } finally { | ||
| await redisClient.disconnect(); | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| run(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| import { afterAll, describe, expect, test } from 'vitest'; | ||
| import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; | ||
|
|
||
| describe('redis v5 diagnostics_channel auto instrumentation', () => { | ||
| afterAll(() => { | ||
| cleanupChildProcesses(); | ||
| }); | ||
|
|
||
| test('should create spans for redis v5 commands via diagnostics_channel', { timeout: 60_000 }, async () => { | ||
| const EXPECTED_TRANSACTION = { | ||
| transaction: 'Test Span Redis 5 DC', | ||
| spans: expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| op: 'db.redis', | ||
| origin: 'auto.db.redis.diagnostic_channel', | ||
| data: expect.objectContaining({ | ||
| 'sentry.op': 'db.redis', | ||
| 'sentry.origin': 'auto.db.redis.diagnostic_channel', | ||
| 'db.system': 'redis', | ||
| 'db.statement': 'SET dc-test-key [1 other arguments]', | ||
| }), | ||
| }), | ||
| // cache SET: span name updated to key by cacheResponseHook | ||
| expect.objectContaining({ | ||
| description: 'dc-cache:test-key', | ||
| op: 'cache.put', | ||
| origin: 'auto.db.redis.diagnostic_channel', | ||
| data: expect.objectContaining({ | ||
| 'sentry.origin': 'auto.db.redis.diagnostic_channel', | ||
| 'db.statement': 'SET dc-cache:test-key [1 other arguments]', | ||
| 'cache.key': ['dc-cache:test-key'], | ||
| 'cache.item_size': 2, | ||
| }), | ||
| }), | ||
| // cache SET with EX option: redis v5 sends SET key value EX 10 as the command | ||
| expect.objectContaining({ | ||
| description: 'dc-cache:test-key-ex', | ||
| op: 'cache.put', | ||
| origin: 'auto.db.redis.diagnostic_channel', | ||
| data: expect.objectContaining({ | ||
| 'sentry.origin': 'auto.db.redis.diagnostic_channel', | ||
| 'db.statement': 'SET dc-cache:test-key-ex [3 other arguments]', | ||
| 'cache.key': ['dc-cache:test-key-ex'], | ||
| 'cache.item_size': 2, | ||
| }), | ||
| }), | ||
| expect.objectContaining({ | ||
| op: 'db.redis', | ||
| origin: 'auto.db.redis.diagnostic_channel', | ||
| data: expect.objectContaining({ | ||
| 'sentry.op': 'db.redis', | ||
| 'sentry.origin': 'auto.db.redis.diagnostic_channel', | ||
| 'db.system': 'redis', | ||
| 'db.statement': 'GET dc-test-key', | ||
| }), | ||
| }), | ||
| // cache GET (hit) | ||
| expect.objectContaining({ | ||
| description: 'dc-cache:test-key', | ||
| op: 'cache.get', | ||
| origin: 'auto.db.redis.diagnostic_channel', | ||
| data: expect.objectContaining({ | ||
| 'sentry.origin': 'auto.db.redis.diagnostic_channel', | ||
| 'db.statement': 'GET dc-cache:test-key', | ||
| 'cache.hit': true, | ||
| 'cache.key': ['dc-cache:test-key'], | ||
| 'cache.item_size': 10, | ||
| }), | ||
| }), | ||
| // cache GET (miss) | ||
| expect.objectContaining({ | ||
| description: 'dc-cache:unavailable-data', | ||
| op: 'cache.get', | ||
| origin: 'auto.db.redis.diagnostic_channel', | ||
| data: expect.objectContaining({ | ||
| 'sentry.origin': 'auto.db.redis.diagnostic_channel', | ||
| 'db.statement': 'GET dc-cache:unavailable-data', | ||
| 'cache.hit': false, | ||
| 'cache.key': ['dc-cache:unavailable-data'], | ||
| }), | ||
| }), | ||
| // MGET: node-redis sanitizes args for diagnostics_channel (keys become '?'), | ||
| // so cache detection cannot match prefixes — remains a plain db.redis span. | ||
| expect.objectContaining({ | ||
| op: 'db.redis', | ||
| origin: 'auto.db.redis.diagnostic_channel', | ||
| data: expect.objectContaining({ | ||
| 'sentry.op': 'db.redis', | ||
| 'sentry.origin': 'auto.db.redis.diagnostic_channel', | ||
| 'db.system': 'redis', | ||
| 'db.statement': 'MGET [3 other arguments]', | ||
| }), | ||
| }), | ||
| ]), | ||
| }; | ||
|
|
||
| // node-redis emits a node-redis:connect DC event for the initial connection. | ||
| // That fires before startSpan so it arrives as the first envelope. | ||
| const EXPECTED_CONNECT = { | ||
| transaction: 'redis-connect', | ||
| }; | ||
|
|
||
| await createRunner(__dirname, 'scenario-redis-5.js') | ||
| .withDockerCompose({ workingDirectory: [__dirname] }) | ||
| .expect({ transaction: EXPECTED_CONNECT }) | ||
| .expect({ transaction: EXPECTED_TRANSACTION }) | ||
| .start() | ||
| .completed(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ import { | |
| SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, | ||
| SEMANTIC_ATTRIBUTE_CACHE_KEY, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
| spanToJSON, | ||
| truncate, | ||
| } from '@sentry/core'; | ||
|
|
@@ -23,6 +22,7 @@ import { | |
| import type { IORedisResponseCustomAttributeFunction } from './vendored/types'; | ||
| import { IORedisInstrumentation } from './vendored/ioredis-instrumentation'; | ||
| import { RedisInstrumentation } from './vendored/redis-instrumentation'; | ||
| import { subscribeRedisDiagnosticChannels } from './redis-dc-subscriber'; | ||
|
|
||
| interface RedisOptions { | ||
| /** | ||
|
|
@@ -53,8 +53,6 @@ export const cacheResponseHook: IORedisResponseCustomAttributeFunction = ( | |
| cmdArgs: IORedisCommandArgs, | ||
| response: unknown, | ||
| ) => { | ||
| span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis'); | ||
|
|
||
|
cursor[bot] marked this conversation as resolved.
|
||
| const safeKey = getCacheKeySafely(redisCommand, cmdArgs); | ||
| const cacheOperation = getCacheOperation(redisCommand); | ||
|
|
||
|
|
@@ -120,6 +118,11 @@ export const instrumentRedis = Object.assign( | |
| (): void => { | ||
| instrumentIORedis(); | ||
| instrumentRedisModule(); | ||
| // node-redis >= 5.12.0 publishes via diagnostics_channel. The subscriber uses | ||
| // `@sentry/opentelemetry/tracing-channel`, which needs the Sentry OTel context manager | ||
| // to be registered before it can `bindStore`. `initOpenTelemetry()` runs after integration | ||
| // `setupOnce`, so defer to the next tick. | ||
| void Promise.resolve().then(() => subscribeRedisDiagnosticChannels(cacheResponseHook)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: Would this work the same with CJS and ESM in Node? I wonder if there is a better way, than defer it to the next tick, like a "afterSetup" hook or so
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, it works the same. We have an Let me give There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preload path permanently breaks DC context propagationMedium Severity When Additional Locations (1)Reviewed by Cursor Bugbot for commit 340e44a. Configure here. |
||
|
|
||
| // todo: implement them gradually | ||
| // new LegacyRedisInstrumentation({}), | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: I think it would make sense to add 2 tests. One for redis 5 with tracing channels and another without them. Then we would know if we break something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are subtle differences between span counts with 5.12 and
<5.12as a result of some decisions in the redis tracing channel PR.But yea we should have both to make sure we capture correctly.