Skip to content
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

feat(tracing): Add PropagationContext to scope #8421

Merged
merged 10 commits into from
Jun 29, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ sentryTest('should capture replays (@sentry/browser export)', async ({ getLocalT
version: SDK_VERSION,
name: 'sentry.javascript.browser',
},
sdkProcessingMetadata: {},
request: {
url: expect.stringContaining('/dist/index.html'),
headers: {
Expand Down Expand Up @@ -94,7 +93,6 @@ sentryTest('should capture replays (@sentry/browser export)', async ({ getLocalT
version: SDK_VERSION,
name: 'sentry.javascript.browser',
},
sdkProcessingMetadata: {},
request: {
url: expect.stringContaining('/dist/index.html'),
headers: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ sentryTest('should capture replays (@sentry/replay export)', async ({ getLocalTe
version: SDK_VERSION,
name: 'sentry.javascript.browser',
},
sdkProcessingMetadata: {},
request: {
url: expect.stringContaining('/dist/index.html'),
headers: {
Expand Down Expand Up @@ -94,7 +93,6 @@ sentryTest('should capture replays (@sentry/replay export)', async ({ getLocalTe
version: SDK_VERSION,
name: 'sentry.javascript.browser',
},
sdkProcessingMetadata: {},
request: {
url: expect.stringContaining('/dist/index.html'),
headers: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const DEFAULT_REPLAY_EVENT = {
version: SDK_VERSION,
name: 'sentry.javascript.browser',
},
sdkProcessingMetadata: {},
request: {
url: expect.stringContaining('/dist/index.html'),
headers: {
Expand Down
47 changes: 46 additions & 1 deletion packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
Integration,
IntegrationClass,
Outcome,
PropagationContext,
SdkMetadata,
Session,
SessionAggregates,
Expand All @@ -29,6 +30,7 @@ import {
addItemToEnvelope,
checkOrSetAlreadyCaught,
createAttachmentEnvelopeItem,
dropUndefinedKeys,
isPlainObject,
isPrimitive,
isThenable,
Expand All @@ -41,6 +43,7 @@ import {
} from '@sentry/utils';

import { getEnvelopeEndpointWithUrlEncodedAuth } from './api';
import { DEFAULT_ENVIRONMENT } from './constants';
import { createEventEnvelope, createSessionEnvelope } from './envelope';
import type { IntegrationIndex } from './integration';
import { setupIntegration, setupIntegrations } from './integration';
Expand Down Expand Up @@ -507,7 +510,49 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
if (!hint.integrations && integrations.length > 0) {
hint.integrations = integrations;
}
return prepareEvent(options, event, hint, scope);
return prepareEvent(options, event, hint, scope).then(evt => {
if (evt === null) {
return evt;
}

// If a trace context is not set on the event, we use the propagationContext set on the event to
// generate a trace context. If the propagationContext does not have a dynamic sampling context, we
// also generate one for it.
const { propagationContext } = evt.sdkProcessingMetadata || {};
const trace = evt.contexts && evt.contexts.trace;
if (!trace && propagationContext) {
const { traceId: trace_id, spanId, parentSpanId, dsc } = propagationContext as PropagationContext;
evt.contexts = {
trace: {
trace_id,
span_id: spanId,
parent_span_id: parentSpanId,
},
...evt.contexts,
};

const { publicKey: public_key } = this.getDsn() || {};
const { segment: user_segment } = (scope && scope.getUser()) || {};

let dynamicSamplingContext = dsc;
if (!dsc) {
dynamicSamplingContext = dropUndefinedKeys({
environment: options.environment || DEFAULT_ENVIRONMENT,
release: options.release,
user_segment,
public_key,
trace_id,
});
this.emit && this.emit('createDsc', dynamicSamplingContext);
}

evt.sdkProcessingMetadata = {
dynamicSamplingContext,
...evt.sdkProcessingMetadata,
};
}
return evt;
});
}

/**
Expand Down
36 changes: 35 additions & 1 deletion packages/core/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type {
Extra,
Extras,
Primitive,
PropagationContext,
RequestSession,
Scope as ScopeInterface,
ScopeContext,
Expand All @@ -29,6 +30,7 @@ import {
isThenable,
logger,
SyncPromise,
uuid4,
} from '@sentry/utils';

import { updateSession } from './session';
Expand Down Expand Up @@ -70,6 +72,9 @@ export class Scope implements ScopeInterface {
/** Attachments */
protected _attachments: Attachment[];

/** Propagation Context for distributed tracing */
protected _propagationContext: PropagationContext;

/**
* A place to stash data which is needed at some point in the SDK's event processing pipeline but which shouldn't get
* sent to Sentry
Expand Down Expand Up @@ -108,6 +113,7 @@ export class Scope implements ScopeInterface {
this._extra = {};
this._contexts = {};
this._sdkProcessingMetadata = {};
this._propagationContext = generatePropagationContext();
}

/**
Expand All @@ -131,6 +137,7 @@ export class Scope implements ScopeInterface {
newScope._requestSession = scope._requestSession;
newScope._attachments = [...scope._attachments];
newScope._sdkProcessingMetadata = { ...scope._sdkProcessingMetadata };
newScope._propagationContext = { ...scope._propagationContext };
}
return newScope;
}
Expand Down Expand Up @@ -347,6 +354,9 @@ export class Scope implements ScopeInterface {
if (captureContext._requestSession) {
this._requestSession = captureContext._requestSession;
}
if (captureContext._propagationContext) {
this._propagationContext = captureContext._propagationContext;
}
} else if (isPlainObject(captureContext)) {
// eslint-disable-next-line no-param-reassign
captureContext = captureContext as ScopeContext;
Expand All @@ -365,6 +375,9 @@ export class Scope implements ScopeInterface {
if (captureContext.requestSession) {
this._requestSession = captureContext.requestSession;
}
if (captureContext.propagationContext) {
this._propagationContext = captureContext.propagationContext;
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm seems like we have quite some duplicated logic here when distinguishing between captureContext being a class instance or a plain object. No action required in this PR but we should probable deduplicate this.

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 something we can keep in mind, I'll leave a note.

}

return this;
Expand All @@ -387,6 +400,7 @@ export class Scope implements ScopeInterface {
this._session = undefined;
this._notifyScopeListeners();
this._attachments = [];
this._propagationContext = generatePropagationContext();
return this;
}

Expand Down Expand Up @@ -500,7 +514,11 @@ export class Scope implements ScopeInterface {
event.breadcrumbs = [...(event.breadcrumbs || []), ...this._breadcrumbs];
event.breadcrumbs = event.breadcrumbs.length > 0 ? event.breadcrumbs : undefined;

event.sdkProcessingMetadata = { ...event.sdkProcessingMetadata, ...this._sdkProcessingMetadata };
event.sdkProcessingMetadata = {
...event.sdkProcessingMetadata,
...this._sdkProcessingMetadata,
propagationContext: this._propagationContext,
};

return this._notifyEventProcessors([...getGlobalEventProcessors(), ...this._eventProcessors], event, hint);
}
Expand All @@ -514,6 +532,14 @@ export class Scope implements ScopeInterface {
return this;
}

/**
* @inheritdoc
*/
public setPropagationContext(context: PropagationContext): this {
this._propagationContext = context;
return this;
}

/**
* This will be called after {@link applyToEvent} is finished.
*/
Expand Down Expand Up @@ -598,3 +624,11 @@ function getGlobalEventProcessors(): EventProcessor[] {
export function addGlobalEventProcessor(callback: EventProcessor): void {
getGlobalEventProcessors().push(callback);
}

function generatePropagationContext(): PropagationContext {
return {
traceId: uuid4(),
spanId: uuid4().substring(16),
sampled: false,
};
}
22 changes: 22 additions & 0 deletions packages/core/test/lib/base.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,28 @@ describe('BaseClient', () => {
);
});

test('it adds a trace context all events', () => {
Copy link
Member

Choose a reason for hiding this comment

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

super-l:

Suggested change
test('it adds a trace context all events', () => {
test('it adds a trace context to all events', () => {

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update in the next PR!

expect.assertions(1);

const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
const client = new TestClient(options);
const scope = new Scope();

client.captureEvent({ message: 'message' }, { event_id: 'wat' }, scope);

expect(TestClient.instance!.event!).toEqual(
expect.objectContaining({
contexts: {
trace: {
parent_span_id: undefined,
span_id: expect.any(String),
trace_id: expect.any(String),
},
},
}),
);
});

test('adds `event_id` from hint if available', () => {
expect.assertions(1);

Expand Down
54 changes: 52 additions & 2 deletions packages/hub/test/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,21 @@ describe('Scope', () => {
GLOBAL_OBJ.__SENTRY__.globalEventProcessors = undefined;
});

describe('init', () => {
test('it creates a propagation context', () => {
const scope = new Scope();

// @ts-ignore asserting on private properties
expect(scope._propagationContext).toEqual({
traceId: expect.any(String),
spanId: expect.any(String),
sampled: false,
dsc: undefined,
parentSpanId: undefined,
});
});
});

describe('attributes modification', () => {
test('setFingerprint', () => {
const scope = new Scope();
Expand Down Expand Up @@ -193,6 +208,14 @@ describe('Scope', () => {
expect(parentScope.getRequestSession()).toEqual({ status: 'ok' });
expect(scope.getRequestSession()).toEqual({ status: 'ok' });
});

test('should clone propagation context', () => {
const parentScope = new Scope();
const scope = Scope.clone(parentScope);

// @ts-ignore accessing private property for test
expect(scope._propagationContext).toEqual(parentScope._propagationContext);
});
});

describe('applyToEvent', () => {
Expand Down Expand Up @@ -220,7 +243,11 @@ describe('Scope', () => {
expect(processedEvent!.transaction).toEqual('/abc');
expect(processedEvent!.breadcrumbs![0]).toHaveProperty('message', 'test');
expect(processedEvent!.contexts).toEqual({ os: { id: '1' } });
expect(processedEvent!.sdkProcessingMetadata).toEqual({ dogs: 'are great!' });
expect(processedEvent!.sdkProcessingMetadata).toEqual({
dogs: 'are great!',
// @ts-expect-error accessing private property for test
propagationContext: scope._propagationContext,
});
});
});

Expand Down Expand Up @@ -339,7 +366,7 @@ describe('Scope', () => {
scope.setSpan(span);
const event: Event = {
contexts: {
trace: { a: 'c' },
trace: { a: 'c' } as any,
},
};
return scope.applyToEvent(event).then(processedEvent => {
Expand Down Expand Up @@ -383,6 +410,8 @@ describe('Scope', () => {

test('clear', () => {
const scope = new Scope();
// @ts-expect-error accessing private property
const oldPropagationContext = scope._propagationContext;
scope.setExtra('a', 2);
scope.setTag('a', 'b');
scope.setUser({ id: '1' });
Expand All @@ -393,6 +422,14 @@ describe('Scope', () => {
scope.clear();
expect((scope as any)._extra).toEqual({});
expect((scope as any)._requestSession).toEqual(undefined);
// @ts-expect-error accessing private property
expect(scope._propagationContext).toEqual({
traceId: expect.any(String),
spanId: expect.any(String),
sampled: false,
});
// @ts-expect-error accessing private property
expect(scope._propagationContext).not.toEqual(oldPropagationContext);
});

test('clearBreadcrumbs', () => {
Expand Down Expand Up @@ -486,6 +523,8 @@ describe('Scope', () => {
expect(updatedScope._level).toEqual('warning');
expect(updatedScope._fingerprint).toEqual(['bar']);
expect(updatedScope._requestSession.status).toEqual('ok');
// @ts-ignore accessing private property for test
expect(updatedScope._propagationContext).toEqual(localScope._propagationContext);
});

test('given an empty instance of Scope, it should preserve all the original scope data', () => {
Expand Down Expand Up @@ -518,7 +557,13 @@ describe('Scope', () => {
tags: { bar: '3', baz: '4' },
user: { id: '42' },
requestSession: { status: 'errored' as RequestSessionStatus },
propagationContext: {
traceId: '8949daf83f4a4a70bee4c1eb9ab242ed',
spanId: 'a024ad8fea82680e',
sampled: true,
},
};

const updatedScope = scope.update(localAttributes) as any;

expect(updatedScope._tags).toEqual({
Expand All @@ -540,6 +585,11 @@ describe('Scope', () => {
expect(updatedScope._level).toEqual('warning');
expect(updatedScope._fingerprint).toEqual(['bar']);
expect(updatedScope._requestSession).toEqual({ status: 'errored' });
expect(updatedScope._propagationContext).toEqual({
traceId: '8949daf83f4a4a70bee4c1eb9ab242ed',
spanId: 'a024ad8fea82680e',
sampled: true,
});
});
});

Expand Down
11 changes: 2 additions & 9 deletions packages/node/test/async/domain.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getCurrentHub, Hub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core';
import * as domain from 'domain';
import type { Hub } from '@sentry/core';
import { getCurrentHub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core';

import { setDomainAsyncContextStrategy } from '../../src/async/domain';

Expand All @@ -9,13 +9,6 @@ describe('domains', () => {
setAsyncContextStrategy(undefined);
});

test('without domain', () => {
// @ts-ignore property active does not exist on domain
expect(domain.active).toBeFalsy();
const hub = getCurrentHub();
expect(hub).toEqual(new Hub());
});

test('hub scope inheritance', () => {
setDomainAsyncContextStrategy();

Expand Down
Loading
Loading