Skip to content

feat(core): Implement strictTraceContinuation #16313

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

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { hasSpansEnabled } from '../utils/hasSpansEnabled';
import { parseSampleRate } from '../utils/parseSampleRate';
import { _getSpanForScope, _setSpanForScope } from '../utils/spanOnScope';
import { addChildSpanToSpan, getRootSpan, spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';
import { baggageHeaderToDynamicSamplingContext } from '../utils-hoist/baggage';
import { extractOrgIdFromDsnHost } from '../utils-hoist/dsn';
import { logger } from '../utils-hoist/logger';
import { generateTraceId } from '../utils-hoist/propagationContext';
import { propagationContextFromHeaders } from '../utils-hoist/tracing';
Expand Down Expand Up @@ -215,6 +217,47 @@ export const continueTrace = <V>(
}

const { sentryTrace, baggage } = options;
const client = getClient();

const clientOptions = client?.getOptions();
const strictTraceContinuation = clientOptions?.strictTraceContinuation || false; // default for `strictTraceContinuation` is `false` todo(v10): set default to `true`

const incomingDsc = baggageHeaderToDynamicSamplingContext(baggage);
const baggageOrgId = incomingDsc?.org_id;

let sdkOrgId: string | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: I think we should use the same logic here as for injecting the org id, which is use the specified one if defined, else extract it from dsn? We may also put this into a reusable utility, I suppose!

const dsn = client?.getDsn();
if (dsn?.host) {
sdkOrgId = extractOrgIdFromDsnHost(dsn.host);
}

const shouldStartNewTrace = (): boolean => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: Let's maybe make this a separate function that takes arguments instead of inlining this function? No strong feelings, but this is how we usually do this I suppose 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah no strong opinions about this. Just thought it's not a lot of logic and when inlining this it does not disrupt you from the code-reading flow but it still keeps this part of the code separated :D

// Case: baggage org ID and SDK org ID don't match - always start new trace
if (baggageOrgId && sdkOrgId && baggageOrgId !== sdkOrgId) {
DEBUG_BUILD &&
logger.info(`Starting a new trace because org IDs don't match (incoming: ${baggageOrgId}, sdk: ${sdkOrgId})`);
return true;
}

if (strictTraceContinuation) {
// With strict continuation enabled, start new trace if:
// - Baggage has org ID but SDK doesn't have one
// - SDK has org ID but baggage doesn't have one
if ((baggageOrgId && !sdkOrgId) || (!baggageOrgId && sdkOrgId)) {
DEBUG_BUILD &&
logger.info(
`Starting a new trace because strict trace continuation is enabled and one org ID is missing (incoming: ${baggageOrgId}, sdk: ${sdkOrgId})`,
);
return true;
}
}

return false;
};

if (shouldStartNewTrace()) {
return startNewTrace(callback);
}

return withScope(scope => {
const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
Expand Down
12 changes: 12 additions & 0 deletions packages/core/src/types-hoist/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,18 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
*/
tracePropagationTargets?: TracePropagationTargets;

/**
* Controls whether trace continuation should be strict about matching organization IDs.
*
* When set to `true`, the SDK will only continue a trace if the organization ID in the incoming baggage
* matches the organization ID of the current SDK (extracted from the DSN).
*
* If there is no match, the SDK will start a new trace instead of continuing the incoming one.
*
* @default false
*/
strictTraceContinuation?: boolean;

/**
* The organization ID of the current SDK. The organization ID is a string containing only numbers. This ID is used to
* propagate traces to other Sentry services.
Expand Down
146 changes: 146 additions & 0 deletions packages/core/test/lib/tracing/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type { Span } from '../../../src/types-hoist/span';
import type { StartSpanOptions } from '../../../src/types-hoist/startSpanOptions';
import { _setSpanForScope } from '../../../src/utils/spanOnScope';
import { getActiveSpan, getRootSpan, getSpanDescendants, spanIsSampled } from '../../../src/utils/spanUtils';
import { extractOrgIdFromDsnHost } from '../../../src/utils-hoist/dsn';
import { getDefaultTestClientOptions, TestClient } from '../../mocks/client';

const enum Type {
Expand Down Expand Up @@ -1733,6 +1734,151 @@ describe('continueTrace', () => {

expect(result).toEqual('aha');
});

describe('strictTraceContinuation', () => {
const creatOrgIdInDsn = (orgId: number) => {
vi.spyOn(client, 'getDsn').mockReturnValue({
host: `o${orgId}.ingest.sentry.io`,
protocol: 'https',
projectId: 'projId',
});
};

afterEach(() => {
vi.clearAllMocks();
});

it('continues trace when org IDs match', () => {
creatOrgIdInDsn(123);

const scope = continueTrace(
{
sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-org_id=123',
},
() => {
return getCurrentScope();
},
);

expect(scope.getPropagationContext().traceId).toBe('12312012123120121231201212312012');
expect(scope.getPropagationContext().parentSpanId).toBe('1121201211212012');
});

it('starts new trace when both SDK and baggage org IDs are set and do not match', () => {
creatOrgIdInDsn(123);

const scope = continueTrace(
{
sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-org_id=456',
},
() => {
return getCurrentScope();
},
);

// Should start a new trace with a different trace ID
expect(scope.getPropagationContext().traceId).not.toBe('12312012123120121231201212312012');
expect(scope.getPropagationContext().parentSpanId).toBeUndefined();
});

describe('when strictTraceContinuation is true', () => {
it('starts new trace when baggage org ID is missing', () => {
client.getOptions().strictTraceContinuation = true;

creatOrgIdInDsn(123);

const scope = continueTrace(
{
sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-environment=production',
},
() => {
return getCurrentScope();
},
);

// Should start a new trace with a different trace ID
expect(scope.getPropagationContext().traceId).not.toBe('12312012123120121231201212312012');
expect(scope.getPropagationContext().parentSpanId).toBeUndefined();
});

it('starts new trace when SDK org ID is missing', () => {
client.getOptions().strictTraceContinuation = true;

const scope = continueTrace(
{
sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-org_id=123',
},
() => {
return getCurrentScope();
},
);

// Should start a new trace with a different trace ID
expect(scope.getPropagationContext().traceId).not.toBe('12312012123120121231201212312012');
expect(scope.getPropagationContext().parentSpanId).toBeUndefined();
});

it('continues trace when both org IDs are missing', () => {
client.getOptions().strictTraceContinuation = true;

const scope = continueTrace(
{
sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-environment=production',
},
() => {
return getCurrentScope();
},
);

// Should continue the trace
expect(scope.getPropagationContext().traceId).toBe('12312012123120121231201212312012');
expect(scope.getPropagationContext().parentSpanId).toBe('1121201211212012');
});
});

describe('when strictTraceContinuation is false', () => {
it('continues trace when baggage org ID is missing', () => {
client.getOptions().strictTraceContinuation = false;

creatOrgIdInDsn(123);

const scope = continueTrace(
{
sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-environment=production',
},
() => {
return getCurrentScope();
},
);

expect(scope.getPropagationContext().traceId).toBe('12312012123120121231201212312012');
expect(scope.getPropagationContext().parentSpanId).toBe('1121201211212012');
});

it('SDK org ID is missing', () => {
client.getOptions().strictTraceContinuation = false;

const scope = continueTrace(
{
sentryTrace: '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-org_id=123',
},
() => {
return getCurrentScope();
},
);

expect(scope.getPropagationContext().traceId).toBe('12312012123120121231201212312012');
expect(scope.getPropagationContext().parentSpanId).toBe('1121201211212012');
});
});
});
});

describe('getActiveSpan', () => {
Expand Down
Loading