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

fix(sveltekit): Ensure trace meta tags are always injected #13231

Merged
merged 10 commits into from
Oct 8, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 5, 2024

Based on #13201

Removes the incorrect root span check guarding the trace <meta> tag injection. With this fix, trace data is now also injected for Tracing Without Performance scenarios where there is no active span.

closes #13106

@Lms24 Lms24 changed the base branch from develop to lms/feat-core-metaTags August 5, 2024 12:43
@Lms24 Lms24 self-assigned this Aug 5, 2024
@Lms24 Lms24 linked an issue Aug 5, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Aug 5, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.73 KB - -
@sentry/browser - with treeshaking flags 21.53 KB - -
@sentry/browser (incl. Tracing) 34.97 KB - -
@sentry/browser (incl. Tracing, Replay) 71.54 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.94 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 75.87 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 88.64 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.5 KB - -
@sentry/browser (incl. metrics) 27 KB - -
@sentry/browser (incl. Feedback) 39.87 KB - -
@sentry/browser (incl. sendFeedback) 27.38 KB - -
@sentry/browser (incl. FeedbackAsync) 32.17 KB - -
@sentry/react 25.49 KB - -
@sentry/react (incl. Tracing) 37.94 KB - -
@sentry/vue 26.9 KB - -
@sentry/vue (incl. Tracing) 36.86 KB - -
@sentry/svelte 22.87 KB - -
CDN Bundle 24.05 KB - -
CDN Bundle (incl. Tracing) 36.76 KB - -
CDN Bundle (incl. Tracing, Replay) 71.29 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 76.6 KB - -
CDN Bundle - uncompressed 70.53 KB - -
CDN Bundle (incl. Tracing) - uncompressed 109.04 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 221.13 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 234.34 KB - -
@sentry/nextjs (client) 37.91 KB - -
@sentry/sveltekit (client) 35.56 KB - -
@sentry/node 124.5 KB - -
@sentry/node - without tracing 93.63 KB - -
@sentry/aws-serverless 103.3 KB - -

View base workflow run

Base automatically changed from lms/feat-core-metaTags to develop August 6, 2024 12:38
@Lms24 Lms24 force-pushed the lms/fix-sveltekit-meta-tags-twp branch from 1b0510e to 1e3e0e7 Compare August 6, 2024 12:42
@Lms24 Lms24 marked this pull request as ready for review August 6, 2024 12:42
@Lms24 Lms24 requested review from mydea and s1gr1d August 6, 2024 12:43
@Lms24
Copy link
Member Author

Lms24 commented Aug 7, 2024

I'm gonna hold off from merging this for a bit because I discovered that we have a bug in the sentry-trace meta tag where we propagate a -0 sampling decision when it should actually be deferred (i.e. not set). Gonna add some e2e tests for TwP

@Lms24 Lms24 force-pushed the lms/fix-sveltekit-meta-tags-twp branch from 1e3e0e7 to 4a23c22 Compare August 7, 2024 12:41
@Lms24 Lms24 force-pushed the lms/fix-sveltekit-meta-tags-twp branch from 4a23c22 to fea1cb7 Compare August 9, 2024 08:28
s1gr1d added a commit that referenced this pull request Aug 12, 2024
Making sure tracing without performance works.

ref (SvelteKit):
#13231
@Lms24 Lms24 force-pushed the lms/fix-sveltekit-meta-tags-twp branch from 2526b01 to b0d22ad Compare August 14, 2024 11:51
@Lms24 Lms24 force-pushed the lms/fix-sveltekit-meta-tags-twp branch from b0d22ad to 5e2e77a Compare September 18, 2024 14:40
@mydea
Copy link
Member

mydea commented Sep 18, 2024

hmm, it feels a bit excessive to have a separate E2E app for TWP, feels like a slippery slope to add this for every framework? 😅 no strong feelings but our list of E2E apps is ever growing 😬 is there not a way to test this (at least in some simplified way) without an E2E test app?

Copy link

codecov bot commented Sep 18, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
623 1 622 33
View the full list of 1 ❄️ flaky tests
errorboundary.test.ts captures an exception

Flake rate in main: 29.41% (Passed 12 times, Failed 5 times)

Stack Traces | 30s run time
errorboundary.test.ts:4:1 captures an exception

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@Lms24
Copy link
Member Author

Lms24 commented Sep 18, 2024

our list of E2E apps is ever growing

that we won't solve long-term if we remove this one 😅 I think we need to move the e2e tests out of this repo sooner or later but that's another story...

I'll remove the test app. We can do this now more easily because we're at least testing TwP now better in Node integration tests. Unfortunately, I don't think there's a way to realistically test TwP within a "tracing with performance" e2e test app.
The test app already served its purpose already, as otherwise, I never would have discovered the bug fixed in #13381.

@Lms24
Copy link
Member Author

Lms24 commented Sep 18, 2024

Actually, I do believe there's value to have one meta framework test app configured for TwP. It doesn't have to be sveltekit but there's value in ensuring that backend<->frontend errors are still connected by the same trace

@Lms24 Lms24 force-pushed the lms/fix-sveltekit-meta-tags-twp branch 2 times, most recently from d7e3d01 to 1886c87 Compare September 27, 2024 13:34
@Lms24 Lms24 force-pushed the lms/fix-sveltekit-meta-tags-twp branch from 24d5518 to 73eb9c4 Compare October 7, 2024 13:28
@Lms24 Lms24 enabled auto-merge (squash) October 8, 2024 14:57
@Lms24 Lms24 merged commit 5929a1b into develop Oct 8, 2024
146 of 148 checks passed
@Lms24 Lms24 deleted the lms/fix-sveltekit-meta-tags-twp branch October 8, 2024 15:09
alexandresoro pushed a commit to alexandresoro/ouca that referenced this pull request Oct 10, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.33.1` -> `8.34.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.33.1/8.34.0) |
| [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.33.1` -> `8.34.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.33.1/8.34.0) |

---

### Release Notes

<details>
<summary>getsentry/sentry-javascript (@&#8203;sentry/node)</summary>

### [`v8.34.0`](https://github.com/getsentry/sentry-javascript/releases/tag/8.34.0)

[Compare Source](getsentry/sentry-javascript@8.33.1...8.34.0)

##### Important Changes

-   **ref(nextjs): Remove dead code ([#&#8203;13828](getsentry/sentry-javascript#13903

Relevant for users of the `@sentry/nextjs` package: If you have previously configured a
`SENTRY_IGNORE_API_RESOLUTION_ERROR` environment variable, it is now safe to unset it.

##### Other Changes

-   feat(cdn): Export `getReplay` in replay CDN bundles
    ([#&#8203;13881](getsentry/sentry-javascript#13881))
-   feat(replay): Clear fallback buffer when switching buffers
    ([#&#8203;13914](getsentry/sentry-javascript#13914))
-   feat(replay): Upgrade rrweb packages to 2.28.0 ([#&#8203;13732](getsentry/sentry-javascript#13732))
-   fix(docs): Correct supported browsers due to `globalThis`
    ([#&#8203;13788](getsentry/sentry-javascript#13788))
-   fix(nextjs): Adjust path to `requestAsyncStorageShim.js` template file
    ([#&#8203;13928](getsentry/sentry-javascript#13928))
-   fix(nextjs): Detect new locations for request async storage to support Next.js v15.0.0-canary.180 and higher
    ([#&#8203;13920](getsentry/sentry-javascript#13920))
-   fix(nextjs): Drop `_not-found` spans for all HTTP methods
    ([#&#8203;13906](getsentry/sentry-javascript#13906))
-   fix(nextjs): Fix resolution of request storage shim fallback
    ([#&#8203;13929](getsentry/sentry-javascript#13929))
-   fix(node): Ensure graphql options are correct when preloading
    ([#&#8203;13769](getsentry/sentry-javascript#13769))
-   fix(node): Local variables handle error ([#&#8203;13827](getsentry/sentry-javascript#13827))
-   fix(node): Remove `dataloader` instrumentation from default integrations
    ([#&#8203;13873](getsentry/sentry-javascript#13873))
-   fix(nuxt): Create declaration files for Nuxt module
    ([#&#8203;13909](getsentry/sentry-javascript#13909))
-   fix(replay): Ensure `replay_id` is removed from frozen DSC when stopped
    ([#&#8203;13893](getsentry/sentry-javascript#13893))
-   fix(replay): Try/catch `sendBufferedReplayOrFlush` to prevent cycles
    ([#&#8203;13900](getsentry/sentry-javascript#13900))
-   fix(sveltekit): Ensure trace meta tags are always injected
    ([#&#8203;13231](getsentry/sentry-javascript#13231))
-   fix(sveltekit): Update `wrapServerRouteWithSentry` to respect ParamMatchers
    ([#&#8203;13390](getsentry/sentry-javascript#13390))
-   fix(wasm): Integration wasm uncaught WebAssembly.Exception
    ([#&#8203;13787](getsentry/sentry-javascript#13787)) ([#&#8203;13854](getsentry/sentry-javascript#13854))
-   ref(nextjs): Ignore sentry spans based on query param attribute
    ([#&#8203;13905](getsentry/sentry-javascript#13905))
-   ref(utils): Move `vercelWaitUntil` to utils ([#&#8203;13891](getsentry/sentry-javascript#13891))

Work in this release was contributed by [@&#8203;trzeciak](https://github.com/trzeciak), [@&#8203;gurpreetatwal](https://github.com/gurpreetatwal), [@&#8203;ykzts](https://github.com/ykzts) and [@&#8203;lizhiyao](https://github.com/lizhiyao). Thank you for your
contributions!

##### Bundle size 📦

| Path                                                             | Size              |
| ---------------------------------------------------------------- | ----------------- |
| [@&#8203;sentry/browser](https://github.com/sentry/browser)                                                  | 22.73 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) - with treeshaking flags                         | 21.53 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing)                                  | 34.97 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay)                          | 71.62 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) - with treeshaking flags | 62.03 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay with Canvas)              | 75.97 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback)                | 88.73 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback, metrics)       | 90.59 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. metrics)                                  | 27 KB     |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. Feedback)                                 | 39.87 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. sendFeedback)                             | 27.38 KB  |
| [@&#8203;sentry/browser](https://github.com/sentry/browser) (incl. FeedbackAsync)                            | 32.17 KB  |
| [@&#8203;sentry/react](https://github.com/sentry/react)                                                    | 25.49 KB  |
| [@&#8203;sentry/react](https://github.com/sentry/react) (incl. Tracing)                                    | 37.94 KB  |
| [@&#8203;sentry/vue](https://github.com/sentry/vue)                                                      | 26.91 KB  |
| [@&#8203;sentry/vue](https://github.com/sentry/vue) (incl. Tracing)                                      | 36.86 KB  |
| [@&#8203;sentry/svelte](https://github.com/sentry/svelte)                                                   | 22.87 KB  |
| CDN Bundle                                                       | 24.05 KB  |
| CDN Bundle (incl. Tracing)                                       | 36.76 KB  |
| CDN Bundle (incl. Tracing, Replay)                               | 71.38 KB  |
| CDN Bundle (incl. Tracing, Replay, Feedback)                     | 76.7 KB   |
| CDN Bundle - uncompressed                                        | 70.53 KB  |
| CDN Bundle (incl. Tracing) - uncompressed                        | 109.04 KB |
| CDN Bundle (incl. Tracing, Replay) - uncompressed                | 221.4 KB  |
| CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed      | 234.62 KB |
| [@&#8203;sentry/nextjs](https://github.com/sentry/nextjs) (client)                                          | 37.91 KB  |
| [@&#8203;sentry/sveltekit](https://github.com/sentry/sveltekit) (client)                                       | 35.56 KB  |
| [@&#8203;sentry/node](https://github.com/sentry/node)                                                     | 124.5 KB  |
| [@&#8203;sentry/node](https://github.com/sentry/node) - without tracing                                   | 93.64 KB  |
| [@&#8203;sentry/aws-serverless](https://github.com/sentry/aws-serverless)                                           | 103.3 KB  |

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4xMTYuMCIsInVwZGF0ZWRJblZlciI6IjM4LjExNi4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=-->

Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/212
Reviewed-by: Alexandre Soro <code@soro.dev>
Co-authored-by: renovate <renovate@git.tristess.app>
Co-committed-by: renovate <renovate@git.tristess.app>
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.

[Sveltekit] Inject meta tags in Tracing-without-Performance-mode
3 participants