fix(opentelemetry): Always use active span in `Propagator.inject` #13381

Lms24 · 2024-08-14T14:51:34Z

This PR attempts to fix an inconsistency in TwP mode where we previously returned a different trace id in getTraceData than the one we include in the trace context of error events.

Background:

when we create the error event, we create the trace context from the active span (if there is one)
- since there always is an ambient, non-recording span in TwP mode, we always take this span's traceId
for getTraceData, we take the span id from Propagator.inject
- which checks hasTracingEnabled to either return the traceId from a span (true) or from the propagationContext on the scope (false).
- this happens in the getInjectionData helper function

=> This PR removes the check for hasTracingEnabled in getInjectionData and simply always takes the non-recording span if there is one. Which I guess(?) is always the case 🤔

I'm not sure if this is the way to fix this but it is one way and it seems to not break any existing tests.
Also added a regression test that only passes with this fix.

Alternatively, we can adapt the error event creation pipeline to follow the same logic for the trace context as in our propagator. For example, we create a potel/core (acs) implementation for something like getTraceContext. I can look into this if reviewers prefer it. I guess it comes down to: Do we effectively want to replace the propagationContext with the ambient non-recording span or should we still fall back to the PC if we're in TwP mode?

github-actions · 2024-08-14T14:56:26Z

size-limit report 📦

Path	Size	% Change	Change
@sentry/browser	22.52 KB	-	-
@sentry/browser - with treeshaking flags	21.3 KB	-	-
@sentry/browser (incl. Tracing)	34.8 KB	-	-
@sentry/browser (incl. Tracing, Replay)	71.26 KB	-	-
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	61.7 KB	-	-
@sentry/browser (incl. Tracing, Replay with Canvas)	75.61 KB	-	-
@sentry/browser (incl. Tracing, Replay, Feedback)	88.39 KB	-	-
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)	90.23 KB	-	-
@sentry/browser (incl. metrics)	26.83 KB	-	-
@sentry/browser (incl. Feedback)	39.66 KB	-	-
@sentry/browser (incl. sendFeedback)	27.19 KB	-	-
@sentry/browser (incl. FeedbackAsync)	31.96 KB	-	-
@sentry/react	25.28 KB	-	-
@sentry/react (incl. Tracing)	37.77 KB	-	-
@sentry/vue	26.72 KB	-	-
@sentry/vue (incl. Tracing)	36.67 KB	-	-
@sentry/svelte	22.66 KB	-	-
CDN Bundle	23.83 KB	-	-
CDN Bundle (incl. Tracing)	36.56 KB	-	-
CDN Bundle (incl. Tracing, Replay)	71.02 KB	-	-
CDN Bundle (incl. Tracing, Replay, Feedback)	76.33 KB	-	-
CDN Bundle - uncompressed	69.81 KB	-	-
CDN Bundle (incl. Tracing) - uncompressed	108.44 KB	-	-
CDN Bundle (incl. Tracing, Replay) - uncompressed	220.21 KB	-	-
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	233.43 KB	-	-
@sentry/nextjs (client)	37.53 KB	-	-
@sentry/sveltekit (client)	35.37 KB	-	-
@sentry/node	121.05 KB	-0.01%	-6 B 🔽
@sentry/node - without tracing	93.34 KB	-	-
@sentry/aws-serverless	103.04 KB	-0.01%	-6 B 🔽

View base workflow run

Lms24 · 2024-08-14T16:20:55Z

dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js

+      res.send({
+        traceData: Sentry.getTraceData(),
+        traceMetaTags: Sentry.getTraceMetaTags(),
+        errorTraceContext: event.contexts.trace,
+      });


I get this is not the way how we usually test event payloads but I don't think I can assert on a specific traceId in our runner expectError function which we don't know upfront.

Lms24 · 2024-08-14T16:21:31Z

dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

+    expect(traceData).toEqual({
+      'sentry-trace': `${traceId}-${spanId}`,
+      baggage: expect.stringContaining(`sentry-trace_id=${traceId}`),
+    });
+
+    expect(traceMetaTags).toContain(`content="${traceId}-${spanId}"/>\n`);
+    expect(traceMetaTags).toContain(`sentry-trace_id=${traceId}`);


these trace and span ids were not the same as the error trace context's trace id without the change

Lms24 · 2024-08-14T16:53:58Z

Hmm I don't understand why the node integration tests fail on CI. They pass locally so I suspect it's rather a test problem than a problem with the change. But I'll look into this on Monday.

mydea

I am not 100% sure anymore if/why this was necessary in the PR that introduced this (#11564). But if tests pass, this should be fine I'd say 😅

FWIW there is not always a remote span active, only if an incoming trace is continued I believe.

Lms24 · 2024-08-30T10:25:33Z

FWIW there is not always a remote span active, only if an incoming trace is continued I believe.

Right... let me check the scenario for when there's no incoming trace then 😅

Lms24 · 2024-09-18T08:18:45Z

FWIW there is not always a remote span active, only if an incoming trace is continued I believe.

Right... let me check the scenario for when there's no incoming trace then 😅

@mydea so the thing is, the test I added has no incoming trace headers. So looks like we always have an active span now? 😅

mydea · 2024-09-18T09:08:45Z

FWIW there is not always a remote span active, only if an incoming trace is continued I believe.

Right... let me check the scenario for when there's no incoming trace then 😅

@mydea so the thing is, the test I added has no incoming trace headers. So looks like we always have an active span now? 😅

Not 100% sure, but I think we always have an active span if there is an incoming request. We probably (I think?) have none when outside of a request context 🤔 I think the propagator always creates a virtual span, but extract only runs for incoming requests!

Lms24 · 2024-09-18T09:22:44Z

Ahh I see, so adding a test without a server should cover this scenario then, right?

mydea · 2024-09-18T09:43:52Z

Ahh I see, so adding a test without a server should cover this scenario then, right?

I'd say so 👍

Lms24 · 2024-09-18T12:52:18Z

I added a test for non-server/request events. Also rewrote the existing test for events within a request. Looks like TwP mode now has consistent traces everywhere. Just gotta get them passing in CI now 😅

Lms24 · 2024-09-18T14:36:31Z

Ahh yes, I think this makes sense now! In case there's no active span, we simply fall back to reading the trace from the propagationContext on the scope. Which is identical to what we do when populating the trace context. I'm going to merge this PR.

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.30.0` -> `8.31.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.30.0/8.31.0) | | [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.30.0` -> `8.31.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.30.0/8.31.0) | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@sentry/node)</summary> ### [`v8.31.0`](https://github.com/getsentry/sentry-javascript/blob/HEAD/CHANGELOG.md#8310) [Compare Source](getsentry/sentry-javascript@8.30.0...8.31.0) ##### Important Changes - **feat(node): Add `dataloader` integration ([#13664](getsentry/sentry-javascript#13664 This release adds a new integration for the [`dataloader` package](https://www.npmjs.com/package/dataloader). The Node SDK (and all SDKs that depend on it) will now automatically instrument `dataloader` instances. You can also add it manually: ```js Sentry.init({ integrations: [Sentry.dataloaderIntegration()], }); ``` ##### Other Changes - feat(browser): Add navigation `activationStart` timestamp to pageload span ([#13658](getsentry/sentry-javascript#13658)) - feat(gatsby): Add optional `deleteSourcemapsAfterUpload` ([#13610](getsentry/sentry-javascript#13610)) - feat(nextjs): Give app router prefetch requests a `http.server.prefetch` op ([#13600](getsentry/sentry-javascript#13600)) - feat(nextjs): Improve Next.js serverside span data quality ([#13652](getsentry/sentry-javascript#13652)) - feat(node): Add `disableInstrumentationWarnings` option ([#13693](getsentry/sentry-javascript#13693)) - feat(nuxt): Adding `experimental_basicServerTracing` option to Nuxt module ([#13643](getsentry/sentry-javascript#13643)) - feat(nuxt): Improve logs about adding Node option 'import' ([#13726](getsentry/sentry-javascript#13726)) - feat(replay): Add `onError` callback + other small improvements to debugging ([#13721](getsentry/sentry-javascript#13721)) - feat(replay): Add experimental option to allow for a checkout every 6 minutes ([#13069](getsentry/sentry-javascript#13069)) - feat(wasm): Unconditionally parse instruction addresses ([#13655](getsentry/sentry-javascript#13655)) - fix: Ensure all logs are wrapped with `consoleSandbox` ([#13690](getsentry/sentry-javascript#13690)) - fix(browser): Try multiple options for `lazyLoadIntegration` script parent element lookup ([#13717](getsentry/sentry-javascript#13717)) - fix(feedback): Actor color applies to feedback icon ([#13702](getsentry/sentry-javascript#13702)) - fix(feedback): Fix form width on mobile devices ([#13068](getsentry/sentry-javascript#13068)) - fix(nestjs): Preserve original function name on `SentryTraced` functions ([#13684](getsentry/sentry-javascript#13684)) - fix(node): Don't overwrite local variables for re-thrown errors ([#13644](getsentry/sentry-javascript#13644)) - fix(normalize): Treat Infinity as NaN both are non-serializable numbers ([#13406](getsentry/sentry-javascript#13406)) - fix(nuxt): Use correct server output file path ([#13725](getsentry/sentry-javascript#13725)) - fix(opentelemetry): Always use active span in `Propagator.inject` ([#13381](getsentry/sentry-javascript#13381)) - fix(replay): Fixes potential out-of-order segments ([#13609](getsentry/sentry-javascript#13609)) Work in this release was contributed by [@KyGuy2002](https://github.com/KyGuy2002), [@artzhookov](https://github.com/artzhookov), and [@julianCast](https://github.com/julianCast). Thank you for your contributions! </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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/149 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>

Lms24 closed this Aug 14, 2024

Lms24 reopened this Aug 14, 2024

Lms24 commented Aug 14, 2024

View reviewed changes

Lms24 requested review from mydea and AbhiPrasad August 14, 2024 16:38

Lms24 changed the title ~~fix(opentelemetry): Always use active span in getInjectionData~~ fix(opentelemetry): Always use active span in Propagator.inject Aug 14, 2024

Lms24 self-assigned this Aug 14, 2024

Lms24 marked this pull request as ready for review August 14, 2024 16:54

Lms24 force-pushed the lms/fix-opentelemetry-getInjectionData branch from 29e1ae0 to 733483d Compare August 30, 2024 08:45

mydea approved these changes Aug 30, 2024

View reviewed changes

Lms24 added 4 commits September 18, 2024 10:11

fix(opentelemetry): Always use active span in getInjectionData

b1f4ccf

formatting, linting

d558c1b

add test

921d7f0

cleanup

79921ff

Lms24 force-pushed the lms/fix-opentelemetry-getInjectionData branch from 733483d to 79921ff Compare September 18, 2024 08:20

Lms24 added 2 commits September 18, 2024 10:34

fix test?

d35b8f8

tmp debug logs

68ca4e2

Lms24 added 2 commits September 18, 2024 11:24

more debug logs

e394beb

more logs pls?

265865a

Lms24 added 4 commits September 18, 2024 12:49

does this trigger a different error now?

c4be89b

,

f97a740

..

2192d6f

rewrite test, add new test

ef9aa48

lint

d3aa88c

Lms24 merged commit 14f0b6e into develop Sep 18, 2024
130 checks passed

Lms24 deleted the lms/fix-opentelemetry-getInjectionData branch September 18, 2024 14:37

Lms24 mentioned this pull request Sep 18, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(opentelemetry): Always use active span in `Propagator.inject` #13381

fix(opentelemetry): Always use active span in `Propagator.inject` #13381

Lms24 commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

Lms24 Aug 14, 2024

Lms24 Aug 14, 2024

Lms24 commented Aug 14, 2024

mydea left a comment

Lms24 commented Aug 30, 2024

Lms24 commented Sep 18, 2024

mydea commented Sep 18, 2024

Lms24 commented Sep 18, 2024

mydea commented Sep 18, 2024

Lms24 commented Sep 18, 2024

Lms24 commented Sep 18, 2024

fix(opentelemetry): Always use active span in Propagator.inject #13381

fix(opentelemetry): Always use active span in Propagator.inject #13381

Conversation

Lms24 commented Aug 14, 2024 • edited Loading

github-actions bot commented Aug 14, 2024 • edited Loading

size-limit report 📦

Lms24 Aug 14, 2024

Choose a reason for hiding this comment

Lms24 Aug 14, 2024

Choose a reason for hiding this comment

Lms24 commented Aug 14, 2024

mydea left a comment

Choose a reason for hiding this comment

Lms24 commented Aug 30, 2024

Lms24 commented Sep 18, 2024

mydea commented Sep 18, 2024

Lms24 commented Sep 18, 2024

mydea commented Sep 18, 2024

Lms24 commented Sep 18, 2024

Lms24 commented Sep 18, 2024

fix(opentelemetry): Always use active span in `Propagator.inject` #13381

fix(opentelemetry): Always use active span in `Propagator.inject` #13381

Lms24 commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading