Skip to content
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

fix(otel): Make sure we use correct hub on finish #7577

Merged
merged 3 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
}
23 changes: 4 additions & 19 deletions packages/opentelemetry-node/src/spanprocessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove all of this because a hub should always be defined (and scope is actually never used anywhere).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! 🔥

return;
}

const otelSpanId = otelSpan.spanContext().spanId;
const otelParentSpanId = otelSpan.parentSpanId;

Expand All @@ -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',
Expand All @@ -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);

Expand Down Expand Up @@ -143,7 +126,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor {
syntheticError.name = type;
}

hub.captureException(syntheticError, {
getCurrentHub().captureException(syntheticError, {
captureContext: {
contexts: {
otel: {
Expand All @@ -162,6 +145,8 @@ export class SentrySpanProcessor implements OtelSpanProcessor {

if (sentrySpan instanceof Transaction) {
updateTransactionWithOtelData(sentrySpan, otelSpan);
// @ts-ignore TODO: fix this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this still be here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, will remove!

sentrySpan.setHub(getCurrentHub());
} else {
updateSpanWithOtelData(sentrySpan, otelSpan);
}
Expand Down
54 changes: 49 additions & 5 deletions packages/opentelemetry-node/test/spanprocessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@ 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 { addTracingExtensions, createTransport, Hub, makeMain, Span as SentrySpan, Transaction } 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';

Expand Down Expand Up @@ -79,13 +87,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', () => {
Expand Down Expand Up @@ -789,6 +793,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
Expand Down