fix(node): Fix virtual parent span ID handling & update create-next-app E2E test #12368

mydea · 2024-06-05T09:18:25Z

This started out as updating the create-next-app test to not send to sentry anymore, but instead check payloads.

However, while doing this, I noticed some inconsistencies, mostly that there was a weird parent_span_id in api route transactions/errors where there should be none.

EDIT

OK, another iteration on this!

Turns out setting an invalid spanID on a span will make OTEL ignore all of this, including the trace ID, and instead will create a new trace ID for a new span, which is not what we want. So we can't use this...

So instead, I now adjusted the already existing code to keep the incoming parentSpanId on the trace state. The major change I did is ensure we set this even if it is empty (it is set to an empty string then). This way, we can identify if this has been set, and if it has, use this as source of truth. And we can fall back to use the regular parentSpanId if this is not set (for whatever reason).

ORIGINAL

So I set out to figure out what was happening there, and the problem was that when continuing a virtual trace, we would construct a parent spanContext like this:

const spanContext: SpanContext = {
  traceId: propagationContext.traceId,
  spanId: propagationContext.parentSpanId || propagationContext.spanId,
  isRemote: true,
  traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
  traceState,
};

The problematic line is this: spanId: propagationContext.parentSpanId || propagationContext.spanId,. Since spanId is required on the SpanContext, we had to set it to something, but propagationContext.parentSpanId is by design often undefined. With this behavior, we always set this to the random span ID we have on the propagationContext, and picked this up downstream.

this now became:

const spanContext: SpanContext = {
  traceId: propagationContext.traceId,
  spanId: propagationContext.parentSpanId || INVALID_SPANID,
  isRemote: true,
  traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
  traceState,
};

Plus a check further down:

const traceState = makeTraceState({
  dsc,
  parentSpanId: spanId !== INVALID_SPANID ? spanId : undefined,
  sampled,
});

(Note, INVALID_SPANID is a constant exported from OTEL, which is basically 0000....).

I'll investigate in a follow up if it would make sense to always use this for the propagation context, instead of a random one today, plus ensuring that we always filter this out before we send, or something like this 🤔

Part of #11910

dev-packages/e2e-tests/test-applications/create-next-app/tests/server-transactions.test.ts

mydea · 2024-06-06T08:34:51Z

dev-packages/e2e-tests/package.json

@@ -14,7 +14,7 @@
    "test:prepare": "ts-node prepare.ts",
    "test:validate": "run-s test:validate-configuration test:validate-test-app-setups",
    "clean": "rimraf tmp node_modules pnpm-lock.yaml && yarn clean:test-applications",
-    "clean:test-applications": "rimraf test-applications/**/{node_modules,dist,build,.next,.sveltekit,pnpm-lock.yaml} .last-run.json"
+    "clean:test-applications": "rimraf test-applications/**/{node_modules,dist,build,.next,.sveltekit,pnpm-lock.yaml} .last-run.json && pnpm store prune"


Noticed that without this, running tests locally would often get cached node_modules :(

mydea · 2024-06-06T08:35:49Z

packages/nextjs/src/common/utils/tracingUtils.ts

-        traceId: uuid4(),
-        spanId: uuid4().substring(16),
-      });
+    return startNewTrace(() => {


This new API does the same thing, so we can leverage this now instead.

mydea · 2024-06-06T08:36:04Z

packages/nextjs/src/common/wrapApiHandlerWithSentry.ts

@@ -81,8 +81,10 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz
                  // eslint-disable-next-line @typescript-eslint/unbound-method
                  res.end = new Proxy(res.end, {
                    apply(target, thisArg, argArray) {
-                      setHttpStatus(span, res.statusCode);
-                      span.end();
+                      if (span.isRecording()) {


Also noticed this in tests, we sometimes seemed to double-end spans, so guarding against this here.

github-actions · 2024-06-06T08:44:29Z

size-limit report 📦

Path	Size
@sentry/browser	21.92 KB (0%)
@sentry/browser (incl. Tracing)	32.97 KB (0%)
@sentry/browser (incl. Tracing, Replay)	68.57 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	61.86 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)	72.62 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)	84.74 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)	86.58 KB (+0.04% 🔺)
@sentry/browser (incl. metrics)	26.11 KB (0%)
@sentry/browser (incl. Feedback)	38.09 KB (0%)
@sentry/browser (incl. sendFeedback)	26.51 KB (0%)
@sentry/browser (incl. FeedbackAsync)	31.06 KB (0%)
@sentry/react	24.69 KB (0%)
@sentry/react (incl. Tracing)	36 KB (0%)
@sentry/vue	25.93 KB (0%)
@sentry/vue (incl. Tracing)	34.8 KB (0%)
@sentry/svelte	22.06 KB (0%)
CDN Bundle	23.3 KB (0%)
CDN Bundle (incl. Tracing)	34.68 KB (0%)
CDN Bundle (incl. Tracing, Replay)	68.63 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)	73.78 KB (+0.02% 🔺)
CDN Bundle - uncompressed	68.42 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed	102.6 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed	212.52 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	224.99 KB (+0.02% 🔺)
@sentry/nextjs (client)	35.37 KB (0%)
@sentry/sveltekit (client)	33.6 KB (0%)
@sentry/node	115.76 KB (+0.15% 🔺)
@sentry/node - without tracing	94.84 KB (+0.17% 🔺)
@sentry/aws-serverless	104.05 KB (+0.18% 🔺)

mydea · 2024-06-06T12:18:11Z

OK, another iteration on this!

Turns out setting an invalid spanID on a span will make OTEL ignore all of this, including the trace ID, and instead will create a new trace ID for a new span, which is not what we want. So we can't use this...

So instead, I now adjusted the already existing code to keep the incoming parentSpanId on the trace state. The major change I did is ensure we set this even if it is empty (it is set to an empty string then). This way, we can identify if this has been set, and if it has, use this as source of truth. And we can fall back to use the regular parentSpanId if this is not set (for whatever reason).

lforst · 2024-06-06T12:51:41Z

packages/opentelemetry/src/propagator.ts

-  const traceStateBase = parentSpanId
-    ? new TraceState().set(SENTRY_TRACE_STATE_PARENT_SPAN_ID, parentSpanId)
-    : new TraceState();
+  // We _always_ set the parent span ID, because we can infer from the existence of this if we should use this


m: This comment is a bit confusing to read because of all of this. Can we replace this with actual objects? 😄

I updated the comment!

…pp E2E test (#12368) This started out as updating the create-next-app test to not send to sentry anymore, but instead check payloads. However, while doing this, I noticed some inconsistencies, mostly that there was a weird `parent_span_id` in api route transactions/errors where there should be none. **EDIT** OK, another iteration on this! Turns out setting an invalid spanID on a span will make OTEL ignore all of this, including the trace ID, and instead will create a new trace ID for a new span, which is not what we want. So we can't use this... So instead, I now adjusted the already existing code to keep the incoming parentSpanId on the trace state. The major change I did is ensure we set this even if it is empty (it is set to an empty string then). This way, we can identify if this has been set, and if it has, use this as source of truth. And we can fall back to use the regular parentSpanId if this is not set (for whatever reason). **ORIGINAL** So I set out to figure out what was happening there, and the problem was that when continuing a virtual trace, we would construct a parent spanContext like this: ```js const spanContext: SpanContext = { traceId: propagationContext.traceId, spanId: propagationContext.parentSpanId || propagationContext.spanId, isRemote: true, traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, traceState, }; ``` The problematic line is this: `spanId: propagationContext.parentSpanId || propagationContext.spanId,`. Since `spanId` is required on the SpanContext, we had to set it to something, but `propagationContext.parentSpanId` is by design often undefined. With this behavior, we always set this to the random span ID we have on the propagationContext, and picked this up downstream. this now became: ```js const spanContext: SpanContext = { traceId: propagationContext.traceId, spanId: propagationContext.parentSpanId || INVALID_SPANID, isRemote: true, traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, traceState, }; ``` Plus a check further down: ```js const traceState = makeTraceState({ dsc, parentSpanId: spanId !== INVALID_SPANID ? spanId : undefined, sampled, }); ``` (Note, `INVALID_SPANID` is a constant exported from OTEL, which is basically `0000....`). I'll investigate in a follow up if it would make sense to always use this for the propagation context, instead of a random one today, plus ensuring that we always filter this out before we send, or something like this 🤔 Part of #11910

mydea requested a review from lforst June 5, 2024 09:18

mydea self-assigned this Jun 5, 2024

mydea commented Jun 5, 2024

View reviewed changes

dev-packages/e2e-tests/test-applications/create-next-app/tests/server-transactions.test.ts Outdated Show resolved Hide resolved

lforst approved these changes Jun 5, 2024

View reviewed changes

mydea force-pushed the fn/create-next-app-e2e branch from 2ff28ff to 136e30e Compare June 5, 2024 11:19

mydea added 2 commits June 6, 2024 10:30

test(e2e): Update create-next-app test app

e670133

fix stuff...

0532f16

mydea force-pushed the fn/create-next-app-e2e branch from 136e30e to 0532f16 Compare June 6, 2024 08:30

mydea changed the title ~~test(e2e): Update create-next-app test app~~ fix(nextjs): Fix virtual parent span ID handling & update create-next-app E2E test Jun 6, 2024

small fixes

91c0afa

mydea commented Jun 6, 2024

View reviewed changes

mydea requested a review from lforst June 6, 2024 08:36

handle parentSpanId via traceState

6b9e249

fix tests

0acfe22

mydea changed the title ~~fix(nextjs): Fix virtual parent span ID handling & update create-next-app E2E test~~ fix(node): Fix virtual parent span ID handling & update create-next-app E2E test Jun 6, 2024

mydea requested review from Lms24 and AbhiPrasad June 6, 2024 12:37

lforst approved these changes Jun 6, 2024

View reviewed changes

better comment

7c22843

mydea merged commit 0a6c1e5 into develop Jun 6, 2024
95 checks passed

mydea deleted the fn/create-next-app-e2e branch June 6, 2024 14:02

koushikpuppala mentioned this pull request Jun 29, 2024

[Snyk] Upgrade @sentry/nextjs from 8.2.1 to 8.8.0 koushikpuppala/koushikpuppala#141

Closed

This was referenced Jul 3, 2024

[Snyk] Upgrade @sentry/browser from 8.8.0 to 8.9.1 gokulk16/type-to-calculate-web#28

Closed

[Snyk] Upgrade @sentry/browser from 8.8.0 to 8.9.2 gokulk16/type-to-calculate-web#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(node): Fix virtual parent span ID handling & update create-next-app E2E test #12368

fix(node): Fix virtual parent span ID handling & update create-next-app E2E test #12368

Uh oh!

mydea commented Jun 5, 2024 •

edited

Loading

Uh oh!

Uh oh!

mydea Jun 6, 2024

Uh oh!

mydea Jun 6, 2024

Uh oh!

mydea Jun 6, 2024

Uh oh!

github-actions bot commented Jun 6, 2024 •

edited

Loading

Uh oh!

mydea commented Jun 6, 2024

Uh oh!

lforst Jun 6, 2024

Uh oh!

mydea Jun 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(node): Fix virtual parent span ID handling & update create-next-app E2E test #12368

fix(node): Fix virtual parent span ID handling & update create-next-app E2E test #12368

Uh oh!

Conversation

mydea commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mydea Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

mydea Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

mydea Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

mydea commented Jun 6, 2024

Uh oh!

lforst Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

mydea Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mydea commented Jun 5, 2024 •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading