fix(sveltekit): Ensure sub requests are recorded as child spans of parent request #11130
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
size-limit report 📦
|
Lms24
force-pushed
the
lms/ref-sveltekit-isSubRequest
branch
2 times, most recently
from
a899d27 to
53b5dc0
Compare
Lms24
changed the title
isSubRequest flag to decide on server root span creation
Lms24
requested review from
a team,
stephanie-anderson,
s1gr1d and
AbhiPrasad
and removed request for
a team
lforst
approved these changes
Mar 22, 2024
| }, | ||
| }); | ||
|
|
||
| expect(spans).toHaveLength(2); |
There was a problem hiding this comment.
l: I'd argue that this assertion is redundant since we already have the one checking for the two relevant spans below. Just pointing out because it may create unnecessary test failures in case there is ever gonna be additional spans because of instrumentation changes. Same for the sveltekit 2 test!
| // behavior. | ||
| // As a fallback for Kit < 1.21.0, we check if there is an active span only if there's none, | ||
| // we create a new execution context. | ||
| const isSubRequest = typeof input.event.isSubRequest === 'boolean' ? input.event.isSubRequest : !!getActiveSpan(); |
There was a problem hiding this comment.
yo this is surprisingly convenient 😂
There was a problem hiding this comment.
Yeah I was really happy when I found sveltejs/kit#10170 😅
| // We want the `http.server` span of that nested call to be a child span of the | ||
| // currently active span instead of a new root span to correctly reflect this | ||
| // behavior. | ||
| // As a fallback for Kit < 1.21.0, we check if there is an active span only if there's none, |
There was a problem hiding this comment.
I am worried about sveltekit 1 here because shouldn't there always be a span with the OTEL http instrumentation? Or was it that the http integration doesn't work in sveltekit?
There was a problem hiding this comment.
The http integration doesn't seem to do anything in Kit :(
But you're raising a good point. I'll check what getActiveSpan now returns going forward. i.e. if it returns something like a NonRecordingSpan if there previously was no span. In that case, we need to rework the check a bit.
There was a problem hiding this comment.
I checked for Kit < 1.21.0 and we still return undefined in this case. So the logic works correctly here. To make sure we cover this correctly in the future, I downgraded to kit 1.20.5 in our Kit 1.x E2E test version.
Lms24
force-pushed
the
lms/ref-sveltekit-isSubRequest
branch
from
f4174fd to
8d38097
Compare
c298lee
pushed a commit
that referenced
this pull request
Mar 22, 2024
…rent request (#11130) When switching the SvelteKit server side SDK to `@sentry/node` powered by Otel, the semantics behind `continueTrace` changed as outlined in #11199. TLDR: We previously called `continueTrace` in a nested way when dealing with sub-requests`*` in SvelteKit. In our old Node SDK, this did nothing; in the new SDK, this currently causes a new root span/transaction to be created for the sub request. This patch now ensures that we continue to send sub request spans as child spans of the top-level/parent request.
c298lee
pushed a commit
that referenced
this pull request
Mar 23, 2024
…rent request (#11130) When switching the SvelteKit server side SDK to `@sentry/node` powered by Otel, the semantics behind `continueTrace` changed as outlined in #11199. TLDR: We previously called `continueTrace` in a nested way when dealing with sub-requests`*` in SvelteKit. In our old Node SDK, this did nothing; in the new SDK, this currently causes a new root span/transaction to be created for the sub request. This patch now ensures that we continue to send sub request spans as child spans of the top-level/parent request.
cadesalaberry
pushed a commit
to cadesalaberry/sentry-javascript
that referenced
this pull request
Apr 19, 2024
…rent request (getsentry#11130) When switching the SvelteKit server side SDK to `@sentry/node` powered by Otel, the semantics behind `continueTrace` changed as outlined in getsentry#11199. TLDR: We previously called `continueTrace` in a nested way when dealing with sub-requests`*` in SvelteKit. In our old Node SDK, this did nothing; in the new SDK, this currently causes a new root span/transaction to be created for the sub request. This patch now ensures that we continue to send sub request spans as child spans of the top-level/parent request.
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.
When switching the SvelteKit server side SDK to
@sentry/nodepowered by Otel, the semantics behindcontinueTracechanged as outlined in #11199. TLDR: We previously calledcontinueTracein a nested way when dealing with sub-requests*in SvelteKit. In our old Node SDK, this did nothing; in the new SDK, this currently causes a new root span/transaction to be created for the sub request.This PR now ensures that we continue to send sub request spans as child spans of the top-level/parent request by making the following changes:
continueTracecall in ourloadinstrumentation. Server-side, load spans will never be root spans because they're always wrapped byhttp.serverspans from thesentryHandlehook.continueTraceif we're not in a sub requestisSubRequestflag to our sub request check. This flag is available since kit 1.21.0 and we should leverage it whenever it's available. Otherwise, we fall back to checking for an active span as previously.*What is a sub request?In SvelteKit, you can make
fetchcalls within a dataloadfunction that runs on the server (e.g. during pageload or a navigation). If such aloadfunction makes afetchcall to a same-origin server (API) route, SveteKit detects that it's not necessary to make another http request but basically redirects the fetch call to directly re-enter the server request handler. This new request is called a sub request.Example (from this Sentry event)
closes #11125
ref #11041