Skip to content

Commit

Permalink
fix(node): Fix virtual parent span ID handling & update create-next-a…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
mydea authored Jun 6, 2024
1 parent a062912 commit 0a6c1e5
Show file tree
Hide file tree
Showing 31 changed files with 320 additions and 386 deletions.
2 changes: 1 addition & 1 deletion dev-packages/e2e-tests/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
"devDependencies": {
"@types/glob": "8.0.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +0,0 @@
interface Window {
recordedTransactions?: string[];
capturedExceptionId?: string;
}
Original file line number Diff line number Diff line change
@@ -1,33 +1,13 @@
import * as Sentry from '@sentry/nextjs';

declare global {
namespace globalThis {
var transactionIds: string[];
}
}

export function register() {
if (process.env.NEXT_RUNTIME === 'nodejs') {
Sentry.init({
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.NEXT_PUBLIC_E2E_TEST_DSN,
// Adjust this value in production, or use tracesSampler for greater control
tracesSampleRate: 1.0,
integrations: [Sentry.localVariablesIntegration()],
});

Sentry.addEventProcessor(event => {
global.transactionIds = global.transactionIds || [];

if (event.type === 'transaction') {
const eventId = event.event_id;

if (eventId) {
global.transactionIds.push(eventId);
}
}

return event;
tunnel: 'http://localhost:3031',
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"private": true,
"scripts": {
"build": "next build",
"clean": "npx rimraf node_modules pnpm-lock.yaml",
"clean": "npx rimraf node_modules pnpm-lock.yaml .next",
"test:prod": "TEST_ENV=prod playwright test",
"test:dev": "TEST_ENV=dev playwright test",
"test:build": "pnpm install && npx playwright install && pnpm build",
Expand All @@ -23,7 +23,8 @@
"typescript": "4.9.5"
},
"devDependencies": {
"@playwright/test": "^1.44.1"
"@playwright/test": "^1.44.1",
"@sentry-internal/test-utils": "link:../../../test-utils"
},
"volta": {
"extends": "../../package.json"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import * as Sentry from '@sentry/nextjs';
// Next.js API route support: https://nextjs.org/docs/api-routes/introduction
import type { NextApiRequest, NextApiResponse } from 'next';

export default async function handler(req: NextApiRequest, res: NextApiResponse) {
const exceptionId = Sentry.captureException(new Error('This is an error'));

await Sentry.flush(2000);

res.status(200).json({ exceptionId });
throw new Error('I am a server error!');
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ import * as Sentry from '@sentry/nextjs';
import type { NextApiRequest, NextApiResponse } from 'next';

export default function handler(req: NextApiRequest, res: NextApiResponse) {
Sentry.startSpan({ name: 'test-span' }, span => undefined);
Sentry.startSpan({ name: 'test-span' }, () => undefined);

Sentry.flush().then(() => {
res.status(200).json({
transactionIds: global.transactionIds,
});
res.status(200).json({});
});
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as Sentry from '@sentry/nextjs';
import Head from 'next/head';
import Link from 'next/link';

Expand All @@ -17,8 +16,7 @@ export default function Home() {
value="Capture Exception"
id="exception-button"
onClick={() => {
const eventId = Sentry.captureException(new Error('I am an error!'));
window.capturedExceptionId = eventId;
throw new Error('I am an error!');
}}
/>
<Link href="/user/5" id="navigation">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { getPlaywrightConfig } from '@sentry-internal/test-utils';

const testEnv = process.env.TEST_ENV;

if (!testEnv) {
throw new Error('No test env defined');
}

const config = getPlaywrightConfig({
startCommand: testEnv === 'development' ? `pnpm next dev -p 3030` : `pnpm next start -p 3030`,
});

export default config;

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,6 @@ import * as Sentry from '@sentry/nextjs';
Sentry.init({
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.NEXT_PUBLIC_E2E_TEST_DSN,
// Adjust this value in production, or use tracesSampler for greater control
tracesSampleRate: 1.0,

// ...
// Note: if you want to override the automatic release value, do not set a
// `release` value here - use the environment variable `SENTRY_RELEASE`, so
// that it will also get attached to your source maps
});

Sentry.addEventProcessor(event => {
if (
event.type === 'transaction' &&
(event.contexts?.trace?.op === 'pageload' || event.contexts?.trace?.op === 'navigation')
) {
const eventId = event.event_id;
if (eventId) {
window.recordedTransactions = window.recordedTransactions || [];
window.recordedTransactions.push(eventId);
}
}

return event;
tunnel: 'http://localhost:3031',
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { startEventProxyServer } from '@sentry-internal/test-utils';

startEventProxyServer({
port: 3031,
proxyServerName: 'create-next-app',
});

This file was deleted.

Loading

0 comments on commit 0a6c1e5

Please sign in to comment.