From f1ede5764d5595868c460186abb53df2d4a2fdcb Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 29 Jun 2023 11:08:53 -0400 Subject: [PATCH] feat(tracing): Add `PropagationContext` to scope (#8421) ref https://github.com/getsentry/sentry-javascript/issues/8352 For more details about PropagationContext, see https://www.notion.so/sentry/Tracing-without-performance-efab307eb7f64e71a04f09dc72722530 Building off of work in both https://github.com/getsentry/sentry-javascript/pull/8403 and https://github.com/getsentry/sentry-javascript/pull/8418, this PR adds `PropagationContext` and uses that to always set a trace context on outgoing error events. Currently if there is an active span on the scope, we automatically attach that span's trace context to all outgoing events. Now, we want to rely on either the active span or fallback to the propagation context to ensure that there is always a trace being generated and propagated. Next up we'll work on updating the node/browser SDKs to update the propagation context. For example, we should update the propagation context for node based on the incoming sentry-trace/baggage headers. --- .../suites/replay/captureReplay/test.ts | 2 - .../captureReplayFromReplayPackage/test.ts | 2 - .../utils/replayEventTemplates.ts | 1 - packages/core/src/baseclient.ts | 47 +++++++++++++++- packages/core/src/scope.ts | 36 ++++++++++++- packages/core/test/lib/base.test.ts | 22 ++++++++ packages/hub/test/scope.test.ts | 54 ++++++++++++++++++- packages/node/test/async/domain.test.ts | 11 +--- packages/node/test/async/hooks.test.ts | 8 +-- packages/replay/src/util/sendReplayRequest.ts | 6 +++ .../test/unit/util/prepareReplayEvent.test.ts | 2 +- packages/types/src/context.ts | 1 + packages/types/src/hub.ts | 3 ++ packages/types/src/index.ts | 2 +- packages/types/src/scope.ts | 7 +++ packages/types/src/tracing.ts | 10 ++++ 16 files changed, 188 insertions(+), 26 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/captureReplay/test.ts b/packages/browser-integration-tests/suites/replay/captureReplay/test.ts index 72cbc47efe99..421e725bb3e4 100644 --- a/packages/browser-integration-tests/suites/replay/captureReplay/test.ts +++ b/packages/browser-integration-tests/suites/replay/captureReplay/test.ts @@ -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: { @@ -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: { diff --git a/packages/browser-integration-tests/suites/replay/captureReplayFromReplayPackage/test.ts b/packages/browser-integration-tests/suites/replay/captureReplayFromReplayPackage/test.ts index 6caf1e4ea57c..c42bfc692018 100644 --- a/packages/browser-integration-tests/suites/replay/captureReplayFromReplayPackage/test.ts +++ b/packages/browser-integration-tests/suites/replay/captureReplayFromReplayPackage/test.ts @@ -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: { @@ -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: { diff --git a/packages/browser-integration-tests/utils/replayEventTemplates.ts b/packages/browser-integration-tests/utils/replayEventTemplates.ts index d88fbd1bf0e5..46208b39af00 100644 --- a/packages/browser-integration-tests/utils/replayEventTemplates.ts +++ b/packages/browser-integration-tests/utils/replayEventTemplates.ts @@ -30,7 +30,6 @@ const DEFAULT_REPLAY_EVENT = { version: SDK_VERSION, name: 'sentry.javascript.browser', }, - sdkProcessingMetadata: {}, request: { url: expect.stringContaining('/dist/index.html'), headers: { diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 9a1312a131ac..191feef7fd6a 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -15,6 +15,7 @@ import type { Integration, IntegrationClass, Outcome, + PropagationContext, SdkMetadata, Session, SessionAggregates, @@ -29,6 +30,7 @@ import { addItemToEnvelope, checkOrSetAlreadyCaught, createAttachmentEnvelopeItem, + dropUndefinedKeys, isPlainObject, isPrimitive, isThenable, @@ -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'; @@ -507,7 +510,49 @@ export abstract class BaseClient implements Client { 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; + }); } /** diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index 40a1fa135417..8d964f033739 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -11,6 +11,7 @@ import type { Extra, Extras, Primitive, + PropagationContext, RequestSession, Scope as ScopeInterface, ScopeContext, @@ -29,6 +30,7 @@ import { isThenable, logger, SyncPromise, + uuid4, } from '@sentry/utils'; import { updateSession } from './session'; @@ -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 @@ -108,6 +113,7 @@ export class Scope implements ScopeInterface { this._extra = {}; this._contexts = {}; this._sdkProcessingMetadata = {}; + this._propagationContext = generatePropagationContext(); } /** @@ -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; } @@ -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; @@ -365,6 +375,9 @@ export class Scope implements ScopeInterface { if (captureContext.requestSession) { this._requestSession = captureContext.requestSession; } + if (captureContext.propagationContext) { + this._propagationContext = captureContext.propagationContext; + } } return this; @@ -387,6 +400,7 @@ export class Scope implements ScopeInterface { this._session = undefined; this._notifyScopeListeners(); this._attachments = []; + this._propagationContext = generatePropagationContext(); return this; } @@ -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); } @@ -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. */ @@ -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, + }; +} diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index aca8784ad511..162b53e4bb51 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -492,6 +492,28 @@ describe('BaseClient', () => { ); }); + test('it adds a trace context all events', () => { + 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); diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index 6571cd3122b4..3b09c02a9f84 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -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(); @@ -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', () => { @@ -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, + }); }); }); @@ -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 => { @@ -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' }); @@ -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', () => { @@ -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', () => { @@ -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({ @@ -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, + }); }); }); diff --git a/packages/node/test/async/domain.test.ts b/packages/node/test/async/domain.test.ts index 84c3362b9882..8794a938ce50 100644 --- a/packages/node/test/async/domain.test.ts +++ b/packages/node/test/async/domain.test.ts @@ -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'; @@ -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(); diff --git a/packages/node/test/async/hooks.test.ts b/packages/node/test/async/hooks.test.ts index 8c4c5decab76..a08271230579 100644 --- a/packages/node/test/async/hooks.test.ts +++ b/packages/node/test/async/hooks.test.ts @@ -1,4 +1,5 @@ -import { getCurrentHub, Hub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core'; +import type { Hub } from '@sentry/core'; +import { getCurrentHub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core'; import { setHooksAsyncContextStrategy } from '../../src/async/hooks'; import { conditionalTest } from '../utils'; @@ -9,11 +10,6 @@ conditionalTest({ min: 12 })('async_hooks', () => { setAsyncContextStrategy(undefined); }); - test('without context', () => { - const hub = getCurrentHub(); - expect(hub).toEqual(new Hub()); - }); - test('without strategy hubs should be equal', () => { runWithAsyncContext(() => { const hub1 = getCurrentHub(); diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts index b6f49c0b9c9a..f0a01f0bf79a 100644 --- a/packages/replay/src/util/sendReplayRequest.ts +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -93,6 +93,12 @@ export async function sendReplayRequest({ } */ + // Prevent this data (which, if it exists, was used in earlier steps in the processing pipeline) from being sent to + // sentry. (Note: Our use of this property comes and goes with whatever we might be debugging, whatever hacks we may + // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid + // of this `delete`, lest we miss putting it back in the next time the property is in use.) + delete replayEvent.sdkProcessingMetadata; + const envelope = createReplayEnvelope(replayEvent, preparedRecordingData, dsn, client.getOptions().tunnel); let response: void | TransportMakeRequestResponse; diff --git a/packages/replay/test/unit/util/prepareReplayEvent.test.ts b/packages/replay/test/unit/util/prepareReplayEvent.test.ts index b0f121e1256a..5ada534f65a0 100644 --- a/packages/replay/test/unit/util/prepareReplayEvent.test.ts +++ b/packages/replay/test/unit/util/prepareReplayEvent.test.ts @@ -82,7 +82,7 @@ describe('Unit | util | prepareReplayEvent', () => { name: 'sentry.javascript.testSdk', version: '1.0.0', }, - sdkProcessingMetadata: {}, + sdkProcessingMetadata: expect.any(Object), breadcrumbs: undefined, }); }); diff --git a/packages/types/src/context.ts b/packages/types/src/context.ts index bab969899cea..052d6f4a6523 100644 --- a/packages/types/src/context.ts +++ b/packages/types/src/context.ts @@ -8,6 +8,7 @@ export interface Contexts extends Record { os?: OsContext; culture?: CultureContext; response?: ResponseContext; + trace?: TraceContext; } export interface AppContext extends Record { diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 35a3f9f4a82e..b7649cede039 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -70,6 +70,9 @@ export interface Hub { /** Returns the client of the top stack. */ getClient(): Client | undefined; + /** Returns the scope of the top stack */ + getScope(): Scope; + /** * Captures an exception event and sends it to Sentry. * diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index ccabae59a995..d9b55aeef077 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -83,7 +83,7 @@ export type { Span, SpanContext } from './span'; export type { StackFrame } from './stackframe'; export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace'; export type { TextEncoderInternal } from './textencoder'; -export type { TracePropagationTargets } from './tracing'; +export type { PropagationContext, TracePropagationTargets } from './tracing'; export type { CustomSamplingContext, SamplingContext, diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts index 4ed11b287421..1351d2ce3161 100644 --- a/packages/types/src/scope.ts +++ b/packages/types/src/scope.ts @@ -7,6 +7,7 @@ import type { Primitive } from './misc'; import type { RequestSession, Session } from './session'; import type { Severity, SeverityLevel } from './severity'; import type { Span } from './span'; +import type { PropagationContext } from './tracing'; import type { Transaction } from './transaction'; import type { User } from './user'; @@ -23,6 +24,7 @@ export interface ScopeContext { tags: { [key: string]: Primitive }; fingerprint: string[]; requestSession: RequestSession; + propagationContext: PropagationContext; } /** @@ -185,4 +187,9 @@ export interface Scope { * Add data which will be accessible during event processing but won't get sent to Sentry */ setSDKProcessingMetadata(newData: { [key: string]: unknown }): this; + + /** + * Add propagation context to the scope, used for distributed tracing + */ + setPropagationContext(context: PropagationContext): this; } diff --git a/packages/types/src/tracing.ts b/packages/types/src/tracing.ts index d11db382e2ed..11c4a1658d50 100644 --- a/packages/types/src/tracing.ts +++ b/packages/types/src/tracing.ts @@ -1 +1,11 @@ +import type { DynamicSamplingContext } from './envelope'; + export type TracePropagationTargets = (string | RegExp)[]; + +export interface PropagationContext { + traceId: string; + spanId: string; + sampled: boolean; + parentSpanId?: string; + dsc?: DynamicSamplingContext; +}