feat(opentelemetry): Add SentryTracerProvider#21666
Conversation
andreiborza
requested review from
JPeer264 and
mydea
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
size-limit report 📦
|
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
2 times, most recently
from
20bf510 to
7f2f88d
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
from
7f2f88d to
172dd9f
Compare
| const { description } = spanHasName(rootSpan) ? parseSpanDescription(rootSpan) : { description: undefined }; | ||
| if (source !== 'url' && description) { | ||
| dsc.transaction = description; | ||
| if (jsonSpan.description) { |
There was a problem hiding this comment.
can you explain this, why do we guard this based on jsonSpan.description?
There was a problem hiding this comment.
The previous guard spanHasName looks at Otel ReadableSpan's name field which we don't have in the new tracer provider path.
The guard is so that we don't end up with <unknown> from parseSpanDescription in the case that no description was set.
| inferred.source !== undefined && | ||
| inferred.source !== 'custom' && | ||
| (options.finalizeStatus || inferred.source !== 'url') && | ||
| (spanJSON.parent_span_id === undefined || kind === SpanKind.SERVER); |
There was a problem hiding this comment.
the comment is pretty good but does not really explain this part of the condition?
| inferred.description !== spanJSON.description && | ||
| (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] !== 'custom' || (mayInferSource && !hasCustomSpanName)) | ||
| ) { | ||
| addNonEnumerableProperty(span as Span & { _name?: string }, '_name', inferred.description); |
There was a problem hiding this comment.
uhh, this looks dangerous? What does that do? is the idea to override the protected _name field on the sentry span? If so, this may break I think when stuff is minimized, as I doubt this would catch this being the same field?
There was a problem hiding this comment.
This is a left-over from when we wanted to avoid calling updateName as it sets the source to 'custom'. But we now landed exempts from tracer started spans from that so we no longer need this workaround. I updated to a normal invocation of updateName in 2c670f3
| const capturedIsolationScope = getCapturedScopesOnSpan(span as unknown as Span).isolationScope; | ||
| if (capturedIsolationScope) { | ||
| ctxWithSpan = ctxWithSpan.setValue(SENTRY_FORK_SET_ISOLATION_SCOPE_CONTEXT_KEY, capturedIsolationScope); | ||
| } |
There was a problem hiding this comment.
is this really necessary? I thought this should just be inherited anyhow from child contexts...?
There was a problem hiding this comment.
No not necessary. There's on possibility where the a passed context to startActiveSpan could come with a differently bound scope but I can't come up with any examples of where that might happen.
Removed in 4ef5072
There was a problem hiding this comment.
I had to reinstate this as nextjs e2e middleware tests broke on #21680.
Previously, in the Otel SDK path, we handled the isolation scope capturing via SentrySpanProcessor.onStart, but since we no longer have a span processor, we need to replicate that behavior here. So we pin the captured scope onto the context, otherwise work done inside the span (e.g. tags and breadcrumbs) would land on a different scope.
| // @ts-expect-error We just assume that this is non-abstract, if you pass in an abstract class this would make it non-abstract | ||
| class OpenTelemetryClient extends ClientClass implements OpenTelemetryClientInterface { | ||
| public traceProvider: BasicTracerProvider | undefined; | ||
| public traceProvider: OpenTelemetryTraceProvider | undefined; |
There was a problem hiding this comment.
m: do we need to update this in node-core and vercel-edge as well?
There was a problem hiding this comment.
The plan was to only setup this tracer provider in the node sdk, others will then switch to either no tracer provider at all or the sentry tracer provider in v11.
There was a problem hiding this comment.
right so for node-core it's a noop since it won't exist anymore. are we tracking somewhere that we need to adjust the other SDKs?
| const attributes = jsonSpan.data; | ||
| const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]; | ||
|
|
||
| const { description } = spanHasName(rootSpan) ? parseSpanDescription(rootSpan) : { description: undefined }; |
There was a problem hiding this comment.
q: do we need to audit these at some point? as in check if we need these anymore
There was a problem hiding this comment.
Yeah, with v11 we should be able to drop these.
There was a problem hiding this comment.
do we have issues for these cleanups so we don't forget?
| * A minimal OpenTelemetry TracerProvider which creates native Sentry spans. | ||
| */ | ||
| export class SentryTracerProvider implements TracerProvider { | ||
| public readonly resource?: { attributes: SpanAttributes }; |
There was a problem hiding this comment.
l: is this used anywhere? if not maybe we should remove it
There was a problem hiding this comment.
Technically no, we don't apply resources on spans like in the otel path so this could go, but I'd prefer to remove this in conjunction with the vendored sentry resource in v11.
There was a problem hiding this comment.
do we have issues for these cleanups so we don't forget?
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
2 times, most recently
from
afb77ef to
fcdf2df
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
2 times, most recently
from
2c670f3 to
308e560
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
from
09f43b1 to
cc82764
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
2 times, most recently
from
400be89 to
0cb493b
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
from
4c1244d to
a1613c2
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
2 times, most recently
from
6bd79ac to
e005612
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
2 times, most recently
from
924ce11 to
630bb67
Compare
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
from
630bb67 to
15154ed
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 15154ed. Configure here.
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
3 times, most recently
from
97e8266 to
cfbfb8b
Compare
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
LGTM. I didn't go over every edge cases I guess, but I tried it in Cloudflare SDK and replaced it with the current mocked trace provider. And it works like a charm
| */ | ||
| export function backfillStreamedSpanDataFromOtel(spanJSON: StreamedSpanJSON, hint?: { spanKind?: number }): void { | ||
| const attributes = spanJSON.attributes; | ||
| if (!attributes) { |
There was a problem hiding this comment.
l/m: (I know it moved 1:1, but just saw this now) Should this guard against undefined? If this is undefined then it wouldn't set otel.kind nor the 'custom' source. Just want to clarify if that is intended. And if it is intended what would be the difference between attributes being undefined vs. an empty object {}
There was a problem hiding this comment.
In both paths (SentryTracerProvider with SentrySpans and OTel's BasicTracerProvider with ReadableSpans) this basically never hits.
SentrySpan's attributes init to {} and ReadableSpan's attributes are always an object so this is basically always at least {}.
Nonetheless, I updated it because "this shouldn't happen" are famous last words and it costs us nothing to fallback to {}.
Updated in 98255e6
Add a minimal OpenTelemetry `TracerProvider` that creates native Sentry spans instead of bridging through the full OTel SDK.
A root span with no parent and no remote (incoming) parent previously continued the scope's propagation context, so manually-started parallel root spans in the same scope all collapsed into a single shared trace. The OpenTelemetry SDK instead mints a fresh trace id per such root span. Wrap the no-parent branch of `_startSentrySpan` in `startNewTrace` (matching the existing `options.root` branch) so each parentless root span gets its own trace. Incoming traces are unaffected, since `continueTrace` sets a remote parent and takes the `_startRootSpanWithRemoteParent` branch instead.
…race When `SentryTracer` continues a remote trace whose incoming headers carried no baggage, `_startRootSpanWithRemoteParent` froze a derived-but-incomplete dynamic sampling context (missing `sample_rand` and `transaction`) onto the span, which then propagated downstream. Only freeze the DSC when the remote parent actually carried one (its trace state has the `sentry.dsc` key); otherwise leave it unset so it is derived dynamically from the span, matching the OpenTelemetry SDK path, which never freezes the DSC there and resolves it lazily (picking up `transaction` and `sample_rand`).
andreiborza
force-pushed
the
ab/sentry-trace-provider-otel
branch
from
cfbfb8b to
1c93e34
Compare
4 participants
Add a minimal OpenTelemetry
TracerProviderthat creates native Sentry spans instead of bridging through the full OTel SDK.Hooking up the tracer provider and e2e tests are in #21680