diff --git a/CHANGELOG.md b/CHANGELOG.md index 59d6e29c6c2e..05d364438dfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ - "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 `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) + ## 5.14.2 - [apm] fix: Use Performance API for timings when available, including Web Workers (#2492) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 10a0cc30c770..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,36 +24,42 @@ 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 spanContext Already constructed span or properties with which the span should be created */ -function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean = false): Span { +function startSpan(spanContext?: SpanContext): 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) && !forceNoChild) { - if (scope) { - const parentSpan = scope.getSpan() as Span; - if (parentSpan) { - span = parentSpan.child(spanOrSpanContext); - } + // 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 (scope) { + const parentSpan = scope.getSpan() as Span; + if (parentSpan) { + span = parentSpan.child(spanContext); + addedAsChild = true; } } - if (!isSpanInstance(span)) { - span = new Span(spanOrSpanContext, that); + if (!span) { + span = new Span(spanContext, hub); } - 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.isRootSpan()) { const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; span.sampled = Math.random() < sampleRate; } - if (span.sampled) { + // 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 && !addedAsChild) { const experimentsOptions = (client && client.getOptions()._experiments) || {}; - span.initFinishedSpans(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 7f284e848d5f..2f718619e9ad 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, @@ -54,15 +55,6 @@ interface TracingOptions { * Default: true */ startTransactionOnLocationChange: boolean; - /** - * Sample to determine if the Integration should instrument anything. The decision will be taken once per load - * on initalization. - * 0 = 0% chance of instrumenting - * 1 = 100% change of instrumenting - * - * Default: 1 - */ - tracesSampleRate: number; /** * The maximum duration of a transaction before it will be discarded. This is for some edge cases where a browser @@ -109,11 +101,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 +150,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 +175,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 +202,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; @@ -294,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 = {}; } @@ -345,7 +337,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({ @@ -358,31 +350,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 @@ -400,21 +371,17 @@ export class Tracing implements Integration { return undefined; } - const span = hub.startSpan( - { - ...spanContext, - transaction: name, - }, - true, - ); - - Tracing._activeTransaction = span; + Tracing._activeTransaction = hub.startSpan({ + ...spanContext, + transaction: name, + }); - // 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); + // 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 @@ -423,24 +390,7 @@ export class Tracing implements Integration { Tracing.popActivity(id); }, (Tracing.options && Tracing.options.idleTimeout) || 100); - 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); - }); - } - } + return Tracing._activeTransaction; } /** @@ -473,13 +423,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({ @@ -488,7 +431,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 @@ -499,14 +441,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; @@ -560,14 +501,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; @@ -585,7 +525,6 @@ export class Tracing implements Integration { if (entryScriptStartEndTime === undefined && (entryScriptSrc || '').includes(resourceName)) { entryScriptStartEndTime = resource.timestamp; } - addSpan(resource); } break; default: @@ -600,7 +539,6 @@ export class Tracing implements Integration { }); evaluation.startTimestamp = entryScriptStartEndTime; evaluation.timestamp = tracingInitMarkStartTime; - addSpan(evaluation); } Tracing._performanceCursor = Math.max(performance.getEntries().length - 1, 0); @@ -641,11 +579,9 @@ export class Tracing implements Integration { autoPopAfter?: number; }, ): number { - if (!Tracing._isEnabled()) { - // Tracing is not enabled - return 0; - } - if (!Tracing._activeTransaction) { + const activeTransaction = Tracing._activeTransaction; + + if (!activeTransaction) { logger.log(`[Tracing] Not pushing activity ${name} since there is no active transaction`); return 0; } @@ -657,7 +593,7 @@ export class Tracing implements Integration { if (spanContext && _getCurrentHub) { const hub = _getCurrentHub(); if (hub) { - const span = hub.startSpan(spanContext); + const span = activeTransaction.child(spanContext); Tracing._activities[Tracing._currentIndex] = { name, span, @@ -689,10 +625,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; } @@ -700,7 +634,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; if (span) { if (spanData) { Object.keys(spanData).forEach((key: string) => { @@ -778,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 @@ -839,7 +777,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/src/span.ts b/packages/apm/src/span.ts index 0621fb5949da..6fafa6d8e34b 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -18,10 +18,9 @@ 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) { + public constructor(maxlen: number = 1000) { this._maxlen = maxlen; } @@ -31,20 +30,13 @@ class SpanRecorder { * trace tree (i.e.the first n spans with the smallest * start_timestamp). */ - public startSpan(span: Span): void { - this._openSpanCount += 1; - if (this._openSpanCount > this._maxlen) { + public add(span: Span): void { + if (this.spans.length > this._maxlen) { span.spanRecorder = undefined; + } else { + this.spans.push(span); } } - - /** - * Appends a span to finished spans table - * @param span Span to be added - */ - public finishSpan(span: Span): void { - this.finishedSpans.push(span); - } } /** @@ -121,6 +113,12 @@ export class Span implements SpanInterface, SpanContext { */ public spanRecorder?: SpanRecorder; + /** + * You should never call the custructor manually, always use `hub.startSpan()`. + * @internal + * @hideconstructor + * @hidden + */ public constructor(spanContext?: SpanContext, hub?: Hub) { if (isInstanceOf(hub, Hub)) { this._hub = hub as Hub; @@ -167,18 +165,19 @@ 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 { + public initSpanRecorder(maxlen: number = 1000): void { if (!this.spanRecorder) { this.spanRecorder = new SpanRecorder(maxlen); } - this.spanRecorder.startSpan(this); + this.spanRecorder.add(this); } /** - * 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 { + public child( + spanContext?: Pick>, + ): Span { const span = new Span({ ...spanContext, parentSpanId: this._spanId, @@ -187,17 +186,27 @@ export class Span implements SpanInterface, SpanContext { }); span.spanRecorder = this.spanRecorder; + if (span.spanRecorder) { + span.spanRecorder.add(span); + } return span; } + /** + * @inheritDoc + */ + public isRootSpan(): boolean { + return this._parentSpanId === undefined; + } + /** * Continues a trace from a string (usually the header). * @param traceparent Traceparent string */ public static fromTraceparent( traceparent: string, - spanContext?: Pick>, + spanContext?: Pick>, ): Span | undefined { const matches = traceparent.match(TRACEPARENT_REGEXP); if (matches) { @@ -275,28 +284,24 @@ export class Span implements SpanInterface, SpanContext { this.timestamp = timestampWithMs(); - if (this.spanRecorder === undefined) { + // We will not send any child spans + if (!this.isRootSpan()) { return undefined; } - 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. + // 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) { 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. - logger.warn('Discarding transaction Span without sampling decision'); + 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) => { @@ -334,7 +339,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, @@ -350,7 +364,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/hub.test.ts b/packages/apm/test/hub.test.ts index d6873ec2f53d..cd8f864ab9ee 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,40 @@ describe('Hub', () => { }); describe('spans', () => { + describe('sampling', () => { + test('set tracesSampleRate 0 root span', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); + const span = hub.startSpan() as any; + 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 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', () => { - 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/apm/test/span.test.ts b/packages/apm/test/span.test.ts index b18aa2051293..5276d06ad65c 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.spanRecorder).toBe(span2.spanRecorder); + expect(span.spanRecorder).toBe(span3.spanRecorder); + }); }); describe('toTraceparent', () => { @@ -183,50 +192,166 @@ 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 should add the span and not call captureEvent', () => { + 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); + }); - test('finish a span with another one on the scope shouldnt override contexts.trace', () => { - const spy = jest.spyOn(hub as any, 'captureEvent') as any; + const spanTwo = hub.startSpan(); + spanTwo.finish(); - const spanOne = new Span({ transaction: 'testOne', sampled: false }, hub); - spanOne.initFinishedSpans(); - const childSpanOne = spanOne.child(); - childSpanOne.finish(); - hub.configureScope(scope => { - scope.setSpan(spanOne); + expect(spy).not.toHaveBeenCalled(); + 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); }); - 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).toHaveBeenCalled(); + expect(spy.mock.calls[0][0].spans).toHaveLength(2); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(spanOne.getTraceContext()); + }); - expect(spy.mock.calls[0][0].spans).toHaveLength(0); - expect(spy.mock.calls[0][0].contexts.trace).toEqual(spanTwo.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).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/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..a6cc609db7ac 100644 --- a/packages/core/src/sdk.ts +++ b/packages/core/src/sdk.ts @@ -16,5 +16,7 @@ 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); } diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index efac6e2fcf7e..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,13 +566,18 @@ 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()], }); - expect(Object.keys(client.getIntegrations()).length).toBe(1); + client.setupIntegrations(); + expect(Object.keys((client as any)._integrations).length).toBe(1); expect(client.getIntegration(TestIntegration)).toBeTruthy(); }); @@ -580,7 +586,8 @@ describe('BaseClient', () => { const client = new TestClient({ integrations: [new TestIntegration()], }); - expect(Object.keys(client.getIntegrations()).length).toBe(0); + client.setupIntegrations(); + expect(Object.keys((client as any)._integrations).length).toBe(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); @@ -591,7 +598,8 @@ describe('BaseClient', () => { enabled: false, integrations: [new TestIntegration()], }); - expect(Object.keys(client.getIntegrations()).length).toBe(0); + client.setupIntegrations(); + expect(Object.keys((client as any)._integrations).length).toBe(0); expect(client.getIntegration(TestIntegration)).toBeFalsy(); }); }); 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; }, }; diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 6d297efe64c6..5886c3707e82 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) { + client.setupIntegrations(); + } } /** 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', }); } 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; } diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 103daf452a21..fd23a2904582 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 trace sampling. + * + * 0.0 = 0% chance of instrumenting + * 1.0 = 100% chance of instrumenting + * + * Default: 0.0 + */ tracesSampleRate?: number; /** Attaches stacktraces to pure capture message / log integrations */ diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 4bb18a38fb74..ad95ed9f2584 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 @@ -35,10 +56,23 @@ 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) */ isSuccess(): boolean; + + /** + * Determines if the span is transaction (root) + */ + isRootSpan(): boolean; } /** Interface holder all properties that can be set on a Span on creation. */