Skip to content

Commit

Permalink
test: Add tests to demonstrate root trace ID behavior (#14426)
Browse files Browse the repository at this point in the history
This adds node & browser integration tests to demonstrate the current
behavior of "trace propagation" based on the current scope.

It shows that the behavior is not really consistent and we should
probably adjust it, but for now this PR does no changes.

## Browser

In browser, the propagation context of the current scope is always
picked up. This means that if you call `startSpan` multiple times in the
root, both started spans will have the same trace ID, and possibly the
same `parentSpanId` if there was an incoming trace.

## Node

In node, the propagation context is ignored for the root context. So in
the root, started spans will have different traces, and will also ignore
parentSpanId (which is only theoretical, there will never really be a
parentSpanId on the root scope in node, realistically).

Outside of the root (so e.g. within any route handler, or even any
`withScope` call), the behavior is the same as in browser - any started
spans will share the same trace ID/parent span ID.

## What should we take away from this?

In node, I would align the behavior to ignore the trace ID when we have
no incoming trace (so no parentSpanId), and always use the traceId &
parentSpanId if there is a parentSpanId on the scope (even the root
scope, to be consistent).

In browser, we cannot make this change because we rely on this behavior
for the extended traces after pageload/navigation spans have ended.
  • Loading branch information
mydea authored Nov 25, 2024
1 parent 05479b8 commit d8d9324
Show file tree
Hide file tree
Showing 14 changed files with 313 additions and 3 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Sentry.withScope(() => {
Sentry.startSpan({ name: 'test_span_1' }, () => undefined);
Sentry.startSpan({ name: 'test_span_2' }, () => undefined);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';

import type { TransactionEvent } from '@sentry/types';
import { sentryTest } from '../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../utils/helpers';

sentryTest(
'should send manually started parallel root spans outside of root context',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const transaction1ReqPromise = waitForTransactionRequest(page, event => event.transaction === 'test_span_1');
const transaction2ReqPromise = waitForTransactionRequest(page, event => event.transaction === 'test_span_2');

await page.goto(url);

const [transaction1Req, transaction2Req] = await Promise.all([transaction1ReqPromise, transaction2ReqPromise]);

const transaction1 = envelopeRequestParser<TransactionEvent>(transaction1Req);
const transaction2 = envelopeRequestParser<TransactionEvent>(transaction2Req);

expect(transaction1).toBeDefined();
expect(transaction2).toBeDefined();

const trace1Id = transaction1.contexts?.trace?.trace_id;
const trace2Id = transaction2.contexts?.trace?.trace_id;

expect(trace1Id).toBeDefined();
expect(trace2Id).toBeDefined();

// We use the same traceID from the root propagation context here
expect(trace1Id).toBe(trace2Id);

expect(transaction1.contexts?.trace?.parent_span_id).toBeUndefined();
expect(transaction2.contexts?.trace?.parent_span_id).toBeUndefined();
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Sentry.getCurrentScope().setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

Sentry.startSpan({ name: 'test_span_1' }, () => undefined);
Sentry.startSpan({ name: 'test_span_2' }, () => undefined);
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { expect } from '@playwright/test';

import type { TransactionEvent } from '@sentry/types';
import { sentryTest } from '../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../utils/helpers';

sentryTest(
'should send manually started parallel root spans in root context with parentSpanId',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const transaction1ReqPromise = waitForTransactionRequest(page, event => event.transaction === 'test_span_1');
const transaction2ReqPromise = waitForTransactionRequest(page, event => event.transaction === 'test_span_2');

await page.goto(url);

const [transaction1Req, transaction2Req] = await Promise.all([transaction1ReqPromise, transaction2ReqPromise]);

const transaction1 = envelopeRequestParser<TransactionEvent>(transaction1Req);
const transaction2 = envelopeRequestParser<TransactionEvent>(transaction2Req);

expect(transaction1).toBeDefined();
expect(transaction2).toBeDefined();

const trace1Id = transaction1.contexts?.trace?.trace_id;
const trace2Id = transaction2.contexts?.trace?.trace_id;

expect(trace1Id).toBe('12345678901234567890123456789012');
expect(trace2Id).toBe('12345678901234567890123456789012');

expect(transaction1.contexts?.trace?.parent_span_id).toBe('1234567890123456');
expect(transaction2.contexts?.trace?.parent_span_id).toBe('1234567890123456');
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Sentry.startSpan({ name: 'test_span_1' }, () => undefined);
Sentry.startSpan({ name: 'test_span_2' }, () => undefined);
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { expect } from '@playwright/test';

import type { TransactionEvent } from '@sentry/types';
import { sentryTest } from '../../../../utils/fixtures';
import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../../utils/helpers';

sentryTest('should send manually started parallel root spans in root context', async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const transaction1ReqPromise = waitForTransactionRequest(page, event => event.transaction === 'test_span_1');
const transaction2ReqPromise = waitForTransactionRequest(page, event => event.transaction === 'test_span_2');

await page.goto(url);

const [transaction1Req, transaction2Req] = await Promise.all([transaction1ReqPromise, transaction2ReqPromise]);

const transaction1 = envelopeRequestParser<TransactionEvent>(transaction1Req);
const transaction2 = envelopeRequestParser<TransactionEvent>(transaction2Req);

expect(transaction1).toBeDefined();
expect(transaction2).toBeDefined();

const trace1Id = transaction1.contexts?.trace?.trace_id;
const trace2Id = transaction2.contexts?.trace?.trace_id;

expect(trace1Id).toBeDefined();
expect(trace2Id).toBeDefined();

// We use the same traceID from the root propagation context here
expect(trace1Id).toBe(trace2Id);

expect(transaction1.contexts?.trace?.parent_span_id).toBeUndefined();
expect(transaction2.contexts?.trace?.parent_span_id).toBeUndefined();
});
16 changes: 14 additions & 2 deletions dev-packages/browser-integration-tests/utils/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {
Event,
EventEnvelope,
EventEnvelopeHeaders,
TransactionEvent,
} from '@sentry/types';

export const envelopeUrlRegex = /\.sentry\.io\/api\/\d+\/envelope\//;
Expand Down Expand Up @@ -224,7 +225,10 @@ export function waitForErrorRequest(page: Page, callback?: (event: Event) => boo
});
}

export function waitForTransactionRequest(page: Page): Promise<Request> {
export function waitForTransactionRequest(
page: Page,
callback?: (event: TransactionEvent) => boolean,
): Promise<Request> {
return page.waitForRequest(req => {
const postData = req.postData();
if (!postData) {
Expand All @@ -234,7 +238,15 @@ export function waitForTransactionRequest(page: Page): Promise<Request> {
try {
const event = envelopeRequestParser(req);

return event.type === 'transaction';
if (event.type !== 'transaction') {
return false;
}

if (callback) {
return callback(event as TransactionEvent);
}

return true;
} catch {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
});

Sentry.getCurrentScope().setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

const spanIdTraceId = Sentry.startSpan(
{
name: 'test_span_1',
},
span1 => span1.spanContext().traceId,
);

Sentry.startSpan(
{
name: 'test_span_2',
attributes: { spanIdTraceId },
},
() => undefined,
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

test('should send manually started parallel root spans in root context', done => {
expect.assertions(7);

createRunner(__dirname, 'scenario.ts')
.expect({ transaction: { transaction: 'test_span_1' } })
.expect({
transaction: transaction => {
expect(transaction).toBeDefined();
const traceId = transaction.contexts?.trace?.trace_id;
expect(traceId).toBeDefined();

// It ignores propagation context of the root context
expect(traceId).not.toBe('12345678901234567890123456789012');
expect(transaction.contexts?.trace?.parent_span_id).toBeUndefined();

// Different trace ID than the first span
const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId;
expect(trace1Id).toBeDefined();
expect(trace1Id).not.toBe(traceId);
},
})
.start(done);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
});

Sentry.withScope(scope => {
scope.setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

Sentry.startSpan({ name: 'test_span_1' }, () => undefined);
Sentry.startSpan({ name: 'test_span_2' }, () => undefined);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

test('should send manually started parallel root spans outside of root context with parentSpanId', done => {
createRunner(__dirname, 'scenario.ts')
.expect({
transaction: {
transaction: 'test_span_1',
contexts: {
trace: {
span_id: expect.any(String),
parent_span_id: '1234567890123456',
trace_id: '12345678901234567890123456789012',
},
},
},
})
.expect({
transaction: {
transaction: 'test_span_2',
contexts: {
trace: {
span_id: expect.any(String),
parent_span_id: '1234567890123456',
trace_id: '12345678901234567890123456789012',
},
},
},
})
.start(done);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
});

Sentry.withScope(() => {
const spanIdTraceId = Sentry.startSpan(
{
name: 'test_span_1',
},
span1 => span1.spanContext().traceId,
);

Sentry.startSpan(
{
name: 'test_span_2',
attributes: { spanIdTraceId },
},
() => undefined,
);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

test('should send manually started parallel root spans outside of root context', done => {
expect.assertions(6);

createRunner(__dirname, 'scenario.ts')
.expect({ transaction: { transaction: 'test_span_1' } })
.expect({
transaction: transaction => {
expect(transaction).toBeDefined();
const traceId = transaction.contexts?.trace?.trace_id;
expect(traceId).toBeDefined();
expect(transaction.contexts?.trace?.parent_span_id).toBeUndefined();

const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId;
expect(trace1Id).toBeDefined();

// Same trace ID as the first span
expect(trace1Id).toBe(traceId);
},
})
.start(done);
});
3 changes: 2 additions & 1 deletion packages/opentelemetry/src/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,9 @@ function ensureTimestampInMilliseconds(timestamp: number): number {

function getContext(scope: Scope | undefined, forceTransaction: boolean | undefined): Context {
const ctx = getContextForScope(scope);
// Note: If the context is the ROOT_CONTEXT, no scope is attached
// Thus we will not use the propagation context in this case, which is desired
const actualScope = getScopesFromContext(ctx)?.scope;

const parentSpan = trace.getSpan(ctx);

// In the case that we have no parent span, we need to "simulate" one to ensure the propagation context is correct
Expand Down

0 comments on commit d8d9324

Please sign in to comment.