Skip to content
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

Svelte Kit fetch requests are repeated again during client hydration #8174

Closed
3 tasks done
janvotava opened this issue May 21, 2023 · 9 comments · Fixed by #8391
Closed
3 tasks done

Svelte Kit fetch requests are repeated again during client hydration #8174

janvotava opened this issue May 21, 2023 · 9 comments · Fixed by #8391
Assignees
Labels
Package: sveltekit Issues related to the Sentry SvelteKit SDK Type: Bug

Comments

@janvotava
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/sveltekit

SDK Version

7.52.1

Framework Version

1.5.0

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

  1. Install default Sentry plugin with an auto instrumentation
  2. Load page - fetch request are repeated although they have been loaded before on the server (SSR)

Expected Result

No request made on the client side.

Actual Result

There is duplicate requests after the client hydration.

@Lms24
Copy link
Member

Lms24 commented May 22, 2023

Hi @janvotava thanks for writing in!

Can you share a little more about what is happening, ideally a minimal reproduction where we can clearly see that the SDK is causing this?

Based on my understanding of SvelteKit's load functions (which I assume is where you're calling fetch), universal loads (+page.ts) are supposed to happen on both, client and server (?). Meaning initially, when the page is SSR'ed, a fetch will occur and then on the client side, why hydration is performed, another one happens.

We're patching SvelteKit's fetch function on the client-side to add tracing headers for distributed tracing between client and server but we're not actively making requests or interfering with hydration. At least knowingly 😅

@Lms24 Lms24 added Status: Needs Reproduction Package: sveltekit Issues related to the Sentry SvelteKit SDK and removed Status: Untriaged labels May 22, 2023
@janvotava
Copy link
Author

janvotava commented May 22, 2023

Hello @Lms24, thanks for a reply!

I have a sample app that fetches a endpoint with a random number.

On the client side with loads patched by Sentry
image

And with autoInstrument: false
image

Basically it skips reusing that <script type="application/json" data-sveltekit-fetched data-url="/api/random-number">{"status":200,"statusText":"","headers":{},"body":"0.07213929057627122"}</script> at the bottom of the page. Probably because of some Svelte Kit internal logic. Also can give you a sample PoC app (just tell me what to put into DSN).

Thanks a lot.

@Lms24
Copy link
Member

Lms24 commented May 22, 2023

Thanks for the additional information and clarification. Just to confirm, this request is made from a universal load function, right? I still need to reproduce it formally but I think, I already know what's going on and you're right with the script tags:

  • during SSR, fetch is executed and the result is stored in a data-sveltekit-fetched script tag
  • On the client, during hydration, SvelteKit's fetch functions try to obtain the data from the "cache" that's populated from the script tag(s) by matching the request.
    • For the request in question, there's no cache hit because we do in fact modify the fetch request by adding our sentry-trace and baggage headers to it. Headers influence the cache key and hence there's a cache miss :(
    • As a result, the data is actually fetched

So the question now is, how can we solve this? I see two potential solutions:

  • Ideally, we wouldn't need to patch SvelteKit's fetch implementation at all. Instead, SvelteKit would expose a handleFetch hook on the client, just like it does on the server. This should give us a safe way of adding headers to the request. Although, caching can be a challenge in this scenario as well, depending on the order of the hook vs. the cache lookup. We already proposed this a while ago but haven't gotten much feedback form the Kit maintainers at this time.
  • Less preferable but probably necessary until we have a better handleFetch hook: We do the cache lookup ourselves in the SDK client code and only add headers if there's a cache hit. It's probably not too hard to do but it's brittle, given that the cache selector creation could change at any time in Kit.

I'll need to think about this a bit more but feel free to suggest other options if you have an idea ;)

@Lms24 Lms24 self-assigned this May 22, 2023
@Lms24
Copy link
Member

Lms24 commented May 22, 2023

I created a reproduction: https://github.com/Lms24/gh-js-8174

This shows the erroneous behavour by default and how disabeling autoInstrumentation solves it. I took some time with this repro so that ideally, SvelteKit maintainers can also look at it.

@Lms24
Copy link
Member

Lms24 commented May 22, 2023

One thing to add to this issue:

While it is clearly not intended that we cause this cache miss and we should do something about it (we will), the current behaviour of the SDK does not break applications. The reason is that universal load functions need to be built in a way that they can run on both, client and server.

@Lms24
Copy link
Member

Lms24 commented May 23, 2023

I opened sveltejs/kit#10009 which would solve this in a clean way. I want to give the Svelte folks a few days to look at this. Maybe we can get it in in which case I'd be more than happy to remove our custom kit fetch instrumentation. Otherwise, we'll have to resort to the brittle cache detection logic in the SDK.

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@Lms24
Copy link
Member

Lms24 commented Jun 19, 2023

It seems like Svelte folks aren't too keen on merging my fix so we'll probably need to handle this SDK-side. Will take this on in the near to mid future.

Lms24 added a commit that referenced this issue Jun 26, 2023
…mentation (#8391)

As outlined in
#8174 (comment),
our current SvelteKit fetch instrumentation breaks SvelteKit's request
caching mechanism. This is problematic as `fetch` requests from
universal `load` functions were made again on the client side during
hydration although the response was already cached from the initial
server-side request.

The reason for the cache miss is that in the instrumentation we add our
tracing headers to the requests, which lead to a different cache key
than the one produced on the server side.

This fix vendors in code from [the SvelteKit
repo](https://github.com/sveltejs/kit) so that we can perform the same
cache lookup in our instrumentation. If the lookup was successful (-->
cache hit), we won't attach any headers or create breadcrumbs to 1. let
Kit's fetch return the cached response and 2. not add spans for a fetch
request that didn't even happen.
@ngalaiko
Copy link

hey @Lms24. I've noticed a similar issue with the whole load() function, not just with fetch.

client-side load runs on every navigation when instrumentation is enabled or load function is manually wrapped.

this changed sveltekit's default behaviour in a similar way. for example, we instantiate cache objects inside the load function, and sentry's wrapper effectively breaks caches.

let me know if you want me to create a separate ticket for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: sveltekit Issues related to the Sentry SvelteKit SDK Type: Bug
Projects
None yet
3 participants