From 94a9e2eeb13514b877319d0c4b37ef2a5171716b Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 09:00:12 +0100 Subject: [PATCH 01/33] fix(apm): Set sampled to true by default Also set op otherwise span will be discarded by the server --- packages/apm/src/integrations/tracing.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 7f284e848d5f..aec7fb15d867 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -402,6 +402,8 @@ export class Tracing implements Integration { const span = hub.startSpan( { + op: 'operation', + sampled: true, ...spanContext, transaction: name, }, From a92c7307c983896a24875c9ef24e52c370ff8fec Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 09:04:30 +0100 Subject: [PATCH 02/33] meta: Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59d6e29c6c2e..b8e541790e6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +- [apm] fix: Set sampled and op by default (#2500) +- [apm] ref: Remove status from tags in transaction (#2497) +- [browser] fix: Respect breadcrumbs sentry:false option (#2499) + ## 5.14.2 - [apm] fix: Use Performance API for timings when available, including Web Workers (#2492) From 715bd45ad148a12128731c5f469975d3783abbef Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 10:39:48 +0100 Subject: [PATCH 03/33] fix(apm): Sampling --- CHANGELOG.md | 2 +- packages/apm/src/hubextensions.ts | 10 ++-- packages/apm/src/integrations/tracing.ts | 67 +++++------------------- packages/apm/test/hub.test.ts | 23 ++++++-- packages/types/src/options.ts | 9 +++- 5 files changed, 49 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8e541790e6c..8927bd34ad92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott -- [apm] fix: Set sampled and op by default (#2500) +- [apm] fix: Sampling of traces (#2500) - [apm] ref: Remove status from tags in transaction (#2497) - [browser] fix: Respect breadcrumbs sentry:false option (#2499) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 10a0cc30c770..85666c7bf16e 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -60,9 +60,13 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean span.sampled = Math.random() < sampleRate; } - if (span.sampled) { - const experimentsOptions = (client && client.getOptions()._experiments) || {}; - span.initFinishedSpans(experimentsOptions.maxSpans as number); + // We always want to record spans independent from the sample rate + const experimentsOptions = (client && client.getOptions()._experiments) || {}; + span.initFinishedSpans(experimentsOptions.maxSpans as number); + + // If we do not have an op by now by default we set a name otherwise the server will discard the transaction/span + if (span.op === undefined) { + span.op = 'op'; } return span; diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index aec7fb15d867..e308d6f448b7 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -54,7 +54,9 @@ interface TracingOptions { * Default: true */ startTransactionOnLocationChange: boolean; + /** + * @deprecated Use tracesSampleRate in the SDK options * Sample to determine if the Integration should instrument anything. The decision will be taken once per load * on initalization. * 0 = 0% chance of instrumenting @@ -62,7 +64,7 @@ interface TracingOptions { * * Default: 1 */ - tracesSampleRate: number; + tracesSampleRate?: number; /** * The maximum duration of a transaction before it will be discarded. This is for some edge cases where a browser @@ -109,11 +111,6 @@ export class Tracing implements Integration { */ public static id: string = 'Tracing'; - /** - * Is Tracing enabled, this will be determined once per pageload. - */ - private static _enabled?: boolean; - /** JSDoc */ public static options: TracingOptions; @@ -163,7 +160,6 @@ export class Tracing implements Integration { startTransactionOnLocationChange: true, traceFetch: true, traceXHR: true, - tracesSampleRate: 1, tracingOrigins: defaultTracingOrigins, }; // NOTE: Logger doesn't work in contructors, as it's initialized after integrations instances @@ -189,16 +185,11 @@ export class Tracing implements Integration { logger.warn(`[Tracing] We added a reasonable default for you: ${defaultTracingOrigins}`); } - if (!Tracing._isEnabled()) { - return; - } - // Starting our inital pageload transaction if (global.location && global.location.href) { // `${global.location.href}` will be used a temp transaction name Tracing.startIdleTransaction(global.location.href, { op: 'pageload', - sampled: true, }); } @@ -221,17 +212,15 @@ export class Tracing implements Integration { return event; } - if (Tracing._isEnabled()) { - const isOutdatedTransaction = - event.timestamp && - event.start_timestamp && - (event.timestamp - event.start_timestamp > Tracing.options.maxTransactionDuration || - event.timestamp - event.start_timestamp < 0); + const isOutdatedTransaction = + event.timestamp && + event.start_timestamp && + (event.timestamp - event.start_timestamp > Tracing.options.maxTransactionDuration || + event.timestamp - event.start_timestamp < 0); - if (Tracing.options.maxTransactionDuration !== 0 && event.type === 'transaction' && isOutdatedTransaction) { - logger.log('[Tracing] Discarded transaction since it maxed out maxTransactionDuration'); - return null; - } + if (Tracing.options.maxTransactionDuration !== 0 && event.type === 'transaction' && isOutdatedTransaction) { + logger.log('[Tracing] Discarded transaction since it maxed out maxTransactionDuration'); + return null; } return event; @@ -358,31 +347,10 @@ export class Tracing implements Integration { }); } - /** - * Is tracing enabled - */ - private static _isEnabled(): boolean { - if (Tracing._enabled !== undefined) { - return Tracing._enabled; - } - // This happens only in test cases where the integration isn't initalized properly - // tslint:disable-next-line: strict-type-predicates - if (!Tracing.options || typeof Tracing.options.tracesSampleRate !== 'number') { - return false; - } - Tracing._enabled = Math.random() > Tracing.options.tracesSampleRate ? false : true; - return Tracing._enabled; - } - /** * Starts a Transaction waiting for activity idle to finish */ public static startIdleTransaction(name: string, spanContext?: SpanContext): Span | undefined { - if (!Tracing._isEnabled()) { - // Tracing is not enabled - return undefined; - } - // If we already have an active transaction it means one of two things // a) The user did rapid navigation changes and didn't wait until the transaction was finished // b) A activity wasn't popped correctly and therefore the transaction is stalling @@ -402,8 +370,6 @@ export class Tracing implements Integration { const span = hub.startSpan( { - op: 'operation', - sampled: true, ...spanContext, transaction: name, }, @@ -643,10 +609,6 @@ export class Tracing implements Integration { autoPopAfter?: number; }, ): number { - if (!Tracing._isEnabled()) { - // Tracing is not enabled - return 0; - } if (!Tracing._activeTransaction) { logger.log(`[Tracing] Not pushing activity ${name} since there is no active transaction`); return 0; @@ -691,10 +653,8 @@ export class Tracing implements Integration { */ public static popActivity(id: number, spanData?: { [key: string]: any }): void { // The !id is on purpose to also fail with 0 - // Since 0 is returned by push activity in case tracing is not enabled - // or there is no active transaction - if (!Tracing._isEnabled() || !id) { - // Tracing is not enabled + // Since 0 is returned by push activity in case there is no active transaction + if (!id) { return; } @@ -841,7 +801,6 @@ function historyCallback(_: { [key: string]: any }): void { if (Tracing.options.startTransactionOnLocationChange && global && global.location) { Tracing.startIdleTransaction(global.location.href, { op: 'navigation', - sampled: true, }); } } diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index d6873ec2f53d..393ef18f29af 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -1,9 +1,9 @@ +import { BrowserClient } from '@sentry/browser'; import { Hub, Scope } from '@sentry/hub'; import { addExtensionMethods } from '../src/hubextensions'; addExtensionMethods(); -const clientFn: any = jest.fn(); describe('Hub', () => { afterEach(() => { @@ -12,16 +12,33 @@ describe('Hub', () => { }); describe('spans', () => { + describe('sampling', () => { + test('set tracesSampleRate 0', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); + const span = hub.startSpan() as any; + expect(span.sampled).toBeUndefined(); + }); + test('set tracesSampleRate 0 on transaction', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); + const span = hub.startSpan({ transaction: 'foo' }) as any; + expect(span.sampled).toBe(false); + }); + test('set tracesSampleRate 1', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const span = hub.startSpan({ transaction: 'foo' }) as any; + expect(span.sampled).toBeTruthy(); + }); + }); describe('start', () => { test('simple', () => { - const hub = new Hub(clientFn); + const hub = new Hub(new BrowserClient()); const span = hub.startSpan() as any; expect(span._spanId).toBeTruthy(); }); test('inherits from parent span', () => { const myScope = new Scope(); - const hub = new Hub(clientFn, myScope); + const hub = new Hub(new BrowserClient(), myScope); const parentSpan = hub.startSpan({}) as any; expect(parentSpan._parentId).toBeFalsy(); hub.configureScope(scope => { diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 103daf452a21..21cc7ce6f67b 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -78,7 +78,14 @@ export interface Options { /** A global sample rate to apply to all events (0 - 1). */ sampleRate?: number; - /** A global sample rate to apply to all transactions (0 - 1). */ + /** + * Sample rate to determine transaction/span sampling. + * + * 0 = 0% chance of instrumenting + * 1 = 100% change of instrumenting + * + * Default: 1 + */ tracesSampleRate?: number; /** Attaches stacktraces to pure capture message / log integrations */ From c82f1d89473b158999142414583af747829355b9 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 10:55:57 +0100 Subject: [PATCH 04/33] ref: Remove set default op --- packages/apm/src/hubextensions.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 85666c7bf16e..9ec887a045b0 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -64,11 +64,6 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean const experimentsOptions = (client && client.getOptions()._experiments) || {}; span.initFinishedSpans(experimentsOptions.maxSpans as number); - // If we do not have an op by now by default we set a name otherwise the server will discard the transaction/span - if (span.op === undefined) { - span.op = 'op'; - } - return span; } From 7c8d8030e51eae343d3bc83ef0faaf6d1d4a51ba Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:03:09 +0100 Subject: [PATCH 05/33] fix: Sampling decision --- packages/apm/src/span.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 0621fb5949da..044ec4372e07 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -287,11 +287,8 @@ export class Span implements SpanInterface, SpanContext { return undefined; } - if (this.sampled === undefined) { - // At this point a `sampled === undefined` should have already been - // resolved to a concrete decision. If `sampled` is `undefined`, it's - // likely that somebody used `Sentry.startSpan(...)` on a - // non-transaction span and later decided to make it a transaction. + if (this.sampled !== true) { + // At this point if `sampled !== true` we want to discard the transaction. logger.warn('Discarding transaction Span without sampling decision'); return undefined; } From 895c188ed611b96498dec810ae88d99e67b03e59 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:23:40 +0100 Subject: [PATCH 06/33] ref: Add comment --- packages/apm/src/hubextensions.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 9ec887a045b0..a21e61e59dfb 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -60,7 +60,9 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean span.sampled = Math.random() < sampleRate; } - // We always want to record spans independent from the sample rate + // We always want to record spans independent from the sample rate. + // The Span tree should be built regardless and the top level sample rate shouldn't determine if we create + // child spans. const experimentsOptions = (client && client.getOptions()._experiments) || {}; span.initFinishedSpans(experimentsOptions.maxSpans as number); From b0870b6c6e21d272b1350d42d0dbbb3718399137 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:24:14 +0100 Subject: [PATCH 07/33] ref: typo --- packages/apm/src/integrations/tracing.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index e308d6f448b7..56fb8d7731e6 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -59,7 +59,7 @@ interface TracingOptions { * @deprecated Use tracesSampleRate in the SDK options * Sample to determine if the Integration should instrument anything. The decision will be taken once per load * on initalization. - * 0 = 0% chance of instrumenting + * 0 = 0% change of instrumenting * 1 = 100% change of instrumenting * * Default: 1 From a5b5f2b7eb5257722296a557479f6d01e3cdc959 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:26:37 +0100 Subject: [PATCH 08/33] ref: Changes to docblock --- packages/apm/src/integrations/tracing.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 56fb8d7731e6..c4c92757aaba 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -57,12 +57,7 @@ interface TracingOptions { /** * @deprecated Use tracesSampleRate in the SDK options - * Sample to determine if the Integration should instrument anything. The decision will be taken once per load - * on initalization. - * 0 = 0% change of instrumenting - * 1 = 100% change of instrumenting - * - * Default: 1 + * This is a noop */ tracesSampleRate?: number; From 2c662dc023ce4740db942aa540d41aa03861259b Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:33:50 +0100 Subject: [PATCH 09/33] ref: Change message --- packages/apm/src/span.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 044ec4372e07..584d1035175c 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -289,7 +289,7 @@ export class Span implements SpanInterface, SpanContext { if (this.sampled !== true) { // At this point if `sampled !== true` we want to discard the transaction. - logger.warn('Discarding transaction Span without sampling decision'); + logger.warn('Discarding transaction Span because it was sampled == false || undefined'); return undefined; } From b96e5127b3de50c453be0d13338b897a45089a57 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:39:55 +0100 Subject: [PATCH 10/33] Apply suggestions from code review Co-Authored-By: Rodolfo Carvalho --- packages/types/src/options.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 21cc7ce6f67b..766cda05fc9d 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -79,7 +79,7 @@ export interface Options { sampleRate?: number; /** - * Sample rate to determine transaction/span sampling. + * Sample rate to determine trace sampling. * * 0 = 0% chance of instrumenting * 1 = 100% change of instrumenting From 4139da7d37c5c07d1af53045a9af21bf3f0cf4f1 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:40:19 +0100 Subject: [PATCH 11/33] Update packages/types/src/options.ts Co-Authored-By: Rodolfo Carvalho --- packages/types/src/options.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 766cda05fc9d..6411332db9d1 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -84,7 +84,7 @@ export interface Options { * 0 = 0% chance of instrumenting * 1 = 100% change of instrumenting * - * Default: 1 + * Default: 0.0 */ tracesSampleRate?: number; From 5a66e0b367d0b6442387d53182b13658ed635b74 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:40:28 +0100 Subject: [PATCH 12/33] Update packages/types/src/options.ts Co-Authored-By: Rodolfo Carvalho --- packages/types/src/options.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 6411332db9d1..fd23a2904582 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -81,8 +81,8 @@ export interface Options { /** * Sample rate to determine trace sampling. * - * 0 = 0% chance of instrumenting - * 1 = 100% change of instrumenting + * 0.0 = 0% chance of instrumenting + * 1.0 = 100% chance of instrumenting * * Default: 0.0 */ From 450259eba8bacea00529b1944e72d6c6928a41f5 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:47:01 +0100 Subject: [PATCH 13/33] ref: Remove deprecated parts --- packages/apm/src/integrations/tracing.ts | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index c4c92757aaba..482ec79329d8 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -55,12 +55,6 @@ interface TracingOptions { */ startTransactionOnLocationChange: boolean; - /** - * @deprecated Use tracesSampleRate in the SDK options - * This is a noop - */ - tracesSampleRate?: number; - /** * The maximum duration of a transaction before it will be discarded. This is for some edge cases where a browser * completely freezes the JS state and picks it up later (background tabs). @@ -389,23 +383,6 @@ export class Tracing implements Integration { return span; } - /** - * Update transaction - * @deprecated - */ - public static updateTransactionName(name: string): void { - logger.log('[Tracing] DEPRECATED, use Sentry.configureScope => scope.setTransaction instead', name); - const _getCurrentHub = Tracing._getCurrentHub; - if (_getCurrentHub) { - const hub = _getCurrentHub(); - if (hub) { - hub.configureScope(scope => { - scope.setTransaction(name); - }); - } - } - } - /** * Finshes the current active transaction */ From 86ce10f62b41e90ffb2566a1982f884471837fd8 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 11:59:36 +0100 Subject: [PATCH 14/33] fix: Maxlen --- packages/apm/src/span.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 584d1035175c..8775b2c839dd 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -21,8 +21,9 @@ class SpanRecorder { private _openSpanCount: number = 0; public finishedSpans: Span[] = []; - public constructor(maxlen: number) { - this._maxlen = maxlen; + public constructor(maxlen?: number) { + // tslint:disable-next-line: strict-type-predicates + this._maxlen = typeof maxlen !== 'number' ? 1000 : maxlen; } /** From a1b249676574b163d8c7950f8e1f606e2d5f7991 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 13:53:11 +0100 Subject: [PATCH 15/33] ref: Rework when and how integrations are setup --- packages/core/src/baseclient.ts | 14 ++++++-------- packages/core/src/sdk.ts | 5 ++++- packages/core/test/lib/base.test.ts | 6 +++--- packages/types/src/client.ts | 3 +++ 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 5b48161a52ef..3904807e002d 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -52,7 +52,7 @@ export abstract class BaseClient implement protected readonly _dsn?: Dsn; /** Array of used integrations. */ - protected readonly _integrations: IntegrationIndex = {}; + protected _integrations: IntegrationIndex = {}; /** Is the client still processing a call? */ protected _processing: boolean = false; @@ -70,10 +70,6 @@ export abstract class BaseClient implement if (options.dsn) { this._dsn = new Dsn(options.dsn); } - - if (this._isEnabled()) { - this._integrations = setupIntegrations(this._options); - } } /** @@ -185,10 +181,12 @@ export abstract class BaseClient implement } /** - * @inheritDoc + * Sets up the integrations */ - public getIntegrations(): IntegrationIndex { - return this._integrations || {}; + public setupIntegrations(): void { + if (this._isEnabled()) { + this._integrations = setupIntegrations(this._options); + } } /** diff --git a/packages/core/src/sdk.ts b/packages/core/src/sdk.ts index 10137fa92066..d1008357fb4f 100644 --- a/packages/core/src/sdk.ts +++ b/packages/core/src/sdk.ts @@ -16,5 +16,8 @@ export function initAndBind(clientClass: Cl if (options.debug === true) { logger.enable(); } - getCurrentHub().bindClient(new clientClass(options)); + const hub = getCurrentHub(); + const client = new clientClass(options); + hub.bindClient(client); + client.setupIntegrations(); } diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index efac6e2fcf7e..bfb208dac277 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -571,7 +571,7 @@ describe('BaseClient', () => { dsn: PUBLIC_DSN, integrations: [new TestIntegration()], }); - expect(Object.keys(client.getIntegrations()).length).toBe(1); + expect(Object.keys((client as any)._integrations).length).toBe(1); expect(client.getIntegration(TestIntegration)).toBeTruthy(); }); @@ -580,7 +580,7 @@ describe('BaseClient', () => { const client = new TestClient({ integrations: [new TestIntegration()], }); - expect(Object.keys(client.getIntegrations()).length).toBe(0); + expect(Object.keys((client as any)._integrations).length).toBe(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); @@ -591,7 +591,7 @@ describe('BaseClient', () => { enabled: false, integrations: [new TestIntegration()], }); - expect(Object.keys(client.getIntegrations()).length).toBe(0); + expect(Object.keys((client as any)._integrations).length).toBe(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); }); diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index 90985723f6e6..c84d2a1a9dab 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -70,4 +70,7 @@ export interface Client { /** Returns an array of installed integrations on the client. */ getIntegration(integartion: IntegrationClass): T | null; + + /** This is an internal function to setup all integrations that should run on the client */ + setupIntegrations(): void; } From ea581e7e750f2a27b457582e956c7e4cc53dd11d Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 13:54:52 +0100 Subject: [PATCH 16/33] ref(apm): Send a span if it's not a child --- packages/apm/src/hubextensions.ts | 8 ++++++-- packages/apm/src/span.ts | 18 ++++++++++++------ packages/types/src/span.ts | 5 +++++ 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index a21e61e59dfb..18510d257fa6 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -45,7 +45,10 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean if (!isSpanInstance(spanOrSpanContext) && !forceNoChild) { if (scope) { const parentSpan = scope.getSpan() as Span; - if (parentSpan) { + // If we have a span on the scope, it means we want to create a child on this span + // but only if there is no timestamp set. This means that this span has already be finished. + // And it's not valid that you set another span as a child. + if (parentSpan && parentSpan.timestamp === undefined) { span = parentSpan.child(spanOrSpanContext); } } @@ -55,7 +58,8 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean span = new Span(spanOrSpanContext, that); } - if (span.sampled === undefined && span.transaction !== undefined) { + // We only roll the dice on sampling for "root" spans (transactions) because the childs inherit this state + if (span.sampled === undefined && !span.isChildSpan()) { const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; span.sampled = Math.random() < sampleRate; } diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 8775b2c839dd..87b779b5c085 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -192,6 +192,13 @@ export class Span implements SpanInterface, SpanContext { return span; } + /** + * @inheritDoc + */ + public isChildSpan(): boolean { + return this._parentSpanId !== undefined; + } + /** * Continues a trace from a string (usually the header). * @param traceparent Traceparent string @@ -282,12 +289,6 @@ export class Span implements SpanInterface, SpanContext { this.spanRecorder.finishSpan(this); - if (this.transaction === undefined) { - // If this has no transaction set we assume there's a parent - // transaction for this span that would be flushed out eventually. - return undefined; - } - if (this.sampled !== true) { // At this point if `sampled !== true` we want to discard the transaction. logger.warn('Discarding transaction Span because it was sampled == false || undefined'); @@ -305,6 +306,11 @@ export class Span implements SpanInterface, SpanContext { }).timestamp; } + // We will not send any child spans + if (this.isChildSpan()) { + return undefined; + } + return this._hub.captureEvent({ contexts: { trace: this.getTraceContext(), diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 4bb18a38fb74..7cacd8f8c146 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -39,6 +39,11 @@ export interface Span { * Determines whether span was successful (HTTP200) */ isSuccess(): boolean; + + /** + * Determines if the span is a child of another span + */ + isChildSpan(): boolean; } /** Interface holder all properties that can be set on a Span on creation. */ From 358357890cebf75208dd895809a53bcb1312151d Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 13:55:36 +0100 Subject: [PATCH 17/33] ref: Tracing integration --- packages/apm/src/integrations/tracing.ts | 24 ++++++++++++++---------- packages/node/src/integrations/http.ts | 2 +- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 482ec79329d8..eb222f9df3f0 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -108,7 +108,7 @@ export class Tracing implements Integration { */ private static _getCurrentHub?: () => Hub; - private static _activeTransaction?: Span; + private static _activeTransaction?: SpanClass; private static _currentIndex: number = 1; @@ -323,7 +323,7 @@ export class Tracing implements Integration { * If an error or unhandled promise occurs, we mark the active transaction as failed */ logger.log(`[Tracing] Global error occured, setting status in transaction: ${SpanStatus.InternalError}`); - (Tracing._activeTransaction as SpanClass).setStatus(SpanStatus.InternalError); + Tracing._activeTransaction.setStatus(SpanStatus.InternalError); } } addInstrumentationHandler({ @@ -357,21 +357,19 @@ export class Tracing implements Integration { return undefined; } - const span = hub.startSpan( + Tracing._activeTransaction = hub.startSpan( { ...spanContext, transaction: name, }, true, - ); - - Tracing._activeTransaction = span; + ) as SpanClass; // We need to do this workaround here and not use configureScope // Reason being at the time we start the inital transaction we do not have a client bound on the hub yet // therefore configureScope wouldn't be executed and we would miss setting the transaction // tslint:disable-next-line: no-unsafe-any - (hub as any).getScope().setSpan(span); + (hub as any).getScope().setSpan(Tracing._activeTransaction); // The reason we do this here is because of cached responses // If we start and transaction without an activity it would never finish since there is no activity @@ -380,7 +378,7 @@ export class Tracing implements Integration { Tracing.popActivity(id); }, (Tracing.options && Tracing.options.idleTimeout) || 100); - return span; + return Tracing._activeTransaction; } /** @@ -589,11 +587,13 @@ export class Tracing implements Integration { // We want to clear the timeout also here since we push a new activity clearTimeout(Tracing._debounce); + const activeTransaction = Tracing._activeTransaction; + const _getCurrentHub = Tracing._getCurrentHub; if (spanContext && _getCurrentHub) { const hub = _getCurrentHub(); if (hub) { - const span = hub.startSpan(spanContext); + const span = activeTransaction.child(spanContext); Tracing._activities[Tracing._currentIndex] = { name, span, @@ -712,7 +712,11 @@ function xhrCallback(handlerData: { [key: string]: any }): void { if (activity) { const span = activity.span; if (span && handlerData.xhr.setRequestHeader) { - handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent()); + try { + handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent()); + } catch (_) { + // Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED. + } } } // tslint:enable: no-unsafe-any diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 652d443c8356..63b0a7ccb6ca 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -81,7 +81,7 @@ function createHandlerWrapper( let span: Span; if (tracingEnabled) { span = getCurrentHub().startSpan({ - description: `${typeof options === 'string' || !options.method ? 'GET' : options.method}|${requestUrl}`, + description: `${typeof options === 'string' || !options.method ? 'GET' : options.method} ${requestUrl}`, op: 'request', }); } From 2bbd2e7f8ba4df3a3f483145ae9f52c76f8a4140 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 14:30:55 +0100 Subject: [PATCH 18/33] fix: tests --- packages/core/test/lib/base.test.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index bfb208dac277..9fdbbd3f59e3 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -8,6 +8,7 @@ import { TestIntegration } from '../mocks/integration'; import { FakeTransport } from '../mocks/transport'; const PUBLIC_DSN = 'https://username@domain/path'; +declare var global: any; jest.mock('@sentry/utils', () => { const original = jest.requireActual('@sentry/utils'); @@ -565,12 +566,17 @@ describe('BaseClient', () => { }); describe('integrations', () => { - test('setup each one of them on ctor', () => { + beforeEach(() => { + global.__SENTRY__ = {}; + }); + + test('setup each one of them on setupIntegration call', () => { expect.assertions(2); const client = new TestClient({ dsn: PUBLIC_DSN, integrations: [new TestIntegration()], }); + client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toBe(1); expect(client.getIntegration(TestIntegration)).toBeTruthy(); }); @@ -580,6 +586,7 @@ describe('BaseClient', () => { const client = new TestClient({ integrations: [new TestIntegration()], }); + client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toBe(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); @@ -591,6 +598,7 @@ describe('BaseClient', () => { enabled: false, integrations: [new TestIntegration()], }); + client.setupIntegrations(); expect(Object.keys((client as any)._integrations).length).toBe(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); From c3f2750588fc2fc5f5f199af84d355d326f31ee8 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 15:14:47 +0100 Subject: [PATCH 19/33] fix: Span / Transaction creation --- packages/apm/src/hubextensions.ts | 10 ++++------ packages/apm/src/integrations/tracing.ts | 12 +++--------- packages/apm/src/span.ts | 3 +-- packages/types/src/span.ts | 6 ++++++ 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 18510d257fa6..c6e6a1596027 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -34,21 +34,19 @@ function traceHeaders(): { [key: string]: string } { * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * * @param span Already constructed span which should be started or properties with which the span should be created + * @param makeRoot This will just create the span as it is and will not attach it to the span on the scope (if there is one) */ -function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean = false): Span { +function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = false): Span { // @ts-ignore const that = this as Hub; const scope = that.getScope(); const client = that.getClient(); let span; - if (!isSpanInstance(spanOrSpanContext) && !forceNoChild) { + if (!isSpanInstance(spanOrSpanContext) && !makeRoot) { if (scope) { const parentSpan = scope.getSpan() as Span; - // If we have a span on the scope, it means we want to create a child on this span - // but only if there is no timestamp set. This means that this span has already be finished. - // And it's not valid that you set another span as a child. - if (parentSpan && parentSpan.timestamp === undefined) { + if (parentSpan) { span = parentSpan.child(spanOrSpanContext); } } diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index eb222f9df3f0..b7751611a67b 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -108,7 +108,7 @@ export class Tracing implements Integration { */ private static _getCurrentHub?: () => Hub; - private static _activeTransaction?: SpanClass; + private static _activeTransaction?: Span; private static _currentIndex: number = 1; @@ -363,13 +363,7 @@ export class Tracing implements Integration { transaction: name, }, true, - ) as SpanClass; - - // We need to do this workaround here and not use configureScope - // Reason being at the time we start the inital transaction we do not have a client bound on the hub yet - // therefore configureScope wouldn't be executed and we would miss setting the transaction - // tslint:disable-next-line: no-unsafe-any - (hub as any).getScope().setSpan(Tracing._activeTransaction); + ); // The reason we do this here is because of cached responses // If we start and transaction without an activity it would never finish since there is no activity @@ -634,7 +628,7 @@ export class Tracing implements Integration { if (activity) { logger.log(`[Tracing] popActivity ${activity.name}#${id}`); - const span = activity.span as SpanClass; + const span = activity.span as Span; if (span) { if (spanData) { Object.keys(spanData).forEach((key: string) => { diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 87b779b5c085..54b497b8cd75 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -176,8 +176,7 @@ export class Span implements SpanInterface, SpanContext { } /** - * Creates a new `Span` while setting the current `Span.id` as `parentSpanId`. - * Also the `sampled` decision will be inherited. + * @inheritDoc */ public child(spanContext?: Pick>): Span { const span = new Span({ diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 7cacd8f8c146..fbb0320d0f84 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -35,6 +35,12 @@ export interface Span { */ setHttpStatus(httpStatus: number): this; + /** + * Creates a new `Span` while setting the current `Span.id` as `parentSpanId`. + * Also the `sampled` decision will be inherited. + */ + child(spanContext?: Pick>): Span; + /** * Determines whether span was successful (HTTP200) */ From de8fefabcda05b0ae5d2d5d751c18fae5a9f0184 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 15:19:26 +0100 Subject: [PATCH 20/33] ref: CodeReview --- packages/apm/src/hubextensions.ts | 2 +- packages/apm/src/span.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index c6e6a1596027..039715935d65 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -33,7 +33,7 @@ function traceHeaders(): { [key: string]: string } { * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope, * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * - * @param span Already constructed span which should be started or properties with which the span should be created + * @param spanOrSpanContext Already constructed span which should be started or properties with which the span should be created * @param makeRoot This will just create the span as it is and will not attach it to the span on the scope (if there is one) */ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = false): Span { diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 54b497b8cd75..6aca95cf7c54 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -290,7 +290,7 @@ export class Span implements SpanInterface, SpanContext { if (this.sampled !== true) { // At this point if `sampled !== true` we want to discard the transaction. - logger.warn('Discarding transaction Span because it was sampled == false || undefined'); + logger.warn('Discarding transaction Span because it was span.sampled !== true'); return undefined; } From 29f392eab0d81381978f5ccdee340423bcdc9499 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 16:02:54 +0100 Subject: [PATCH 21/33] fix: Setup integrations when after we bound a client to the hub --- packages/core/src/sdk.ts | 1 - packages/hub/src/hub.ts | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/src/sdk.ts b/packages/core/src/sdk.ts index d1008357fb4f..a6cc609db7ac 100644 --- a/packages/core/src/sdk.ts +++ b/packages/core/src/sdk.ts @@ -19,5 +19,4 @@ export function initAndBind(clientClass: Cl const hub = getCurrentHub(); const client = new clientClass(options); hub.bindClient(client); - client.setupIntegrations(); } diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 6d297efe64c6..bd193bb57be2 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -105,6 +105,9 @@ export class Hub implements HubInterface { public bindClient(client?: Client): void { const top = this.getStackTop(); top.client = client; + if (client) { + client.setupIntegrations(); + } } /** From 878a8fc299cce05e45ba3bdb51e9360d2257c98d Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 17:18:59 +0100 Subject: [PATCH 22/33] ref: CodeReview --- packages/apm/src/hubextensions.ts | 23 +++++++++++------------ packages/apm/src/span.ts | 11 ++++++----- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 039715935d65..12205f68f9e8 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -33,27 +33,26 @@ function traceHeaders(): { [key: string]: string } { * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope, * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * - * @param spanOrSpanContext Already constructed span which should be started or properties with which the span should be created - * @param makeRoot This will just create the span as it is and will not attach it to the span on the scope (if there is one) + * @param spanOrSpanContext Already constructed span or properties with which the span should be created + * @param makeRoot This will just create the span as it is and will not attach it to the span on the scope (if there is one). + * Under some circumstances, in internal integrations, for example, this is used to make sure they are not interfering with each other. */ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = false): Span { // @ts-ignore - const that = this as Hub; - const scope = that.getScope(); - const client = that.getClient(); + const hub = this as Hub; + const scope = hub.getScope(); + const client = hub.getClient(); let span; - if (!isSpanInstance(spanOrSpanContext) && !makeRoot) { - if (scope) { - const parentSpan = scope.getSpan() as Span; - if (parentSpan) { - span = parentSpan.child(spanOrSpanContext); - } + if (!isSpanInstance(spanOrSpanContext) && !makeRoot && scope) { + const parentSpan = scope.getSpan() as Span; + if (parentSpan) { + span = parentSpan.child(spanOrSpanContext); } } if (!isSpanInstance(span)) { - span = new Span(spanOrSpanContext, that); + span = new Span(spanOrSpanContext, hub); } // We only roll the dice on sampling for "root" spans (transactions) because the childs inherit this state diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 6aca95cf7c54..751c465ac5ec 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -21,9 +21,8 @@ class SpanRecorder { private _openSpanCount: number = 0; public finishedSpans: Span[] = []; - public constructor(maxlen?: number) { - // tslint:disable-next-line: strict-type-predicates - this._maxlen = typeof maxlen !== 'number' ? 1000 : maxlen; + public constructor(maxlen: number) { + this._maxlen = maxlen; } /** @@ -178,7 +177,9 @@ export class Span implements SpanInterface, SpanContext { /** * @inheritDoc */ - public child(spanContext?: Pick>): Span { + public child( + spanContext?: Pick>, + ): Span { const span = new Span({ ...spanContext, parentSpanId: this._spanId, @@ -204,7 +205,7 @@ export class Span implements SpanInterface, SpanContext { */ public static fromTraceparent( traceparent: string, - spanContext?: Pick>, + spanContext?: Pick>, ): Span | undefined { const matches = traceparent.match(TRACEPARENT_REGEXP); if (matches) { From a2733e6d0adc0daa70d3c043a485da1b0798d354 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 17:28:38 +0100 Subject: [PATCH 23/33] fix: tests --- packages/hub/src/hub.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index bd193bb57be2..5886c3707e82 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -105,7 +105,7 @@ export class Hub implements HubInterface { public bindClient(client?: Client): void { const top = this.getStackTop(); top.client = client; - if (client) { + if (client && client.setupIntegrations) { client.setupIntegrations(); } } From 74a98945d359991f85f9397cacf370982b623bcb Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 18:26:33 +0100 Subject: [PATCH 24/33] Update packages/types/src/span.ts Co-Authored-By: Rodolfo Carvalho --- packages/types/src/span.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index fbb0320d0f84..71dc91801368 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -39,7 +39,7 @@ export interface Span { * Creates a new `Span` while setting the current `Span.id` as `parentSpanId`. * Also the `sampled` decision will be inherited. */ - child(spanContext?: Pick>): Span; + child(spanContext?: Pick>): Span; /** * Determines whether span was successful (HTTP200) From 4332b357d2c8f805d03893cbce687d4bb5358d04 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 18:27:16 +0100 Subject: [PATCH 25/33] ref: CodeReview --- packages/apm/src/span.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 751c465ac5ec..cfd7adab95eb 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -178,7 +178,7 @@ export class Span implements SpanInterface, SpanContext { * @inheritDoc */ public child( - spanContext?: Pick>, + spanContext?: Pick>, ): Span { const span = new Span({ ...spanContext, @@ -205,7 +205,7 @@ export class Span implements SpanInterface, SpanContext { */ public static fromTraceparent( traceparent: string, - spanContext?: Pick>, + spanContext?: Pick>, ): Span | undefined { const matches = traceparent.match(TRACEPARENT_REGEXP); if (matches) { From 7a114f5c40f4ddca018bd638e4826be6756ae8b6 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 18:29:32 +0100 Subject: [PATCH 26/33] ref: CodeReview --- packages/apm/src/integrations/tracing.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index b7751611a67b..284954993f3d 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -573,7 +573,9 @@ export class Tracing implements Integration { autoPopAfter?: number; }, ): number { - if (!Tracing._activeTransaction) { + const activeTransaction = Tracing._activeTransaction; + + if (!activeTransaction) { logger.log(`[Tracing] Not pushing activity ${name} since there is no active transaction`); return 0; } @@ -581,8 +583,6 @@ export class Tracing implements Integration { // We want to clear the timeout also here since we push a new activity clearTimeout(Tracing._debounce); - const activeTransaction = Tracing._activeTransaction; - const _getCurrentHub = Tracing._getCurrentHub; if (spanContext && _getCurrentHub) { const hub = _getCurrentHub(); @@ -628,7 +628,7 @@ export class Tracing implements Integration { if (activity) { logger.log(`[Tracing] popActivity ${activity.name}#${id}`); - const span = activity.span as Span; + const span = activity.span; if (span) { if (spanData) { Object.keys(spanData).forEach((key: string) => { From a695cd9af851be946801daea163f8ed72b02ed86 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 18 Mar 2020 19:04:37 +0100 Subject: [PATCH 27/33] fix: Tests --- packages/core/test/lib/sdk.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/core/test/lib/sdk.test.ts b/packages/core/test/lib/sdk.test.ts index efa3891dbd68..e16e9b10a29b 100644 --- a/packages/core/test/lib/sdk.test.ts +++ b/packages/core/test/lib/sdk.test.ts @@ -10,14 +10,15 @@ const PUBLIC_DSN = 'https://username@domain/path'; jest.mock('@sentry/hub', () => ({ getCurrentHub(): { - bindClient(): boolean; + bindClient(client: any): boolean; getClient(): boolean; } { return { getClient(): boolean { return false; }, - bindClient(): boolean { + bindClient(client: any): boolean { + client.setupIntegrations(); return true; }, }; From 498e3fff06367bd907709e452fc3613b47a6c1d5 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 19 Mar 2020 10:15:55 +0100 Subject: [PATCH 28/33] ref: Refactor SpanRecorder -> SpanList Fix tests --- packages/apm/src/hubextensions.ts | 13 +- packages/apm/src/integrations/tracing.ts | 8 +- packages/apm/src/span.ts | 38 +++--- packages/apm/test/hub.test.ts | 12 +- packages/apm/test/span.test.ts | 145 +++++++++++++++++------ packages/types/src/hub.ts | 2 +- 6 files changed, 151 insertions(+), 67 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 12205f68f9e8..eb25834247b4 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -30,7 +30,7 @@ function traceHeaders(): { [key: string]: string } { /** * This functions starts a span. If argument passed is of type `Span`, it'll run sampling on it if configured - * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope, + * and attach a `SpanList`. If it's of type `SpanContext` and there is already a `Span` on the Scope, * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * * @param spanOrSpanContext Already constructed span or properties with which the span should be created @@ -61,11 +61,12 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = f span.sampled = Math.random() < sampleRate; } - // We always want to record spans independent from the sample rate. - // The Span tree should be built regardless and the top level sample rate shouldn't determine if we create - // child spans. - const experimentsOptions = (client && client.getOptions()._experiments) || {}; - span.initFinishedSpans(experimentsOptions.maxSpans as number); + // We only want to create a span list if we sampled the transaction + // in case we will discard the span anyway because sampled == false, we safe memory and do not store child spans + if (span.sampled) { + const experimentsOptions = (client && client.getOptions()._experiments) || {}; + span.initSpanList(experimentsOptions.maxSpans as number); + } return span; } diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 284954993f3d..0210e973d782 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -407,8 +407,8 @@ export class Tracing implements Integration { // tslint:disable-next-line: completed-docs function addSpan(span: SpanClass): void { - if (transactionSpan.spanRecorder) { - transactionSpan.spanRecorder.finishSpan(span); + if (transactionSpan.spanList) { + transactionSpan.spanList.finishSpan(span); } } @@ -498,8 +498,8 @@ export class Tracing implements Integration { const resourceName = entry.name.replace(window.location.origin, ''); if (entry.initiatorType === 'xmlhttprequest' || entry.initiatorType === 'fetch') { // We need to update existing spans with new timing info - if (transactionSpan.spanRecorder) { - transactionSpan.spanRecorder.finishedSpans.map((finishedSpan: SpanClass) => { + if (transactionSpan.spanList) { + transactionSpan.spanList.finishedSpans.map((finishedSpan: SpanClass) => { if (finishedSpan.description && finishedSpan.description.indexOf(resourceName) !== -1) { finishedSpan.startTimestamp = timeOrigin + startTime; finishedSpan.timestamp = finishedSpan.startTimestamp + duration; diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index cfd7adab95eb..ee01a9f58b26 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -16,12 +16,12 @@ export const TRACEPARENT_REGEXP = new RegExp( /** * Keeps track of finished spans for a given transaction */ -class SpanRecorder { +class SpanList { private readonly _maxlen: number; private _openSpanCount: number = 0; public finishedSpans: Span[] = []; - public constructor(maxlen: number) { + public constructor(maxlen: number = 1000) { this._maxlen = maxlen; } @@ -31,11 +31,11 @@ class SpanRecorder { * trace tree (i.e.the first n spans with the smallest * start_timestamp). */ - public startSpan(span: Span): void { - this._openSpanCount += 1; + public add(span: Span): void { if (this._openSpanCount > this._maxlen) { - span.spanRecorder = undefined; + span.spanList = undefined; } + this._openSpanCount++; } /** @@ -119,8 +119,13 @@ export class Span implements SpanInterface, SpanContext { /** * List of spans that were finalized */ - public spanRecorder?: SpanRecorder; + public spanList?: SpanList; + /** + * You should never call the custructor manually, always use `hub.startSpan()`. + * @internal + * @hideconstructor + */ public constructor(spanContext?: SpanContext, hub?: Hub) { if (isInstanceOf(hub, Hub)) { this._hub = hub as Hub; @@ -167,11 +172,11 @@ export class Span implements SpanInterface, SpanContext { * Attaches SpanRecorder to the span itself * @param maxlen maximum number of spans that can be recorded */ - public initFinishedSpans(maxlen: number = 1000): void { - if (!this.spanRecorder) { - this.spanRecorder = new SpanRecorder(maxlen); + public initSpanList(maxlen: number = 1000): void { + if (!this.spanList) { + this.spanList = new SpanList(maxlen); } - this.spanRecorder.startSpan(this); + this.spanList.add(this); } /** @@ -187,7 +192,10 @@ export class Span implements SpanInterface, SpanContext { traceId: this._traceId, }); - span.spanRecorder = this.spanRecorder; + span.spanList = this.spanList; + if (span.spanList) { + span.spanList.add(span); + } return span; } @@ -283,11 +291,13 @@ export class Span implements SpanInterface, SpanContext { this.timestamp = timestampWithMs(); - if (this.spanRecorder === undefined) { + // This happens if a span was initiated outside of `hub.startSpan` + // Also if the span was sampled (sampled = false) in `hub.startSpan` already + if (this.spanList === undefined) { return undefined; } - this.spanRecorder.finishSpan(this); + this.spanList.finishSpan(this); if (this.sampled !== true) { // At this point if `sampled !== true` we want to discard the transaction. @@ -295,7 +305,7 @@ export class Span implements SpanInterface, SpanContext { return undefined; } - const finishedSpans = this.spanRecorder ? this.spanRecorder.finishedSpans.filter(s => s !== this) : []; + const finishedSpans = this.spanList ? this.spanList.finishedSpans.filter(s => s !== this) : []; if (trimEnd && finishedSpans.length > 0) { this.timestamp = finishedSpans.reduce((prev: Span, current: Span) => { diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index 393ef18f29af..0366cba97ba6 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -13,21 +13,27 @@ describe('Hub', () => { describe('spans', () => { describe('sampling', () => { - test('set tracesSampleRate 0', () => { + test('set tracesSampleRate 0 root span', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); const span = hub.startSpan() as any; - expect(span.sampled).toBeUndefined(); + expect(span.sampled).toBe(false); }); test('set tracesSampleRate 0 on transaction', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); const span = hub.startSpan({ transaction: 'foo' }) as any; expect(span.sampled).toBe(false); }); - test('set tracesSampleRate 1', () => { + test('set tracesSampleRate 1 on transaction', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const span = hub.startSpan({ transaction: 'foo' }) as any; expect(span.sampled).toBeTruthy(); }); + test('set tracesSampleRate should be propergated to children', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); + const span = hub.startSpan() as any; + const child = span.child({ op: 1 }); + expect(child.sampled).toBeFalsy(); + }); }); describe('start', () => { test('simple', () => { diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index b18aa2051293..e8e9afe79f6a 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -1,3 +1,4 @@ +import { BrowserClient } from '@sentry/browser'; import { Hub, Scope } from '@sentry/hub'; import { SpanStatus } from '@sentry/types'; @@ -7,9 +8,8 @@ describe('Span', () => { let hub: Hub; beforeEach(() => { - const clientFn: any = jest.fn(); const myScope = new Scope(); - hub = new Hub(clientFn, myScope); + hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }), myScope); }); describe('newSpan', () => { @@ -90,6 +90,15 @@ describe('Span', () => { expect((span as any)._hub).toBeInstanceOf(Hub); expect((span2 as any)._hub).toBeInstanceOf(Hub); }); + + test('inherit span list', () => { + const span = new Span({ sampled: true }); + const span2 = span.child(); + const span3 = span.child(); + span3.finish(); + expect(span.spanList).toBe(span2.spanList); + expect(span.spanList).toBe(span3.spanList); + }); }); describe('toTraceparent', () => { @@ -183,50 +192,108 @@ describe('Span', () => { expect(span.timestamp).toBeGreaterThan(1); }); - test('finish a span without transaction', () => { - const spy = jest.spyOn(hub as any, 'captureEvent'); - const span = new Span({}, hub); - span.finish(); - expect(spy).not.toHaveBeenCalled(); - }); + describe('hub.startSpan', () => { + test('finish a span', () => { + const spy = jest.spyOn(hub as any, 'captureEvent') as any; + const span = hub.startSpan(); + span.finish(); + expect(spy).toHaveBeenCalled(); + expect(spy.mock.calls[0][0].spans).toHaveLength(0); + expect(spy.mock.calls[0][0].timestamp).toBeTruthy(); + expect(spy.mock.calls[0][0].start_timestamp).toBeTruthy(); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(span.getTraceContext()); + }); - test('finish a span with transaction', () => { - const spy = jest.spyOn(hub as any, 'captureEvent') as any; - const span = new Span({ transaction: 'test', sampled: false }, hub); - span.initFinishedSpans(); - span.finish(); - expect(spy.mock.calls[0][0].spans).toHaveLength(0); - expect(spy.mock.calls[0][0].contexts.trace).toEqual(span.getTraceContext()); - }); + test('finish a span with transaction + child span', () => { + const spy = jest.spyOn(hub as any, 'captureEvent') as any; + const parentSpan = hub.startSpan(); + const childSpan = parentSpan.child(); + childSpan.finish(); + parentSpan.finish(); + expect(spy).toHaveBeenCalled(); + expect(spy.mock.calls[0][0].spans).toHaveLength(1); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(parentSpan.getTraceContext()); + }); - test('finish a span with transaction + child span', () => { - const spy = jest.spyOn(hub as any, 'captureEvent') as any; - const parentSpan = new Span({ transaction: 'test', sampled: false }, hub); - parentSpan.initFinishedSpans(); - const childSpan = parentSpan.child(); - childSpan.finish(); - parentSpan.finish(); - expect(spy.mock.calls[0][0].spans).toHaveLength(1); - expect(spy.mock.calls[0][0].contexts.trace).toEqual(parentSpan.getTraceContext()); - }); + test("finish a child span shouldn't trigger captureEvent", () => { + const spy = jest.spyOn(hub as any, 'captureEvent') as any; + const parentSpan = hub.startSpan(); + const childSpan = parentSpan.child(); + childSpan.finish(); + expect(spy).not.toHaveBeenCalled(); + }); - test('finish a span with another one on the scope shouldnt override contexts.trace', () => { - const spy = jest.spyOn(hub as any, 'captureEvent') as any; + test('finish a span with another one on the scope should add the span and not call captureEvent', () => { + const spy = jest.spyOn(hub as any, 'captureEvent') as any; - const spanOne = new Span({ transaction: 'testOne', sampled: false }, hub); - spanOne.initFinishedSpans(); - const childSpanOne = spanOne.child(); - childSpanOne.finish(); - hub.configureScope(scope => { - scope.setSpan(spanOne); + const spanOne = hub.startSpan(); + const childSpanOne = spanOne.child(); + childSpanOne.finish(); + + hub.configureScope(scope => { + scope.setSpan(spanOne); + }); + + const spanTwo = hub.startSpan(); + spanTwo.finish(); + + expect(spy).not.toHaveBeenCalled(); + expect((spanOne as any).spanList.finishedSpans).toHaveLength(2); }); - const spanTwo = new Span({ transaction: 'testTwo', sampled: false }, hub); - spanTwo.initFinishedSpans(); - spanTwo.finish(); + test("finish a span with another one on the scope shouldn't override contexts.trace", () => { + const spy = jest.spyOn(hub as any, 'captureEvent') as any; + + const spanOne = hub.startSpan(); + const childSpanOne = spanOne.child(); + childSpanOne.finish(); + + hub.configureScope(scope => { + scope.setSpan(spanOne); + }); + + const spanTwo = hub.startSpan(); + spanTwo.finish(); + spanOne.finish(); - expect(spy.mock.calls[0][0].spans).toHaveLength(0); - expect(spy.mock.calls[0][0].contexts.trace).toEqual(spanTwo.getTraceContext()); + expect(spy).toHaveBeenCalled(); + expect(spy.mock.calls[0][0].spans).toHaveLength(2); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(spanOne.getTraceContext()); + }); + + test('span child limit', () => { + const _hub = new Hub( + new BrowserClient({ + _experiments: { maxSpans: 3 }, + tracesSampleRate: 1, + }), + ); + const spy = jest.spyOn(_hub as any, 'captureEvent') as any; + const span = _hub.startSpan(); + for (let i = 0; i < 10; i++) { + const child = span.child(); + child.finish(); + } + span.finish(); + expect(spy.mock.calls[0][0].spans).toHaveLength(3); + }); + + test('if we sampled the parent (transaction) we do not want any childs', () => { + const _hub = new Hub( + new BrowserClient({ + tracesSampleRate: 0, + }), + ); + const spy = jest.spyOn(_hub as any, 'captureEvent') as any; + const span = _hub.startSpan(); + for (let i = 0; i < 10; i++) { + const child = span.child(); + child.finish(); + } + span.finish(); + expect((span as any).spanList).toBeUndefined(); + expect(spy).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index ccc8fa410e9a..6b97b7af52ca 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -174,7 +174,7 @@ export interface Hub { /** * This functions starts a span. If argument passed is of type `Span`, it'll run sampling on it if configured - * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope, + * and attach a `SpanList`. If it's of type `SpanContext` and there is already a `Span` on the Scope, * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * * @param span Already constructed span which should be started or properties with which the span should be created From 26bee0936a27d6bafb1fad4f7f6b8b93657c2bcb Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 19 Mar 2020 10:32:25 +0100 Subject: [PATCH 29/33] ref: Rename back to SpanRecorder to be consistent with Python --- packages/apm/src/hubextensions.ts | 4 ++-- packages/apm/src/integrations/tracing.ts | 8 +++---- packages/apm/src/span.ts | 28 ++++++++++++------------ packages/apm/test/span.test.ts | 8 +++---- packages/types/src/hub.ts | 2 +- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index eb25834247b4..2fb1fbbe9c26 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -30,7 +30,7 @@ function traceHeaders(): { [key: string]: string } { /** * This functions starts a span. If argument passed is of type `Span`, it'll run sampling on it if configured - * and attach a `SpanList`. If it's of type `SpanContext` and there is already a `Span` on the Scope, + * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope, * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * * @param spanOrSpanContext Already constructed span or properties with which the span should be created @@ -65,7 +65,7 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = f // in case we will discard the span anyway because sampled == false, we safe memory and do not store child spans if (span.sampled) { const experimentsOptions = (client && client.getOptions()._experiments) || {}; - span.initSpanList(experimentsOptions.maxSpans as number); + span.initSpanRecorder(experimentsOptions.maxSpans as number); } return span; diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 0210e973d782..284954993f3d 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -407,8 +407,8 @@ export class Tracing implements Integration { // tslint:disable-next-line: completed-docs function addSpan(span: SpanClass): void { - if (transactionSpan.spanList) { - transactionSpan.spanList.finishSpan(span); + if (transactionSpan.spanRecorder) { + transactionSpan.spanRecorder.finishSpan(span); } } @@ -498,8 +498,8 @@ export class Tracing implements Integration { const resourceName = entry.name.replace(window.location.origin, ''); if (entry.initiatorType === 'xmlhttprequest' || entry.initiatorType === 'fetch') { // We need to update existing spans with new timing info - if (transactionSpan.spanList) { - transactionSpan.spanList.finishedSpans.map((finishedSpan: SpanClass) => { + if (transactionSpan.spanRecorder) { + transactionSpan.spanRecorder.finishedSpans.map((finishedSpan: SpanClass) => { if (finishedSpan.description && finishedSpan.description.indexOf(resourceName) !== -1) { finishedSpan.startTimestamp = timeOrigin + startTime; finishedSpan.timestamp = finishedSpan.startTimestamp + duration; diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index ee01a9f58b26..3c1e044f4d0e 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -16,7 +16,7 @@ export const TRACEPARENT_REGEXP = new RegExp( /** * Keeps track of finished spans for a given transaction */ -class SpanList { +class SpanRecorder { private readonly _maxlen: number; private _openSpanCount: number = 0; public finishedSpans: Span[] = []; @@ -31,9 +31,9 @@ class SpanList { * trace tree (i.e.the first n spans with the smallest * start_timestamp). */ - public add(span: Span): void { + public startSpan(span: Span): void { if (this._openSpanCount > this._maxlen) { - span.spanList = undefined; + span.spanRecorder = undefined; } this._openSpanCount++; } @@ -119,7 +119,7 @@ export class Span implements SpanInterface, SpanContext { /** * List of spans that were finalized */ - public spanList?: SpanList; + public spanRecorder?: SpanRecorder; /** * You should never call the custructor manually, always use `hub.startSpan()`. @@ -172,11 +172,11 @@ export class Span implements SpanInterface, SpanContext { * Attaches SpanRecorder to the span itself * @param maxlen maximum number of spans that can be recorded */ - public initSpanList(maxlen: number = 1000): void { - if (!this.spanList) { - this.spanList = new SpanList(maxlen); + public initSpanRecorder(maxlen: number = 1000): void { + if (!this.spanRecorder) { + this.spanRecorder = new SpanRecorder(maxlen); } - this.spanList.add(this); + this.spanRecorder.startSpan(this); } /** @@ -192,9 +192,9 @@ export class Span implements SpanInterface, SpanContext { traceId: this._traceId, }); - span.spanList = this.spanList; - if (span.spanList) { - span.spanList.add(span); + span.spanRecorder = this.spanRecorder; + if (span.spanRecorder) { + span.spanRecorder.startSpan(span); } return span; @@ -293,11 +293,11 @@ export class Span implements SpanInterface, SpanContext { // This happens if a span was initiated outside of `hub.startSpan` // Also if the span was sampled (sampled = false) in `hub.startSpan` already - if (this.spanList === undefined) { + if (this.spanRecorder === undefined) { return undefined; } - this.spanList.finishSpan(this); + this.spanRecorder.finishSpan(this); if (this.sampled !== true) { // At this point if `sampled !== true` we want to discard the transaction. @@ -305,7 +305,7 @@ export class Span implements SpanInterface, SpanContext { return undefined; } - const finishedSpans = this.spanList ? this.spanList.finishedSpans.filter(s => s !== this) : []; + const finishedSpans = this.spanRecorder ? this.spanRecorder.finishedSpans.filter(s => s !== this) : []; if (trimEnd && finishedSpans.length > 0) { this.timestamp = finishedSpans.reduce((prev: Span, current: Span) => { diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index e8e9afe79f6a..d9029da6b8a3 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -96,8 +96,8 @@ describe('Span', () => { const span2 = span.child(); const span3 = span.child(); span3.finish(); - expect(span.spanList).toBe(span2.spanList); - expect(span.spanList).toBe(span3.spanList); + expect(span.spanRecorder).toBe(span2.spanRecorder); + expect(span.spanRecorder).toBe(span3.spanRecorder); }); }); @@ -238,7 +238,7 @@ describe('Span', () => { spanTwo.finish(); expect(spy).not.toHaveBeenCalled(); - expect((spanOne as any).spanList.finishedSpans).toHaveLength(2); + expect((spanOne as any).spanRecorder.finishedSpans).toHaveLength(2); }); test("finish a span with another one on the scope shouldn't override contexts.trace", () => { @@ -291,7 +291,7 @@ describe('Span', () => { child.finish(); } span.finish(); - expect((span as any).spanList).toBeUndefined(); + expect((span as any).spanRecorder).toBeUndefined(); expect(spy).not.toHaveBeenCalled(); }); }); diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 6b97b7af52ca..ccc8fa410e9a 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -174,7 +174,7 @@ export interface Hub { /** * This functions starts a span. If argument passed is of type `Span`, it'll run sampling on it if configured - * and attach a `SpanList`. If it's of type `SpanContext` and there is already a `Span` on the Scope, + * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope, * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * * @param span Already constructed span which should be started or properties with which the span should be created From a2351ab8a2596e50d9fed04fdd09731a07885fa1 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 19 Mar 2020 11:27:40 +0100 Subject: [PATCH 30/33] ref: SpanRecorder --- packages/apm/src/hubextensions.ts | 7 ++- packages/apm/src/integrations/tracing.ts | 39 +++++++++------ packages/apm/src/span.ts | 51 ++++++++++++-------- packages/apm/test/span.test.ts | 60 +++++++++++++++++++++++- packages/types/src/span.ts | 29 ++++++++++-- 5 files changed, 146 insertions(+), 40 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 2fb1fbbe9c26..1c14124e3236 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -44,10 +44,15 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = f const client = hub.getClient(); let span; + // This flag determines if we already added the span as a child to the span that currently lives on the scope + // If we do not have this, we will add it later on twice to the span recorder and therefore have too many spans + let addedAsChild = false; + if (!isSpanInstance(spanOrSpanContext) && !makeRoot && scope) { const parentSpan = scope.getSpan() as Span; if (parentSpan) { span = parentSpan.child(spanOrSpanContext); + addedAsChild = true; } } @@ -63,7 +68,7 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = f // We only want to create a span list if we sampled the transaction // in case we will discard the span anyway because sampled == false, we safe memory and do not store child spans - if (span.sampled) { + if (span.sampled && !addedAsChild) { const experimentsOptions = (client && client.getOptions()._experiments) || {}; span.initSpanRecorder(experimentsOptions.maxSpans as number); } diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 284954993f3d..82fca2f653c9 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -1,4 +1,5 @@ -import { Event, EventProcessor, Hub, Integration, Span, SpanContext, SpanStatus } from '@sentry/types'; +import { Hub, Scope } from '@sentry/hub'; +import { Event, EventProcessor, Integration, Span, SpanContext, SpanStatus } from '@sentry/types'; import { addInstrumentationHandler, getGlobalObject, @@ -272,6 +273,19 @@ export class Tracing implements Integration { * Unsets the current active transaction + activities */ private static _resetActiveTransaction(): void { + // We want to clean up after ourselves + // If there is still the active transaction on the scope we remove it + const _getCurrentHub = Tracing._getCurrentHub; + if (_getCurrentHub) { + const hub = _getCurrentHub(); + const scope = hub.getScope(); + if (scope) { + if (scope.getSpan() === Tracing._activeTransaction) { + scope.setSpan(undefined); + } + } + } + // ------------------------------------------------------------------ Tracing._activeTransaction = undefined; Tracing._activities = {}; } @@ -365,6 +379,13 @@ export class Tracing implements Integration { true, ); + // We set the transaction on the scope so if there are any other spans started outside of this integration + // we also add them to this transaction. + // Once the idle transaction is finished, we make sure to remove it again. + hub.configureScope((scope: Scope) => { + scope.setSpan(Tracing._activeTransaction); + }); + // The reason we do this here is because of cached responses // If we start and transaction without an activity it would never finish since there is no activity const id = Tracing.pushActivity('idleTransactionStarted'); @@ -405,13 +426,6 @@ export class Tracing implements Integration { const timeOrigin = Tracing._msToSec(performance.timeOrigin); - // tslint:disable-next-line: completed-docs - function addSpan(span: SpanClass): void { - if (transactionSpan.spanRecorder) { - transactionSpan.spanRecorder.finishSpan(span); - } - } - // tslint:disable-next-line: completed-docs function addPerformanceNavigationTiming(parent: SpanClass, entry: { [key: string]: number }, event: string): void { const span = parent.child({ @@ -420,7 +434,6 @@ export class Tracing implements Integration { }); span.startTimestamp = timeOrigin + Tracing._msToSec(entry[`${event}Start`]); span.timestamp = timeOrigin + Tracing._msToSec(entry[`${event}End`]); - addSpan(span); } // tslint:disable-next-line: completed-docs @@ -431,14 +444,13 @@ export class Tracing implements Integration { }); request.startTimestamp = timeOrigin + Tracing._msToSec(entry.requestStart); request.timestamp = timeOrigin + Tracing._msToSec(entry.responseEnd); - addSpan(request); + const response = parent.child({ description: 'response', op: 'browser', }); response.startTimestamp = timeOrigin + Tracing._msToSec(entry.responseStart); response.timestamp = timeOrigin + Tracing._msToSec(entry.responseEnd); - addSpan(response); } let entryScriptSrc: string | undefined; @@ -492,14 +504,13 @@ export class Tracing implements Integration { if (tracingInitMarkStartTime === undefined && entry.name === 'sentry-tracing-init') { tracingInitMarkStartTime = mark.startTimestamp; } - addSpan(mark); break; case 'resource': const resourceName = entry.name.replace(window.location.origin, ''); if (entry.initiatorType === 'xmlhttprequest' || entry.initiatorType === 'fetch') { // We need to update existing spans with new timing info if (transactionSpan.spanRecorder) { - transactionSpan.spanRecorder.finishedSpans.map((finishedSpan: SpanClass) => { + transactionSpan.spanRecorder.spans.map((finishedSpan: SpanClass) => { if (finishedSpan.description && finishedSpan.description.indexOf(resourceName) !== -1) { finishedSpan.startTimestamp = timeOrigin + startTime; finishedSpan.timestamp = finishedSpan.startTimestamp + duration; @@ -517,7 +528,6 @@ export class Tracing implements Integration { if (entryScriptStartEndTime === undefined && (entryScriptSrc || '').includes(resourceName)) { entryScriptStartEndTime = resource.timestamp; } - addSpan(resource); } break; default: @@ -532,7 +542,6 @@ export class Tracing implements Integration { }); evaluation.startTimestamp = entryScriptStartEndTime; evaluation.timestamp = tracingInitMarkStartTime; - addSpan(evaluation); } Tracing._performanceCursor = Math.max(performance.getEntries().length - 1, 0); diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 3c1e044f4d0e..f8e4c009c8bf 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -18,8 +18,7 @@ export const TRACEPARENT_REGEXP = new RegExp( */ class SpanRecorder { private readonly _maxlen: number; - private _openSpanCount: number = 0; - public finishedSpans: Span[] = []; + public spans: Span[] = []; public constructor(maxlen: number = 1000) { this._maxlen = maxlen; @@ -31,19 +30,12 @@ class SpanRecorder { * trace tree (i.e.the first n spans with the smallest * start_timestamp). */ - public startSpan(span: Span): void { - if (this._openSpanCount > this._maxlen) { + public add(span: Span): void { + if (this.spans.length > this._maxlen) { span.spanRecorder = undefined; + } else { + this.spans.push(span); } - this._openSpanCount++; - } - - /** - * Appends a span to finished spans table - * @param span Span to be added - */ - public finishSpan(span: Span): void { - this.finishedSpans.push(span); } } @@ -176,7 +168,7 @@ export class Span implements SpanInterface, SpanContext { if (!this.spanRecorder) { this.spanRecorder = new SpanRecorder(maxlen); } - this.spanRecorder.startSpan(this); + this.spanRecorder.add(this); } /** @@ -194,7 +186,7 @@ export class Span implements SpanInterface, SpanContext { span.spanRecorder = this.spanRecorder; if (span.spanRecorder) { - span.spanRecorder.startSpan(span); + span.spanRecorder.add(span); } return span; @@ -297,15 +289,13 @@ export class Span implements SpanInterface, SpanContext { return undefined; } - this.spanRecorder.finishSpan(this); - if (this.sampled !== true) { // At this point if `sampled !== true` we want to discard the transaction. logger.warn('Discarding transaction Span because it was span.sampled !== true'); return undefined; } - const finishedSpans = this.spanRecorder ? this.spanRecorder.finishedSpans.filter(s => s !== this) : []; + const finishedSpans = this.spanRecorder ? this.spanRecorder.spans.filter(s => s !== this && s.timestamp) : []; if (trimEnd && finishedSpans.length > 0) { this.timestamp = finishedSpans.reduce((prev: Span, current: Span) => { @@ -348,7 +338,16 @@ export class Span implements SpanInterface, SpanContext { /** * @inheritDoc */ - public getTraceContext(): object { + public getTraceContext(): { + data?: { [key: string]: any }; + description?: string; + op?: string; + parent_span_id?: string; + span_id: string; + status?: string; + tags?: { [key: string]: string }; + trace_id: string; + } { return dropUndefinedKeys({ data: Object.keys(this.data).length > 0 ? this.data : undefined, description: this.description, @@ -364,7 +363,19 @@ export class Span implements SpanInterface, SpanContext { /** * @inheritDoc */ - public toJSON(): object { + public toJSON(): { + data?: { [key: string]: any }; + description?: string; + op?: string; + parent_span_id?: string; + sampled?: boolean; + span_id: string; + start_timestamp: number; + tags?: { [key: string]: string }; + timestamp?: number; + trace_id: string; + transaction?: string; + } { return dropUndefinedKeys({ data: Object.keys(this.data).length > 0 ? this.data : undefined, description: this.description, diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index d9029da6b8a3..5276d06ad65c 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -238,7 +238,9 @@ describe('Span', () => { spanTwo.finish(); expect(spy).not.toHaveBeenCalled(); - expect((spanOne as any).spanRecorder.finishedSpans).toHaveLength(2); + expect((spanOne as any).spanRecorder.spans).toHaveLength(3); + // We only want two finished spans + expect((spanOne as any).spanRecorder.spans.filter((s: Span) => !!s.timestamp)).toHaveLength(2); }); test("finish a span with another one on the scope shouldn't override contexts.trace", () => { @@ -294,6 +296,62 @@ describe('Span', () => { expect((span as any).spanRecorder).toBeUndefined(); expect(spy).not.toHaveBeenCalled(); }); + + test('mixing hub.startSpan + span.child + maxSpans', () => { + const _hub = new Hub( + new BrowserClient({ + _experiments: { maxSpans: 2 }, + tracesSampleRate: 1, + }), + ); + const spy = jest.spyOn(_hub as any, 'captureEvent') as any; + + const spanOne = _hub.startSpan(); + const childSpanOne = spanOne.child({ op: '1' }); + childSpanOne.finish(); + + _hub.configureScope(scope => { + scope.setSpan(spanOne); + }); + + const spanTwo = _hub.startSpan({ op: '2' }); + spanTwo.finish(); + + const spanThree = _hub.startSpan({ op: '3' }); + spanThree.finish(); + + spanOne.finish(); + + expect(spy).toHaveBeenCalled(); + expect(spy.mock.calls[0][0].spans).toHaveLength(2); + }); + + test('tree structure of spans should be correct when mixing it with span on scope', () => { + const spy = jest.spyOn(hub as any, 'captureEvent') as any; + + const spanOne = hub.startSpan(); + const childSpanOne = spanOne.child(); + + const childSpanTwo = childSpanOne.child(); + childSpanTwo.finish(); + + childSpanOne.finish(); + + hub.configureScope(scope => { + scope.setSpan(spanOne); + }); + + const spanTwo = hub.startSpan(); + spanTwo.finish(); + spanOne.finish(); + + expect(spy).toHaveBeenCalled(); + expect(spy.mock.calls[0][0].spans).toHaveLength(3); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(spanOne.getTraceContext()); + expect(childSpanOne.toJSON().parent_span_id).toEqual(spanOne.toJSON().span_id); + expect(childSpanTwo.toJSON().parent_span_id).toEqual(childSpanOne.toJSON().span_id); + expect(spanTwo.toJSON().parent_span_id).toEqual(spanOne.toJSON().span_id); + }); }); }); diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 71dc91801368..d498dcc87d1d 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -5,9 +5,30 @@ export interface Span { /** Return a traceparent compatible header string */ toTraceparent(): string; /** Convert the object to JSON for w. spans array info only */ - getTraceContext(): object; + getTraceContext(): { + data?: { [key: string]: any }; + description?: string; + op?: string; + parent_span_id?: string; + span_id: string; + status?: string; + tags?: { [key: string]: string }; + trace_id: string; + }; /** Convert the object to JSON */ - toJSON(): object; + toJSON(): { + data?: { [key: string]: any }; + description?: string; + op?: string; + parent_span_id?: string; + sampled?: boolean; + span_id: string; + start_timestamp: number; + tags?: { [key: string]: string }; + timestamp?: number; + trace_id: string; + transaction?: string; + }; /** * Sets the tag attribute on the current span @@ -39,7 +60,9 @@ export interface Span { * Creates a new `Span` while setting the current `Span.id` as `parentSpanId`. * Also the `sampled` decision will be inherited. */ - child(spanContext?: Pick>): Span; + child( + spanContext?: Pick>, + ): Span; /** * Determines whether span was successful (HTTP200) From 0ad436edf9c7bbe64f2fbf33fb99c5275ce08229 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 19 Mar 2020 13:43:14 +0100 Subject: [PATCH 31/33] ref: Remove makeRoot --- packages/apm/src/hubextensions.ts | 25 +++++++----------------- packages/apm/src/integrations/tracing.ts | 11 ++++------- packages/apm/src/span.ts | 14 ++++++------- packages/apm/test/hub.test.ts | 1 + packages/types/src/span.ts | 4 ++-- 5 files changed, 21 insertions(+), 34 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 1c14124e3236..bf36eac54893 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -1,17 +1,8 @@ import { getMainCarrier, Hub } from '@sentry/hub'; import { SpanContext } from '@sentry/types'; -import { isInstanceOf } from '@sentry/utils'; import { Span } from './span'; -/** - * Checks whether given value is instance of Span - * @param span value to check - */ -function isSpanInstance(span: unknown): span is Span { - return isInstanceOf(span, Span); -} - /** Returns all trace headers that are currently on the top scope. */ function traceHeaders(): { [key: string]: string } { // @ts-ignore @@ -33,11 +24,9 @@ function traceHeaders(): { [key: string]: string } { * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope, * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`. * - * @param spanOrSpanContext Already constructed span or properties with which the span should be created - * @param makeRoot This will just create the span as it is and will not attach it to the span on the scope (if there is one). - * Under some circumstances, in internal integrations, for example, this is used to make sure they are not interfering with each other. + * @param spanContext Already constructed span or properties with which the span should be created */ -function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = false): Span { +function startSpan(spanContext?: SpanContext): Span { // @ts-ignore const hub = this as Hub; const scope = hub.getScope(); @@ -48,20 +37,20 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = f // If we do not have this, we will add it later on twice to the span recorder and therefore have too many spans let addedAsChild = false; - if (!isSpanInstance(spanOrSpanContext) && !makeRoot && scope) { + if (scope) { const parentSpan = scope.getSpan() as Span; if (parentSpan) { - span = parentSpan.child(spanOrSpanContext); + span = parentSpan.child(spanContext); addedAsChild = true; } } - if (!isSpanInstance(span)) { - span = new Span(spanOrSpanContext, hub); + if (!span) { + span = new Span(spanContext, hub); } // We only roll the dice on sampling for "root" spans (transactions) because the childs inherit this state - if (span.sampled === undefined && !span.isChildSpan()) { + if (span.sampled === undefined && span.isRootSpan()) { const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; span.sampled = Math.random() < sampleRate; } diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 82fca2f653c9..2f718619e9ad 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -371,13 +371,10 @@ export class Tracing implements Integration { return undefined; } - Tracing._activeTransaction = hub.startSpan( - { - ...spanContext, - transaction: name, - }, - true, - ); + Tracing._activeTransaction = hub.startSpan({ + ...spanContext, + transaction: name, + }); // We set the transaction on the scope so if there are any other spans started outside of this integration // we also add them to this transaction. diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index f8e4c009c8bf..9002dc869339 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -195,8 +195,8 @@ export class Span implements SpanInterface, SpanContext { /** * @inheritDoc */ - public isChildSpan(): boolean { - return this._parentSpanId !== undefined; + public isRootSpan(): boolean { + return this._parentSpanId === undefined; } /** @@ -283,6 +283,11 @@ export class Span implements SpanInterface, SpanContext { this.timestamp = timestampWithMs(); + // We will not send any child spans + if (!this.isRootSpan()) { + return undefined; + } + // This happens if a span was initiated outside of `hub.startSpan` // Also if the span was sampled (sampled = false) in `hub.startSpan` already if (this.spanRecorder === undefined) { @@ -306,11 +311,6 @@ export class Span implements SpanInterface, SpanContext { }).timestamp; } - // We will not send any child spans - if (this.isChildSpan()) { - return undefined; - } - return this._hub.captureEvent({ contexts: { trace: this.getTraceContext(), diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index 0366cba97ba6..cd8f864ab9ee 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -35,6 +35,7 @@ describe('Hub', () => { expect(child.sampled).toBeFalsy(); }); }); + describe('start', () => { test('simple', () => { const hub = new Hub(new BrowserClient()); diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index d498dcc87d1d..ad95ed9f2584 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -70,9 +70,9 @@ export interface Span { isSuccess(): boolean; /** - * Determines if the span is a child of another span + * Determines if the span is transaction (root) */ - isChildSpan(): boolean; + isRootSpan(): boolean; } /** Interface holder all properties that can be set on a Span on creation. */ From 6d471b9d78b20ff892caef192ad01be419169803 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 19 Mar 2020 14:12:07 +0100 Subject: [PATCH 32/33] ref: Changelog --- CHANGELOG.md | 6 +++++- packages/apm/src/span.ts | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8927bd34ad92..790d5781b5ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,11 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott -- [apm] fix: Sampling of traces (#2500) +- [apm] fix: Sampling of traces work now only depending on the client option `tracesSampleRate` (#2500) +- [apm] fix: Remove internal `makeRoot` parameter from `hub.startSpan` (#2500) +- [apm] fix: Made constructor of `Span` internal, only use `hub.startSpan` or `Sentry.startSpan` in the future (#2500) +- [apm] ref: Refactored SpanRecorder to work correctly when recording child spans (#2500) +- [apm] feat: Now individual transaction can be sent without relying on the Scope (#2500) - [apm] ref: Remove status from tags in transaction (#2497) - [browser] fix: Respect breadcrumbs sentry:false option (#2499) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 9002dc869339..6fafa6d8e34b 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -117,6 +117,7 @@ export class Span implements SpanInterface, SpanContext { * You should never call the custructor manually, always use `hub.startSpan()`. * @internal * @hideconstructor + * @hidden */ public constructor(spanContext?: SpanContext, hub?: Hub) { if (isInstanceOf(hub, Hub)) { From 106905d20755c636531200473ceab1f7b807fc26 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 19 Mar 2020 14:27:43 +0100 Subject: [PATCH 33/33] meta: Changelog --- CHANGELOG.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 790d5781b5ef..05d364438dfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,10 +5,8 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott - [apm] fix: Sampling of traces work now only depending on the client option `tracesSampleRate` (#2500) -- [apm] fix: Remove internal `makeRoot` parameter from `hub.startSpan` (#2500) -- [apm] fix: Made constructor of `Span` internal, only use `hub.startSpan` or `Sentry.startSpan` in the future (#2500) -- [apm] ref: Refactored SpanRecorder to work correctly when recording child spans (#2500) -- [apm] feat: Now individual transaction can be sent without relying on the Scope (#2500) +- [apm] fix: Remove internal `forceNoChild` parameter from `hub.startSpan` (#2500) +- [apm] fix: Made constructor of `Span` internal, only use `hub.startSpan` (#2500) - [apm] ref: Remove status from tags in transaction (#2497) - [browser] fix: Respect breadcrumbs sentry:false option (#2499)