Fix route handling when local root span wasn't created by instrumentation api #13588
Conversation
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
ideally this seems better to me if it works, so that we can preserve the definition of local root |
laurit
changed the title
trask
approved these changes
Mar 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #13583
Currently instrumentation api sets local root only when parent span is either invalid or remote. If the parent span is not created by instrumentation api then the local root span will remain unset and the server span won't be set in http route state. Ideally if the server span is created outside of the instrumentation api we'd like to suppress our server span, but one of the side-effects of having spans that are not created with instrumentation api is that span suppression won't work since we rely on context keys set by instrumentation api to determine whether there already is a server span.
I'm not completely convinced that this is the right thing to do but route not being updated when there is a non instrumentation api parent is unexpected. Another option we could try is that instead of setting the local root span we could just update the server span in the http route state and leave the local root unset. @trask WDYT?This PR fixes updating the route state when the root span wasn't created by instrumentation api.