-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ref(sveltekit): Remove custom client fetch instrumentation and use default instrumentation #8802
Merged
Lms24
merged 4 commits into
develop
from
lms/ref-sveltekit-use-browsertracing-fetch-instrumentation
Aug 17, 2023
Merged
ref(sveltekit): Remove custom client fetch instrumentation and use default instrumentation #8802
Lms24
merged 4 commits into
develop
from
lms/ref-sveltekit-use-browsertracing-fetch-instrumentation
Aug 17, 2023
Conversation
This file contains 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
Lms24
changed the title
ref(sveltekit): Remove custom client-fetch instrumentation and use default instrumentation
ref(sveltekit): Remove custom client fetch instrumentation and use default instrumentation
Aug 11, 2023
size-limit report 📦
|
3 tasks
AbhiPrasad
approved these changes
Aug 16, 2023
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.
Very clever approach!
lforst
approved these changes
Aug 17, 2023
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.
Lms24
deleted the
lms/ref-sveltekit-use-browsertracing-fetch-instrumentation
branch
August 17, 2023 08:20
But what to do with CSP now? |
3 tasks
Tracking in #8925 |
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.
This PR removes our custom SvelteKit client fetch instrumentation which we created when we initially worked on the SDK. Back then I didn't think that it's possible to use our default fetch instrumentation from
BrowserTracing
, due to timing issues where Kit would store awaywindow.fetch
(and use the stored version inload
functions) before our SDK was initialized.After receiving some hints how it might be possible, we now have a way to instrument
fetch
everywhere on the client (including the one inload
) functions.This works in two parts:
handle
wrapper injects a script into the returned HTML that wrapswindow.fetch
and adds a proxy handler (window._sentryFetchProxy
) that at this time just forwards the fetch call to the original fetch.After this script is evaluated by the browser, at some point, SvelteKit loads its initial client-side bundle that stores away
window.fetch
. Kit also patcheswindow.fetch
itself at this time.Sometime later, the code from the
hooks.client.js
file is evaluated in the browser, including ourSentry.init
call:Sentry.init
we now swapwindow.fetch
withwindow._sentryFetchProxy
which will make ourBrowserTracing
integration patch our proxy with our default fetch instrumentation. After the init, we swap the two fetches back and we're done.While this seems a little hacky and confusing at first, I believe this is actually quite a nice way of making things work and it comes with a number of advantages:
BrowserTracing
fetch instrumentation just as SDK users would expect and no longer need to duplicate logic just for SvelteKit(I worked on this while investigating #8610 as I initially believed that the issue was caused by a cache lookup bug in our instrumentation. That didn't turn out to be the cause but now we fixed two things 🎉 )
h/t @myieye for sharing their way of patching fetch