-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(opentelemetry): Ensure Sentry spans don't leak when tracing is disabled #18337
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
Changes from 4 commits
1ced37c
dfaa148
46beb59
b2a15f4
c2baa79
2d4bccc
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 |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import type { Context, Span, SpanContext, SpanOptions, Tracer } from '@opentelemetry/api'; | ||
| import { context, SpanStatusCode, trace, TraceFlags } from '@opentelemetry/api'; | ||
| import { suppressTracing } from '@opentelemetry/core'; | ||
| import { isTracingSuppressed, suppressTracing } from '@opentelemetry/core'; | ||
| import type { | ||
| Client, | ||
| continueTrace as baseContinueTrace, | ||
|
|
@@ -17,6 +17,7 @@ import { | |
| getRootSpan, | ||
| getTraceContextFromScope, | ||
| handleCallbackErrors, | ||
| hasSpansEnabled, | ||
| SDK_VERSION, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
| spanToJSON, | ||
|
|
@@ -53,6 +54,37 @@ export function startSpan<T>(options: OpenTelemetrySpanContext, callback: (span: | |
|
|
||
| const spanOptions = getSpanOptions(options); | ||
|
|
||
| // If spans are not enabled, ensure we suppress tracing for the span creation | ||
| // but preserve the original context for the callback execution | ||
| // This ensures that we don't create spans when tracing is disabled which | ||
| // would otherwise be a problem for users that don't enable tracing but use | ||
| // custom OpenTelemetry setups. | ||
| if (!hasSpansEnabled()) { | ||
| const suppressedCtx = isTracingSuppressed(ctx) ? ctx : suppressTracing(ctx); | ||
|
|
||
| return context.with(suppressedCtx, () => { | ||
| return tracer.startActiveSpan(name, spanOptions, suppressedCtx, span => { | ||
| // Restore the original unsuppressed context for the callback execution | ||
| // so that custom OpenTelemetry spans maintain the correct context. | ||
| // We use activeCtx (not ctx) because ctx may be suppressed when onlyIfParent is true | ||
| // and no parent span exists. Using activeCtx ensures custom OTel spans are never | ||
| // inadvertently suppressed. | ||
| return context.with(activeCtx, () => { | ||
| return handleCallbackErrors( | ||
| () => callback(span), | ||
| () => { | ||
| // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses | ||
| if (spanToJSON(span).status === undefined) { | ||
| span.setStatus({ code: SpanStatusCode.ERROR }); | ||
| } | ||
| }, | ||
| () => span.end(), | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return tracer.startActiveSpan(name, spanOptions, ctx, span => { | ||
| return handleCallbackErrors( | ||
| () => callback(span), | ||
|
|
@@ -96,6 +128,36 @@ export function startSpanManual<T>( | |
|
|
||
| const spanOptions = getSpanOptions(options); | ||
|
|
||
| if (!hasSpansEnabled()) { | ||
| // If spans are not enabled, ensure we suppress tracing for the span creation | ||
| // but preserve the original context for the callback execution | ||
| // This ensures that we don't create spans when tracing is disabled which | ||
| // would otherwise be a problem for users that don't enable tracing but use | ||
| // custom OpenTelemetry setups. | ||
| const suppressedCtx = isTracingSuppressed(ctx) ? ctx : suppressTracing(ctx); | ||
|
|
||
| return context.with(suppressedCtx, () => { | ||
| return tracer.startActiveSpan(name, spanOptions, suppressedCtx, span => { | ||
| // Restore the original unsuppressed context for the callback execution | ||
| // so that custom OpenTelemetry spans maintain the correct context. | ||
| // We use activeCtx (not ctx) because ctx may be suppressed when onlyIfParent is true | ||
| // and no parent span exists. Using activeCtx ensures custom OTel spans are never | ||
| // inadvertently suppressed. | ||
| return context.with(activeCtx, () => { | ||
| return handleCallbackErrors( | ||
| () => callback(span, () => span.end()), | ||
| () => { | ||
| // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses | ||
| if (spanToJSON(span).status === undefined) { | ||
| span.setStatus({ code: SpanStatusCode.ERROR }); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return tracer.startActiveSpan(name, spanOptions, ctx, span => { | ||
| return handleCallbackErrors( | ||
| () => callback(span, () => span.end()), | ||
|
|
@@ -130,13 +192,15 @@ export function startInactiveSpan(options: OpenTelemetrySpanContext): Span { | |
| return wrapper(() => { | ||
| const activeCtx = getContext(options.scope, options.forceTransaction); | ||
| const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); | ||
| const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; | ||
| let ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; | ||
|
|
||
| const spanOptions = getSpanOptions(options); | ||
|
|
||
| const span = tracer.startSpan(name, spanOptions, ctx); | ||
| if (!hasSpansEnabled()) { | ||
|
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. l: what about following to have the same return (so we don't have to touch two parts of the code in case the return changes (or a nested ternary): let context = ctx;
if (!hasSpansEnabled()) {
context = isTracingSuppressed(ctx) ? ctx : suppressTracing(ctx);
}
return tracer.startSpan(name, spanOptions, context);
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. Updated. |
||
| ctx = isTracingSuppressed(ctx) ? ctx : suppressTracing(ctx); | ||
| } | ||
|
|
||
| return span; | ||
| return tracer.startSpan(name, spanOptions, ctx); | ||
| }); | ||
| } | ||
|
|
||
|
|
||
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.
h: This here looks like almost the same code as above - even the comments. Can this be moved into a reusable method?
The only different what I've seen is that the one above has
() => span.end()in thehandleCallbackErrors, not sure if this was intended.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.
I opted not to de-duplicate here. In general
startSpanandstartSpanManualare basically identical other than how they end spans (auto-end vs manual-end).Splitting this out into a common helper makes it harder to understand because of how the callback and the success callback are handled differently. I think it's a complicated abstraction for little gain.
Tbh I would rather not deduplicate here, the code is already easy to get lost in without having an extra abstraction in it.
What do you think?
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.
I ended up updating this, I guess we'll hardly ever see that much diversion between the two apis, so it probably doesn't hurt to keep this one dry.