Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(server-utils): Dedupe ioredis orchestrion span for offline-queued commands#22279
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -67,6 +67,36 @@ function connectionAttributes(host: string | undefined, port: number | undefined | ||
| }; | ||
| } | ||
| // ioredis re-enters `sendCommand` with the same command object when it drains | ||
| // the offline queue on connect which leads to duplicate spans. | ||
| // Track commands we've already traced so each logical command produces one span. | ||
| const tracedCommands = new WeakSet<object>(); | ||
| /** | ||
| * Builds the db span for an `orchestrion:ioredis:command` payload, or returns `undefined` to skip | ||
| * it: for a non-command payload, or the offline-queue re-send of an already-traced command. | ||
| * | ||
| * Exported for unit testing. | ||
| */ | ||
| export function startIORedisCommandSpan(data: IORedisCommandContext): Span | undefined { | ||
| const command = data.arguments?.[0] as RedisCommand | undefined; | ||
| if (!command || typeof command !== 'object') { | ||
| return undefined; | ||
| } | ||
| // guard against duplicate spans | ||
| if (tracedCommands.has(command)) { | ||
| return undefined; | ||
| } | ||
| tracedCommands.add(command); | ||
| const { host, port } = getConnectionOptions(data.self); | ||
| const statement = defaultDbStatementSerializer(command.name, command.args ?? []); | ||
| return startInactiveSpan({ | ||
| name: statement, | ||
| op: 'db', | ||
| attributes: { ...connectionAttributes(host, port), [DB_STATEMENT]: statement }, | ||
| }); | ||
| } | ||
| const _ioredisChannelIntegration = ((options: IORedisChannelIntegrationOptions = {}) => { | ||
| const responseHook = options.responseHook; | ||
| @@ -92,35 +122,19 @@ const _ioredisChannelIntegration = ((options: IORedisChannelIntegrationOptions = | ||
| // binding that `initOpenTelemetry()` registers after integration `setupOnce` — | ||
| // defer until it's available (matches the native redis diagnostics-channel subscriber). | ||
| waitForTracingChannelBinding(() => { | ||
| bindTracingChannelToSpan( | ||
| commandChannel, | ||
| data => { | ||
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. all of this is just refactored to | ||
| bindTracingChannelToSpan(commandChannel, startIORedisCommandSpan, { | ||
| // ioredis' `requireParentSpan` default: only create a span under an active span. | ||
| requiresParentSpan: true, | ||
| beforeSpanEnd(span, data) { | ||
| if ('error' in data || !responseHook) { | ||
| return; | ||
| } | ||
| const command = data.arguments?.[0] as RedisCommand | undefined; | ||
| if (!command || typeof command !== 'object') { | ||
| return undefined; | ||
| if (command) { | ||
| runResponseHook(responseHook, span, command, data.result); | ||
| } | ||
| const { host, port } = getConnectionOptions(data.self); | ||
| const statement = defaultDbStatementSerializer(command.name, command.args ?? []); | ||
| return startInactiveSpan({ | ||
| name: statement, | ||
| op: 'db', | ||
| attributes: { ...connectionAttributes(host, port), [DB_STATEMENT]: statement }, | ||
| }); | ||
| }, | ||
| { | ||
| // ioredis' `requireParentSpan` default: only create a span under an active span. | ||
| requiresParentSpan: true, | ||
| beforeSpanEnd(span, data) { | ||
| if ('error' in data || !responseHook) { | ||
| return; | ||
| } | ||
| const command = data.arguments?.[0] as RedisCommand | undefined; | ||
| if (command) { | ||
| runResponseHook(responseHook, span, command, data.result); | ||
| } | ||
| }, | ||
| }, | ||
| ); | ||
| }); | ||
| bindTracingChannelToSpan( | ||
| connectChannel, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import type { Span } from '@sentry/core'; | ||
| import * as SentryCore from '@sentry/core'; | ||
| import { afterEach, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest'; | ||
| import { startIORedisCommandSpan } from '../../../src/integrations/tracing-channel/ioredis'; | ||
| const CONNECTION = { host: 'localhost', port: 6379 }; | ||
| function ctx(command: unknown): { arguments: unknown[]; self: { options: typeof CONNECTION } } { | ||
| return { arguments: [command], self: { options: CONNECTION } }; | ||
| } | ||
| describe('startIORedisCommandSpan', () => { | ||
| let startInactiveSpanSpy: MockInstance; | ||
| beforeEach(() => { | ||
| startInactiveSpanSpy = vi.spyOn(SentryCore, 'startInactiveSpan').mockReturnValue({} as Span); | ||
| }); | ||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
| it('builds a db span with the orchestrion origin and stable db/net attributes', () => { | ||
| startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] })); | ||
| expect(startInactiveSpanSpy).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| name: 'set test-key [1 other arguments]', | ||
| op: 'db', | ||
| attributes: expect.objectContaining({ | ||
| 'db.system': 'redis', | ||
| 'db.connection_string': 'redis://localhost:6379', | ||
| 'net.peer.name': 'localhost', | ||
| 'net.peer.port': 6379, | ||
| 'db.statement': 'set test-key [1 other arguments]', | ||
| 'sentry.origin': 'auto.db.orchestrion.redis', | ||
| }), | ||
| }), | ||
| ); | ||
| }); | ||
| it('emits a single span when the same command is re-sent from the offline queue', () => { | ||
| const command = { name: 'set', args: ['test-key', 'test-value'] }; | ||
| expect(startIORedisCommandSpan(ctx(command))).toBeDefined(); | ||
| expect(startIORedisCommandSpan(ctx(command))).toBeUndefined(); | ||
| expect(startInactiveSpanSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
| it('spans distinct command objects with the same statement', () => { | ||
| startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] })); | ||
| startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] })); | ||
| expect(startInactiveSpanSpy).toHaveBeenCalledTimes(2); | ||
| }); | ||
| it('skips payloads without a command object', () => { | ||
| expect(startIORedisCommandSpan({ arguments: [], self: { options: CONNECTION } })).toBeUndefined(); | ||
| expect(startInactiveSpanSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
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.
does this already trigger this case?