From ccaabb41aa777e9bc2023fc511117a4f07251976 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 19:49:12 +0100 Subject: [PATCH 1/3] fix(otel): Make sure we use correct hub on finish --- packages/core/src/tracing/transaction.ts | 12 ++++- .../opentelemetry-node/src/spanprocessor.ts | 23 ++-------- .../test/spanprocessor.test.ts | 46 +++++++++++++++++-- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index b1e6e74195be..b7d0d196bcc2 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -23,7 +23,7 @@ export class Transaction extends SpanClass implements TransactionInterface { /** * The reference to the current hub. */ - public readonly _hub: Hub; + public _hub: Hub; private _name: string; @@ -278,4 +278,14 @@ export class Transaction extends SpanClass implements TransactionInterface { return dsc; } + + /** + * Override the current hub with a new one. + * Used if you want another hub to finish the transaction. + * + * @internal + */ + public setHub(hub: Hub): void { + this._hub = hub; + } } diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index 773c68ebf1ff..ba22acd7e7f6 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -50,17 +50,6 @@ export class SentrySpanProcessor implements OtelSpanProcessor { * @inheritDoc */ public onStart(otelSpan: OtelSpan, parentContext: Context): void { - const hub = getCurrentHub(); - if (!hub) { - __DEBUG_BUILD__ && logger.error('SentrySpanProcessor has triggered onStart before a hub has been setup.'); - return; - } - const scope = hub.getScope(); - if (!scope) { - __DEBUG_BUILD__ && logger.error('SentrySpanProcessor has triggered onStart before a scope has been setup.'); - return; - } - const otelSpanId = otelSpan.spanContext().spanId; const otelParentSpanId = otelSpan.parentSpanId; @@ -79,7 +68,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor { SENTRY_SPAN_PROCESSOR_MAP.set(otelSpanId, sentryChildSpan); } else { const traceCtx = getTraceData(otelSpan, parentContext); - const transaction = hub.startTransaction({ + const transaction = getCurrentHub().startTransaction({ name: otelSpan.name, ...traceCtx, instrumenter: 'otel', @@ -95,12 +84,6 @@ export class SentrySpanProcessor implements OtelSpanProcessor { * @inheritDoc */ public onEnd(otelSpan: OtelSpan): void { - const hub = getCurrentHub(); - if (!hub) { - __DEBUG_BUILD__ && logger.error('SentrySpanProcessor has triggered onEnd before a hub has been setup.'); - return; - } - const otelSpanId = otelSpan.spanContext().spanId; const sentrySpan = SENTRY_SPAN_PROCESSOR_MAP.get(otelSpanId); @@ -143,7 +126,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor { syntheticError.name = type; } - hub.captureException(syntheticError, { + getCurrentHub().captureException(syntheticError, { captureContext: { contexts: { otel: { @@ -162,6 +145,8 @@ export class SentrySpanProcessor implements OtelSpanProcessor { if (sentrySpan instanceof Transaction) { updateTransactionWithOtelData(sentrySpan, otelSpan); + // @ts-ignore TODO: fix this + sentrySpan.setHub(getCurrentHub()); } else { updateSpanWithOtelData(sentrySpan, otelSpan); } diff --git a/packages/opentelemetry-node/test/spanprocessor.test.ts b/packages/opentelemetry-node/test/spanprocessor.test.ts index 94abd327314c..55c0c8c59bf6 100644 --- a/packages/opentelemetry-node/test/spanprocessor.test.ts +++ b/packages/opentelemetry-node/test/spanprocessor.test.ts @@ -4,7 +4,7 @@ import { Resource } from '@opentelemetry/resources'; import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { SemanticAttributes, SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; -import type { SpanStatusType } from '@sentry/core'; +import { SpanStatusType, Scope } from '@sentry/core'; import { addTracingExtensions, createTransport, Hub, makeMain, Span as SentrySpan, Transaction } from '@sentry/core'; import { NodeClient } from '@sentry/node'; import { resolvedSyncPromise } from '@sentry/utils'; @@ -79,13 +79,9 @@ describe('SentrySpanProcessor', () => { expect(sentrySpanTransaction?.parentSpanId).toEqual(otelSpan.parentSpanId); expect(sentrySpanTransaction?.spanId).toEqual(otelSpan.spanContext().spanId); - expect(hub.getScope()?.getSpan()).toBeUndefined(); - otelSpan.end(endTime); expect(sentrySpanTransaction?.endTimestamp).toBe(endTimestampMs / 1000); - - expect(hub.getScope()?.getSpan()).toBeUndefined(); }); it('creates a child span if there is a running transaction', () => { @@ -789,6 +785,46 @@ describe('SentrySpanProcessor', () => { trace_id: otelSpan.spanContext().traceId, }); }); + + // Regression test for https://github.com/getsentry/sentry-javascript/issues/7538 + // Since otel context does not map to when Sentry hubs are cloned + // we can't rely on the original hub at transaction creation to contain all + // the scope information we want. Let's test to make sure that the information is + // grabbed from the new hub. + it('handles when a different hub creates the transaction', () => { + let sentryTransaction: any; + + client = new NodeClient({ + ...DEFAULT_NODE_CLIENT_OPTIONS, + tracesSampleRate: 1.0, + }); + + client.on('finishTransaction', transaction => { + sentryTransaction = transaction; + }); + + hub = new Hub(client); + makeMain(hub); + + const newHub = new Hub(client, Scope.clone(hub.getScope())); + newHub.configureScope(scope => { + scope.setTag('foo', 'bar'); + }); + + const tracer = provider.getTracer('default'); + + tracer.startActiveSpan('GET /users', parentOtelSpan => { + tracer.startActiveSpan('SELECT * FROM users;', child => { + makeMain(newHub); + child.end(); + }); + + parentOtelSpan.end(); + }); + + // @ts-ignore Accessing private attributes + expect(sentryTransaction._hub.getScope()._tags.foo).toEqual('bar'); + }); }); // OTEL expects a custom date format From 5be44fb9b9b0ca654ed108e0773631d1f65d8dce Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 20:18:23 +0100 Subject: [PATCH 2/3] lint --- .../opentelemetry-node/test/spanprocessor.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-node/test/spanprocessor.test.ts b/packages/opentelemetry-node/test/spanprocessor.test.ts index 55c0c8c59bf6..97256bd867a4 100644 --- a/packages/opentelemetry-node/test/spanprocessor.test.ts +++ b/packages/opentelemetry-node/test/spanprocessor.test.ts @@ -4,8 +4,16 @@ import { Resource } from '@opentelemetry/resources'; import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { SemanticAttributes, SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; -import { SpanStatusType, Scope } from '@sentry/core'; -import { addTracingExtensions, createTransport, Hub, makeMain, Span as SentrySpan, Transaction } from '@sentry/core'; +import type { SpanStatusType } from '@sentry/core'; +import { + addTracingExtensions, + createTransport, + Hub, + makeMain, + Scope, + Span as SentrySpan, + Transaction, +} from '@sentry/core'; import { NodeClient } from '@sentry/node'; import { resolvedSyncPromise } from '@sentry/utils'; From 8c7d58dc031c4952b2c1cb6ebaea2016f804ce9b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 23 Mar 2023 09:37:34 +0100 Subject: [PATCH 3/3] remove ts-ignore todo --- packages/opentelemetry-node/src/spanprocessor.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index ba22acd7e7f6..d6719fd682ad 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -145,7 +145,6 @@ export class SentrySpanProcessor implements OtelSpanProcessor { if (sentrySpan instanceof Transaction) { updateTransactionWithOtelData(sentrySpan, otelSpan); - // @ts-ignore TODO: fix this sentrySpan.setHub(getCurrentHub()); } else { updateSpanWithOtelData(sentrySpan, otelSpan);