ref(opentelemetry): Remove startSpan overrides from async context strategy - #23198
Open
andreiborza wants to merge 6 commits into
Open
ref(opentelemetry): Remove startSpan overrides from async context strategy#23198andreiborza wants to merge 6 commits into
andreiborza wants to merge 6 commits into
Conversation
…ategy Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
size-limit report 📦
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e11c4b7. Configure here.
mydea
reviewed
Aug 10, 2026
| return wrapper(() => { | ||
| const scope = getCurrentScope(); | ||
| const parentSpan = getParentSpan(scope, customParentSpan); | ||
| const parentSpan = getParentSpan(customParentSpan, customScope ? scope : undefined); |
Member
There was a problem hiding this comment.
can you explain (in a comment maybe?) why we pass in scope if there is a customScope?
Member
Author
There was a problem hiding this comment.
Actually not needed, this is a bit of an indirection since scope is already the customScope. I simplified this in 920a03b
andreiborza
marked this pull request as ready for review
August 10, 2026 13:50
andreiborza
requested review from
JPeer264,
Lms24,
chargome,
mydea and
nicohrubec
and removed request for
a team
August 10, 2026 13:50
mydea
reviewed
Aug 10, 2026
| }): Span { | ||
| const isolationScope = getIsolationScope(); | ||
|
|
||
| // A remote parent (an incoming trace on the ambient OTel context, set by the propagator) cannot |
Member
There was a problem hiding this comment.
I think some word(s) are missing here? 😅
mydea
reviewed
Aug 10, 2026
Comment on lines
+506
to
+513
| const acs = getAcs(); | ||
| if (acs.withActiveSpan) { | ||
| return acs.withActiveSpan(span, callback); | ||
| } | ||
|
|
||
| _setSpanForScope(scope, span); | ||
| return callback(); | ||
| } |
Member
There was a problem hiding this comment.
I think this can just be:
return withScope(scope, () => {
return withActiveSpan(span, () => {
return callback();
})
});without using any specific implementations etc?
Member
Author
There was a problem hiding this comment.
Hm, this doesn't quite work on the browser side because there withActiveSpan also runs withScope and we end up double-forking which messes things up.
Any idea how to get around that?
mydea
reviewed
Aug 10, 2026
| * created from it picks up the incoming trace id, parent span id, and sampling decision. Mirrors | ||
| * `SentryTracer._startRootSpanWithRemoteParent` in `@sentry/opentelemetry`. | ||
| */ | ||
| function scopeWithRemoteParentPropagation(scope: Scope, remoteParentSpan: SentrySpan): Scope { |
Member
There was a problem hiding this comment.
puh, any way we can get rid of this? how/when does this fail if we do not have this special handling in this depth? 🤔
mydea
reviewed
Aug 10, 2026
| } | ||
|
|
||
| const span = _getSpanForScope(scope) as SentrySpan | undefined; | ||
| const span = (explicitScope ? _getSpanForScope(explicitScope) : getActiveSpan()) as SentrySpan | undefined; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
There is now only one implementation of
startSpan,startSpanManualandstartInactiveSpan: the one in core. The separate OpenTelemetry copies and the ACS overrides for them are removed._INTERNAL_startInactiveSpan, theSentryTracernow uses the publicstartInactiveSpanWhy
The OpenTelemetry copies created spans through core anyway and only added context bookkeeping around it. One shared code path removes the double indirection and a lot of duplicated code.