Skip to content

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 28, 2025

This PR adds support for setting up the SDK with SvelteKit's new instrumentation.server.ts instrumentation file, as well as by leveraging the new spans emitted from kit. This requires a SvelteKit version of at least 2.31.0. Of course we want to stay backwards compatible, so this PR adds the following flow:

We check for version from package.json (I'm, this is flawed and could mean we identify a version <2.31.0 when in fact it is already used. But then again, if not explicitly set in package.json, we can't rely on >= 2.31.0 being used). If we identify <2.31.0, we recommend updating to the newest version.

From this point, there are 3 options for the user:

  1. update kit: exits the wizard and lets the user update kit. I don't think we should do this for them.
  2. users are already using >=2.31.0 (i.e. false negative from our end): Continue with new instrumentation file setup
  3. user will upgrade later: Continue with old, hooks-based setup.

If we detect a version >=2.31.0, we don't ask anything but directly continue with the new setup.

For the old setup, nothing changes.

For the new setup, we now apply the following changes:

  • enable instrumentation and tracing in svelte.config.js
  • add Sentry.init into instrumentation.server.mjs
  • add handle and error hooks into hooks.server.ts
  • add Sentry.init and error hook into hooks.client.ts

(So to clear up any confusion: We still make use of hooks files but the server-side SDK init got extracted to the instrumentaiton file)

Additionally, added some tests and a new e2e test.

closes #1075

Copy link

github-actions bot commented Aug 28, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 35bc982

Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 37.01997% with 410 lines in your changes missing coverage. Please review.
✅ Project coverage is 32.06%. Comparing base (a461c2c) to head (35bc982).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/sveltekit/sdk-setup/setup.ts 4.37% 153 Missing ⚠️
src/sveltekit/sdk-setup/svelte-config.ts 65.65% 102 Missing ⚠️
src/sveltekit/sdk-setup/vite.ts 12.38% 92 Missing ⚠️
src/sveltekit/sveltekit-wizard.ts 4.34% 44 Missing ⚠️
src/sveltekit/sdk-setup/utils.ts 13.33% 13 Missing ⚠️
src/sveltekit/utils.ts 0.00% 5 Missing ⚠️
src/sveltekit/sdk-example.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
+ Coverage   31.53%   32.06%   +0.52%     
==========================================
  Files         129      133       +4     
  Lines       15169    15638     +469     
  Branches     1032     1093      +61     
==========================================
+ Hits         4784     5014     +230     
- Misses      10368    10607     +239     
  Partials       17       17              
Flag Coverage Δ
unit-tests 32.06% <37.01%> (+0.52%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Lms24 Lms24 self-assigned this Aug 28, 2025
@Lms24
Copy link
Member Author

Lms24 commented Aug 28, 2025

Hmm I really dislike that our e2e tests don't contribute to our coverage calculation. Maybe we can find some way to run them within the same process as the test. Might be a nice addition for clifty. Though not sure how this would work yet.

vitest and child process coverage doesn't seem to go anywhere (vitest-dev/vitest#2911)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to rename the sveltekit test app to sveltekit-hooks to avoid ambiguity with sveltekit-tracing.

@Lms24 Lms24 marked this pull request as ready for review August 28, 2025 20:35
@Lms24 Lms24 requested review from andreiborza and chargome August 28, 2025 20:35
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to split this file up since there's quite a lot going on in it. Hence the various new files in the sdk-setup dir

Copy link
Member

@andreiborza andreiborza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice!

Lms24 and others added 3 commits September 1, 2025 16:21
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Copy link
Member

@andreiborza andreiborza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@Lms24 Lms24 merged commit cf911af into master Sep 1, 2025
42 checks passed
@Lms24 Lms24 deleted the lms/feat-sveltekit-kit-tracing branch September 1, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Support for SvelteKit Tracing Setup

2 participants