From 26855778174fdd46037d51ffe880236434304e9c Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 19 May 2020 13:42:13 +0200 Subject: [PATCH 01/28] feat: Introduce startTransaction --- packages/apm/src/hubextensions.ts | 55 +++++---- packages/apm/src/integrations/tracing.ts | 58 ++++++---- packages/apm/src/span.ts | 135 +++-------------------- packages/apm/src/transaction.ts | 126 +++++++++++++++++++++ packages/apm/test/span.test.ts | 6 +- packages/hub/src/hub.ts | 5 +- packages/minimal/src/index.ts | 9 +- packages/types/src/hub.ts | 4 +- packages/types/src/index.ts | 1 + packages/types/src/span.ts | 116 ++++++++++--------- packages/types/src/transaction.ts | 18 +++ 11 files changed, 305 insertions(+), 228 deletions(-) create mode 100644 packages/apm/src/transaction.ts create mode 100644 packages/types/src/transaction.ts diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 8f128610113c..e1c5dd211225 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -1,7 +1,8 @@ import { getMainCarrier, Hub } from '@sentry/hub'; -import { SpanContext } from '@sentry/types'; +import { SpanContext, TransactionContext } from '@sentry/types'; import { Span } from './span'; +import { Transaction } from './transaction'; /** Returns all trace headers that are currently on the top scope. */ function traceHeaders(): { [key: string]: string } { @@ -26,43 +27,51 @@ function traceHeaders(): { [key: string]: string } { * * @param spanContext Properties with which the span should be created */ -function startSpan(spanContext?: SpanContext): Span { +function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Span { // @ts-ignore const hub = this as Hub; const scope = hub.getScope(); 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; + let newSpanContext = spanOrTransactionContext; + // We create an empty Span always in case there is nothing on the scope + let parentSpan = new Span(); if (scope) { - const parentSpan = scope.getSpan() as Span; + // If there is a Span on the Scope we use the span_id / trace_id + // To define the parent <-> child relationship + parentSpan = scope.getSpan() as Span; if (parentSpan) { - span = parentSpan.child(spanContext); - addedAsChild = true; + const { span_id, trace_id } = parentSpan.getTraceContext(); + newSpanContext = { + parentSpanId: span_id, + traceId: trace_id, + ...spanOrTransactionContext, + }; } } - if (!span) { - span = new Span(spanContext, hub); - } + // We are dealing with a Transaction + if ((newSpanContext as TransactionContext).name) { + const transaction = new Transaction(newSpanContext as TransactionContext, hub); - // 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; - } + // We only roll the dice on sampling for "root" spans (transactions) because the childs inherit this state + if (transaction.sampled === undefined) { + const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; + transaction.sampled = Math.random() < sampleRate; + } + + // 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 (transaction.sampled) { + const experimentsOptions = (client && client.getOptions()._experiments) || {}; + transaction.initSpanRecorder(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 && !addedAsChild) { - const experimentsOptions = (client && client.getOptions()._experiments) || {}; - span.initSpanRecorder(experimentsOptions.maxSpans as number); + return transaction; } - return span; + return new Span(newSpanContext); } /** diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 525be4a1aec5..9d4586f9751c 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -11,6 +11,7 @@ import { import { Span as SpanClass } from '../span'; import { SpanStatus } from '../spanstatus'; +import { Transaction } from '../transaction'; /** * Options for Tracing integration @@ -132,7 +133,7 @@ export class Tracing implements Integration { */ private static _getCurrentHub?: () => Hub; - private static _activeTransaction?: Span; + private static _activeTransaction?: Transaction; private static _currentIndex: number = 1; @@ -202,6 +203,8 @@ export class Tracing implements Integration { logger.warn(`[Tracing] We added a reasonable default for you: ${defaultTracingOrigins}`); } + Tracing._startTrace(); + // Starting pageload transaction if (global.location && global.location.href) { // Use `${global.location.href}` as transaction name @@ -407,6 +410,26 @@ export class Tracing implements Integration { logger.log(args); } + /** + * TODO + */ + private static _startTrace(): void { + const _getCurrentHub = Tracing._getCurrentHub; + if (!_getCurrentHub) { + return; + } + + const hub = _getCurrentHub(); + if (!hub) { + return; + } + + const trace = hub.startSpan({}); + hub.configureScope((scope: Scope) => { + scope.setSpan(trace); + }); + } + /** * Starts a Transaction waiting for activity idle to finish */ @@ -430,15 +453,8 @@ export class Tracing implements Integration { 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. - // Once the idle transaction is finished, we make sure to remove it again. - hub.configureScope((scope: Scope) => { - scope.setSpan(Tracing._activeTransaction); - }); + name, + }) as Transaction; // 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 @@ -454,11 +470,11 @@ export class Tracing implements Integration { * Finshes the current active transaction */ public static finishIdleTransaction(): void { - const active = Tracing._activeTransaction as SpanClass; + const active = Tracing._activeTransaction; if (active) { Tracing._addPerformanceEntries(active); - Tracing._log('[Tracing] finishIdleTransaction', active.transaction); - active.finish(/*trimEnd*/ true); + Tracing._log('[Tracing] finishIdleTransaction', active.name); + active.finish(undefined, /*trimEnd*/ true); Tracing._resetActiveTransaction(); } } @@ -487,7 +503,7 @@ export class Tracing implements Integration { op: 'browser', }); span.startTimestamp = timeOrigin + Tracing._msToSec(entry[`${event}Start`]); - span.timestamp = timeOrigin + Tracing._msToSec(entry[`${event}End`]); + span.endTimestamp = timeOrigin + Tracing._msToSec(entry[`${event}End`]); } // tslint:disable-next-line: completed-docs @@ -497,14 +513,14 @@ export class Tracing implements Integration { op: 'browser', }); request.startTimestamp = timeOrigin + Tracing._msToSec(entry.requestStart); - request.timestamp = timeOrigin + Tracing._msToSec(entry.responseEnd); + request.endTimestamp = timeOrigin + Tracing._msToSec(entry.responseEnd); const response = parent.child({ description: 'response', op: 'browser', }); response.startTimestamp = timeOrigin + Tracing._msToSec(entry.responseStart); - response.timestamp = timeOrigin + Tracing._msToSec(entry.responseEnd); + response.endTimestamp = timeOrigin + Tracing._msToSec(entry.responseEnd); } let entryScriptSrc: string | undefined; @@ -554,7 +570,7 @@ export class Tracing implements Integration { op: entry.entryType, }); mark.startTimestamp = timeOrigin + startTime; - mark.timestamp = mark.startTimestamp + duration; + mark.endTimestamp = mark.startTimestamp + duration; if (tracingInitMarkStartTime === undefined && entry.name === 'sentry-tracing-init') { tracingInitMarkStartTime = mark.startTimestamp; } @@ -567,7 +583,7 @@ export class Tracing implements Integration { transactionSpan.spanRecorder.spans.map((finishedSpan: SpanClass) => { if (finishedSpan.description && finishedSpan.description.indexOf(resourceName) !== -1) { finishedSpan.startTimestamp = timeOrigin + startTime; - finishedSpan.timestamp = finishedSpan.startTimestamp + duration; + finishedSpan.endTimestamp = finishedSpan.startTimestamp + duration; } }); } @@ -577,10 +593,10 @@ export class Tracing implements Integration { op: `resource`, }); resource.startTimestamp = timeOrigin + startTime; - resource.timestamp = resource.startTimestamp + duration; + resource.endTimestamp = resource.startTimestamp + duration; // We remember the entry script end time to calculate the difference to the first init mark if (entryScriptStartEndTime === undefined && (entryScriptSrc || '').includes(resourceName)) { - entryScriptStartEndTime = resource.timestamp; + entryScriptStartEndTime = resource.endTimestamp; } } break; @@ -595,7 +611,7 @@ export class Tracing implements Integration { op: `script`, }); evaluation.startTimestamp = entryScriptStartEndTime; - evaluation.timestamp = tracingInitMarkStartTime; + evaluation.endTimestamp = tracingInitMarkStartTime; } Tracing._performanceCursor = Math.max(performance.getEntries().length - 1, 0); diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 4b6d2fe0f4e8..82aeb4189508 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -1,10 +1,8 @@ -// tslint:disable:max-classes-per-file - -import { getCurrentHub, Hub } from '@sentry/hub'; import { Span as SpanInterface, SpanContext } from '@sentry/types'; -import { dropUndefinedKeys, isInstanceOf, logger, timestampWithMs, uuid4 } from '@sentry/utils'; +import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils'; import { SpanStatus } from './spanstatus'; +import { SpanRecorder } from './transaction'; // TODO: Should this be exported? export const TRACEPARENT_REGEXP = new RegExp( @@ -15,41 +13,10 @@ export const TRACEPARENT_REGEXP = new RegExp( '[ \\t]*$', // whitespace ); -/** - * Keeps track of finished spans for a given transaction - */ -class SpanRecorder { - private readonly _maxlen: number; - public spans: Span[] = []; - - public constructor(maxlen: number = 1000) { - this._maxlen = maxlen; - } - - /** - * This is just so that we don't run out of memory while recording a lot - * of spans. At some point we just stop and flush out the start of the - * trace tree (i.e.the first n spans with the smallest - * start_timestamp). - */ - public add(span: Span): void { - if (this.spans.length > this._maxlen) { - span.spanRecorder = undefined; - } else { - this.spans.push(span); - } - } -} - /** * Span contains all data about a span */ export class Span implements SpanInterface, SpanContext { - /** - * The reference to the current hub. - */ - private readonly _hub: Hub = (getCurrentHub() as unknown) as Hub; - /** * @inheritDoc */ @@ -83,12 +50,7 @@ export class Span implements SpanInterface, SpanContext { /** * Timestamp in seconds when the span ended. */ - public timestamp?: number; - - /** - * @inheritDoc - */ - public transaction?: string; + public endTimestamp?: number; /** * @inheritDoc @@ -121,15 +83,10 @@ export class Span implements SpanInterface, SpanContext { * @hideconstructor * @hidden */ - public constructor(spanContext?: SpanContext, hub?: Hub) { - if (isInstanceOf(hub, Hub)) { - this._hub = hub as Hub; - } - + public constructor(spanContext?: SpanContext) { if (!spanContext) { return this; } - if (spanContext.traceId) { this._traceId = spanContext.traceId; } @@ -143,9 +100,6 @@ export class Span implements SpanInterface, SpanContext { if ('sampled' in spanContext) { this.sampled = spanContext.sampled; } - if (spanContext.transaction) { - this.transaction = spanContext.transaction; - } if (spanContext.op) { this.op = spanContext.op; } @@ -161,17 +115,12 @@ export class Span implements SpanInterface, SpanContext { if (spanContext.status) { this._status = spanContext.status; } - } - - /** - * Attaches SpanRecorder to the span itself - * @param maxlen maximum number of spans that can be recorded - */ - public initSpanRecorder(maxlen: number = 1000): void { - if (!this.spanRecorder) { - this.spanRecorder = new SpanRecorder(maxlen); + if (spanContext.startTimestamp) { + this.startTimestamp = spanContext.startTimestamp; + } + if (spanContext.endTimestamp) { + this.endTimestamp = spanContext.endTimestamp; } - this.spanRecorder.add(this); } /** @@ -195,13 +144,6 @@ export class Span implements SpanInterface, SpanContext { return span; } - /** - * @inheritDoc - */ - public isRootSpan(): boolean { - return this._parentSpanId === undefined; - } - /** * Continues a trace from a string (usually the header). * @param traceparent Traceparent string @@ -272,59 +214,10 @@ export class Span implements SpanInterface, SpanContext { } /** - * Sets the finish timestamp on the current span. - * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming - * the duration of the transaction span. This is useful to discard extra time in the transaction span that is not - * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the - * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. + * @inheritDoc */ - public finish(trimEnd: boolean = false): string | undefined { - // This transaction is already finished, so we should not flush it again. - if (this.timestamp !== undefined) { - return undefined; - } - - 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) { - 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 span.sampled !== true'); - return undefined; - } - - 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) => { - if (prev.timestamp && current.timestamp) { - return prev.timestamp > current.timestamp ? prev : current; - } - return prev; - }).timestamp; - } - - return this._hub.captureEvent({ - contexts: { - trace: this.getTraceContext(), - }, - spans: finishedSpans, - start_timestamp: this.startTimestamp, - tags: this.tags, - timestamp: this.timestamp, - transaction: this.transaction, - type: 'transaction', - }); + public finish(endTimestamp?: number): void { + this.endTimestamp = typeof endTimestamp === 'number' ? endTimestamp : timestampWithMs(); } /** @@ -377,7 +270,6 @@ export class Span implements SpanInterface, SpanContext { tags?: { [key: string]: string }; timestamp?: number; trace_id: string; - transaction?: string; } { return dropUndefinedKeys({ data: Object.keys(this.data).length > 0 ? this.data : undefined, @@ -388,9 +280,8 @@ export class Span implements SpanInterface, SpanContext { span_id: this._spanId, start_timestamp: this.startTimestamp, tags: Object.keys(this.tags).length > 0 ? this.tags : undefined, - timestamp: this.timestamp, + timestamp: this.endTimestamp, trace_id: this._traceId, - transaction: this.transaction, }); } } diff --git a/packages/apm/src/transaction.ts b/packages/apm/src/transaction.ts new file mode 100644 index 000000000000..0cfa8b95e163 --- /dev/null +++ b/packages/apm/src/transaction.ts @@ -0,0 +1,126 @@ +// tslint:disable:max-classes-per-file +import { getCurrentHub, Hub } from '@sentry/hub'; +import { TransactionContext } from '@sentry/types'; +import { isInstanceOf, logger } from '@sentry/utils'; + +import { Span as SpanClass } from './span'; + +/** + * Keeps track of finished spans for a given transaction + * @internal + * @hideconstructor + * @hidden + */ +export class SpanRecorder { + private readonly _maxlen: number; + public spans: SpanClass[] = []; + + public constructor(maxlen: number = 1000) { + this._maxlen = maxlen; + } + + /** + * This is just so that we don't run out of memory while recording a lot + * of spans. At some point we just stop and flush out the start of the + * trace tree (i.e.the first n spans with the smallest + * start_timestamp). + */ + public add(span: SpanClass): void { + if (this.spans.length > this._maxlen) { + span.spanRecorder = undefined; + } else { + this.spans.push(span); + } + } +} + +/** JSDoc */ +export class Transaction extends SpanClass { + /** + * The reference to the current hub. + */ + private readonly _hub: Hub = (getCurrentHub() as unknown) as Hub; + + public name?: string; + + /** + * You should never call the custructor manually, always use `hub.startSpan()`. + * @internal + * @hideconstructor + * @hidden + */ + public constructor(transactionContext: TransactionContext, hub?: Hub) { + super(transactionContext); + + if (isInstanceOf(hub, Hub)) { + this._hub = hub as Hub; + } + + if (transactionContext.name) { + this.name = transactionContext.name; + } + } + + /** + * JSDoc + */ + public setName(name: string): void { + this.name = name; + } + + /** + * Attaches SpanRecorder to the span itself + * @param maxlen maximum number of spans that can be recorded + */ + public initSpanRecorder(maxlen: number = 1000): void { + if (!this.spanRecorder) { + this.spanRecorder = new SpanRecorder(maxlen); + } + this.spanRecorder.add(this); + } + + /** + * Sets the finish timestamp on the current span. + * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming + * the duration of the transaction span. This is useful to discard extra time in the transaction span that is not + * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the + * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. + */ + public finish(endTimestamp?: number, trimEnd: boolean = false): string | undefined { + // This transaction is already finished, so we should not flush it again. + if (this.endTimestamp !== undefined) { + return undefined; + } + + super.finish(endTimestamp); + + 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.spans.filter(s => s !== this && s.endTimestamp) : []; + + if (trimEnd && finishedSpans.length > 0) { + this.endTimestamp = finishedSpans.reduce((prev: SpanClass, current: SpanClass) => { + if (prev.endTimestamp && current.endTimestamp) { + return prev.endTimestamp > current.endTimestamp ? prev : current; + } + return prev; + }).endTimestamp; + } + + return this._hub.captureEvent({ + contexts: { + trace: this.getTraceContext(), + }, + spans: finishedSpans, + start_timestamp: this.startTimestamp, + tags: this.tags, + timestamp: this.endTimestamp, + transaction: this.name, + type: 'transaction', + }); + } +} diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index 188ff8d6b691..cecca058a614 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -186,9 +186,9 @@ describe('Span', () => { describe('finish', () => { test('simple', () => { const span = new Span({}); - expect(span.timestamp).toBeUndefined(); + expect(span.endTimestamp).toBeUndefined(); span.finish(); - expect(span.timestamp).toBeGreaterThan(1); + expect(span.endTimestamp).toBeGreaterThan(1); }); describe('hub.startSpan', () => { @@ -239,7 +239,7 @@ describe('Span', () => { 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); + expect((spanOne as any).spanRecorder.spans.filter((s: Span) => !!s.endTimestamp)).toHaveLength(2); }); test("finish a span with another one on the scope shouldn't override contexts.trace", () => { diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index ddfd4dc0e9cc..3d95c2c695c4 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -11,6 +11,7 @@ import { Span, SpanContext, User, + TransactionContext, } from '@sentry/types'; import { consoleSandbox, getGlobalObject, isNodeEnv, logger, timestampWithMs, uuid4 } from '@sentry/utils'; @@ -366,8 +367,8 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean = false): Span { - return this._callExtensionMethod('startSpan', spanOrSpanContext, forceNoChild); + public startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Span { + return this._callExtensionMethod('startSpan', spanOrTransactionContext); } /** diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 6e28ce17fcbe..a444195abed4 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -1,5 +1,5 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub'; -import { Breadcrumb, Event, Severity, User } from '@sentry/types'; +import { Breadcrumb, Event, Severity, Transaction, TransactionContext, User } from '@sentry/types'; /** * This calls a function on the current hub. @@ -167,3 +167,10 @@ export function withScope(callback: (scope: Scope) => void): void { export function _callOnClient(method: string, ...args: any[]): void { callOnHub('_invokeClient', method, ...args); } + +/** + * JSDoc TODO + */ +export function startTransaction(transactionContext: TransactionContext): Transaction { + return callOnHub('startSpan', transactionContext); +} diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 20e0adadae08..1eaa95722000 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -6,6 +6,7 @@ import { Scope } from './scope'; import { Severity } from './severity'; import { Span, SpanContext } from './span'; import { User } from './user'; +import { TransactionContext } from './transaction'; /** * Internal class used to make sure we always have the latest internal functions @@ -178,6 +179,7 @@ export interface Hub { * Otherwise it'll crete a new `Span`. * * @param spanContext Properties with which the span should be created + * TODO */ - startSpan(spanContext?: SpanContext): Span; + startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Span; } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 6e82841dde4e..857d304d42c4 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -21,6 +21,7 @@ export { Span, SpanContext } from './span'; export { StackFrame } from './stackframe'; export { Stacktrace } from './stacktrace'; export { Status } from './status'; +export { Transaction, TransactionContext } from './transaction'; export { Thread } from './thread'; export { Transport, TransportOptions, TransportClass } from './transport'; export { User } from './user'; diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index afcfb6e8fd47..7bb35b4018f4 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -1,9 +1,67 @@ +/** Interface holder all properties that can be set on a Span on creation. */ +export interface SpanContext { + /** + * Description of the Span. + */ + description?: string; + /** + * Operation of the Span. + */ + op?: string; + /** + * Completion status of the Span. + * See: {@sentry/apm SpanStatus} for possible values + */ + status?: string; + /** + * Parent Span ID + */ + parentSpanId?: string; + /** + * Has the sampling decision been made? + */ + sampled?: boolean; + /** + * Span ID + */ + spanId?: string; + /** + * Trace ID + */ + traceId?: string; + + /** + * Tags of the Span. + */ + tags?: { [key: string]: string }; + + /** + * Data of the Span. + */ + data?: { [key: string]: any }; + + /** + * Timestamp in seconds when the span started. + */ + startTimestamp?: number; + + /** + * Timestamp in seconds when the span ended. + */ + endTimestamp?: number; +} + /** Span holding trace_id, span_id */ -export interface Span { - /** Sets the finish timestamp on the current span and sends it if it was a transaction */ - finish(useLastSpanTimestamp?: boolean): string | undefined; +export interface Span extends SpanContext { + /** + * Sets the finish timestamp on the current span. + * @param endTimestamp Takes an endTimestamp if the end should not be the time when you call this function. + */ + finish(endTimestamp?: number): void; + /** Return a traceparent compatible header string */ toTraceparent(): string; + /** Convert the object to JSON for w. spans array info only */ getTraceContext(): { data?: { [key: string]: any }; @@ -27,7 +85,6 @@ export interface Span { tags?: { [key: string]: string }; timestamp?: number; trace_id: string; - transaction?: string; }; /** @@ -69,55 +126,4 @@ export interface 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. */ -export interface SpanContext { - /** - * Description of the Span. - */ - description?: string; - /** - * Operation of the Span. - */ - op?: string; - /** - * Completion status of the Span. - * See: {@sentry/apm SpanStatus} for possible values - */ - status?: string; - /** - * Parent Span ID - */ - parentSpanId?: string; - /** - * Has the sampling decision been made? - */ - sampled?: boolean; - /** - * Span ID - */ - spanId?: string; - /** - * Trace ID - */ - traceId?: string; - /** - * Transaction of the Span. - */ - transaction?: string; - /** - * Tags of the Span. - */ - tags?: { [key: string]: string }; - - /** - * Data of the Span. - */ - data?: { [key: string]: any }; } diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts new file mode 100644 index 000000000000..d2437640c3a9 --- /dev/null +++ b/packages/types/src/transaction.ts @@ -0,0 +1,18 @@ +import { SpanContext } from './span'; + +/** + * JSDoc TODO + */ +export interface TransactionContext extends SpanContext { + name: string; +} + +/** + * JSDoc TODO + */ +export interface Transaction extends TransactionContext { + /** + * JSDoc TODO + */ + setName(name: string): void; +} From 59586bb2f478327ebc9e28e95e0cc8e68d82a370 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 19 May 2020 15:12:32 +0200 Subject: [PATCH 02/28] fix: Correctly export interfaces --- packages/browser/src/exports.ts | 1 + packages/core/src/index.ts | 1 + packages/types/src/transaction.ts | 4 ++-- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/browser/src/exports.ts b/packages/browser/src/exports.ts index dc6b03df4b8a..e9ce11d4673f 100644 --- a/packages/browser/src/exports.ts +++ b/packages/browser/src/exports.ts @@ -26,6 +26,7 @@ export { getCurrentHub, Hub, Scope, + startTransaction, setContext, setExtra, setExtras, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0dcfc7bdb9e8..5418c728ff54 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -4,6 +4,7 @@ export { captureEvent, captureMessage, configureScope, + startTransaction, setContext, setExtra, setExtras, diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index d2437640c3a9..00ac53a9c49d 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -1,4 +1,4 @@ -import { SpanContext } from './span'; +import { Span, SpanContext } from './span'; /** * JSDoc TODO @@ -10,7 +10,7 @@ export interface TransactionContext extends SpanContext { /** * JSDoc TODO */ -export interface Transaction extends TransactionContext { +export interface Transaction extends TransactionContext, Span { /** * JSDoc TODO */ From 1409700f47dcb37fbc50d2a295bb515e154a922b Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 19 May 2020 15:29:13 +0200 Subject: [PATCH 03/28] fix: Additional undefined check --- 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 9d4586f9751c..5a5b4846b271 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -743,7 +743,7 @@ export class Tracing implements Integration { } }); } - if (Tracing.options.debug && Tracing.options.debug.spanDebugTimingInfo) { + if (Tracing.options && Tracing.options.debug && Tracing.options.debug.spanDebugTimingInfo) { Tracing._addSpanDebugInfo(span); } span.finish(); From 5192e55d95051531f42e9ddd9d5ab57769712cf2 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 20 May 2020 10:50:38 +0200 Subject: [PATCH 04/28] Apply suggestions from code review Co-authored-by: Katie Byers --- packages/apm/src/hubextensions.ts | 6 +++--- packages/apm/src/transaction.ts | 6 +++--- packages/types/src/span.ts | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index e1c5dd211225..339b76c2468c 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -35,7 +35,7 @@ function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): let newSpanContext = spanOrTransactionContext; - // We create an empty Span always in case there is nothing on the scope + // We create an empty Span in case there is no scope on the hub let parentSpan = new Span(); if (scope) { // If there is a Span on the Scope we use the span_id / trace_id @@ -55,14 +55,14 @@ function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): if ((newSpanContext as TransactionContext).name) { const transaction = new Transaction(newSpanContext as TransactionContext, hub); - // We only roll the dice on sampling for "root" spans (transactions) because the childs inherit this state + // We only roll the dice on sampling for root spans of transactions because all child spans inherit this state if (transaction.sampled === undefined) { const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; transaction.sampled = Math.random() < sampleRate; } // 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 sampled == false, we will discard the span anyway, so we can save memory by not storing child spans if (transaction.sampled) { const experimentsOptions = (client && client.getOptions()._experiments) || {}; transaction.initSpanRecorder(experimentsOptions.maxSpans as number); diff --git a/packages/apm/src/transaction.ts b/packages/apm/src/transaction.ts index 0cfa8b95e163..1a0bf1e2de0d 100644 --- a/packages/apm/src/transaction.ts +++ b/packages/apm/src/transaction.ts @@ -44,7 +44,7 @@ export class Transaction extends SpanClass { public name?: string; /** - * You should never call the custructor manually, always use `hub.startSpan()`. + * This constructor should never be called manually. Those instrumenting tracing should use `Stentry.startTransaction()`, and internal methods should use `hub.startSpan()`. * @internal * @hideconstructor * @hidden @@ -82,7 +82,7 @@ export class Transaction extends SpanClass { /** * Sets the finish timestamp on the current span. * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming - * the duration of the transaction span. This is useful to discard extra time in the transaction span that is not + * the duration of the transaction. This is useful to discard extra time in the transaction that is not * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. */ @@ -96,7 +96,7 @@ export class Transaction extends SpanClass { 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'); + logger.warn('Discarding transaction because it was not chosen to be sampled.); return undefined; } diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 7bb35b4018f4..dec66fe0858e 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -1,4 +1,4 @@ -/** Interface holder all properties that can be set on a Span on creation. */ +/** Interface holding all properties that can be set on a Span on creation. */ export interface SpanContext { /** * Description of the Span. @@ -18,7 +18,7 @@ export interface SpanContext { */ parentSpanId?: string; /** - * Has the sampling decision been made? + * Was this span chosen to be sent as part of the sample? */ sampled?: boolean; /** @@ -41,7 +41,7 @@ export interface SpanContext { data?: { [key: string]: any }; /** - * Timestamp in seconds when the span started. + * Timestamp in seconds (epoch time) indicating when the span started. */ startTimestamp?: number; From 0d5781ad01ce31bd466ae393c2f830daca31d664 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 20 May 2020 11:15:08 +0200 Subject: [PATCH 05/28] fix: Log wording --- packages/apm/src/integrations/tracing.ts | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 741d5b87a1a4..3e873bb496ec 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -71,14 +71,13 @@ interface TracingOptions { maxTransactionDuration: number; /** - * Flag to discard all spans that occur in background. This includes transactions. Browser background tab timing is - * not suited towards doing precise measurements of operations. That's why this option discards any active transaction - * and also doesn't add any spans that happen in the background. Background spans/transaction can mess up your + * Flag Transactions where tabs moved to background with "cancelled".Browser background tab timing is + * not suited towards doing precise measurements of operations. Background transaction can mess up your * statistics in non deterministic ways that's why we by default recommend leaving this opition enabled. * * Default: true */ - discardBackgroundSpans: boolean; + markBackgroundTransactions: boolean; /** * This is only if you want to debug in prod. @@ -165,8 +164,8 @@ export class Tracing implements Integration { spanDebugTimingInfo: false, writeAsBreadcrumbs: false, }, - discardBackgroundSpans: true, idleTimeout: 500, + markBackgroundTransactions: true, maxTransactionDuration: 600, shouldCreateSpanForRequest(url: string): boolean { const origins = (_options && _options.tracingOrigins) || defaultTracingOrigins; @@ -239,7 +238,7 @@ export class Tracing implements Integration { event.timestamp - event.start_timestamp < 0); if (Tracing.options.maxTransactionDuration !== 0 && event.type === 'transaction' && isOutdatedTransaction) { - Tracing._log('[Tracing] Discarded transaction since it maxed out maxTransactionDuration'); + Tracing._log(`[Tracing] Transaction: ${SpanStatus.Cancelled} since it maxed out maxTransactionDuration`); if (event.contexts && event.contexts.trace) { event.contexts.trace = { ...event.contexts.trace, @@ -282,7 +281,9 @@ export class Tracing implements Integration { if (Tracing._heartbeatCounter >= 3) { if (Tracing._activeTransaction) { Tracing._log( - "[Tracing] Heartbeat safeguard kicked in, finishing transaction since activities content hasn't changed for 3 beats", + `[Tracing] Transaction: ${ + SpanStatus.Cancelled + } -> Heartbeat safeguard kicked in since content hasn't changed for 3 beats`, ); Tracing._activeTransaction.setStatus(SpanStatus.DeadlineExceeded); Tracing._activeTransaction.setTag('heartbeat', 'failed'); @@ -298,10 +299,10 @@ export class Tracing implements Integration { * Discards active transactions if tab moves to background */ private _setupBackgroundTabDetection(): void { - if (Tracing.options.discardBackgroundSpans && global.document) { + if (Tracing.options && Tracing.options.markBackgroundTransactions && global.document) { document.addEventListener('visibilitychange', () => { if (document.hidden && Tracing._activeTransaction) { - Tracing._log('[Tracing] Discarded active transaction incl. activities since tab moved to the background'); + Tracing._log(`[Tracing] Transaction: ${SpanStatus.Cancelled} -> since tab moved to the background`); Tracing._activeTransaction.setStatus(SpanStatus.Cancelled); Tracing._activeTransaction.setTag('visibilitychange', 'document.hidden'); Tracing.finishIdleTransaction(); @@ -377,7 +378,7 @@ export class Tracing implements Integration { /** * If an error or unhandled promise occurs, we mark the active transaction as failed */ - Tracing._log(`[Tracing] Global error occured, setting status in transaction: ${SpanStatus.InternalError}`); + Tracing._log(`[Tracing] Transaction: ${SpanStatus.InternalError} -> Global error occured`); Tracing._activeTransaction.setStatus(SpanStatus.InternalError); } } From 5dcc194a3c4c9cb81990461fab3ade7c4820f624 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 20 May 2020 11:15:27 +0200 Subject: [PATCH 06/28] fix: Don't log trace context with non transactions --- packages/hub/src/scope.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index 66c2067fa4cf..c23d49318d92 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -332,7 +332,7 @@ export class Scope implements ScopeInterface { if (this._transaction) { event.transaction = this._transaction; } - if (this._span) { + if (this._span && event.type === 'transaction') { event.contexts = { trace: this._span.getTraceContext(), ...event.contexts }; } From e41640008213f16f6e0770eb4643e5e17f896c6d Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 20 May 2020 11:49:30 +0200 Subject: [PATCH 07/28] fix: Doc Strings --- packages/apm/src/hubextensions.ts | 3 +-- packages/apm/src/integrations/tracing.ts | 2 +- packages/apm/src/span.ts | 1 - packages/minimal/src/index.ts | 5 ++++- packages/types/src/hub.ts | 8 +++----- packages/types/src/transaction.ts | 6 +++--- 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 339b76c2468c..28ef8a666ca9 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -42,9 +42,8 @@ function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): // To define the parent <-> child relationship parentSpan = scope.getSpan() as Span; if (parentSpan) { - const { span_id, trace_id } = parentSpan.getTraceContext(); + const { trace_id } = parentSpan.getTraceContext(); newSpanContext = { - parentSpanId: span_id, traceId: trace_id, ...spanOrTransactionContext, }; diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 3e873bb496ec..798702c16be0 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -412,7 +412,7 @@ export class Tracing implements Integration { } /** - * TODO + * Puts a span on the Scope as the "trace" */ private static _startTrace(): void { const _getCurrentHub = Tracing._getCurrentHub; diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 82aeb4189508..c33951d63504 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -4,7 +4,6 @@ import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils'; import { SpanStatus } from './spanstatus'; import { SpanRecorder } from './transaction'; -// TODO: Should this be exported? export const TRACEPARENT_REGEXP = new RegExp( '^[ \\t]*' + // whitespace '([0-9a-f]{32})?' + // trace_id diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index a444195abed4..8f06826e410e 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -169,7 +169,10 @@ export function _callOnClient(method: string, ...args: any[]): void { } /** - * JSDoc TODO + * This starts a Transaction and is considered the entry point to do manual tracing. You can add child spans + * to a transactions. After that more children can be added to created spans to buld a tree structure. + * This function returns a Transaction and you need to keep track of the instance yourself. When you call `.finsh()` on + * a transaction it will be sent to Sentry. */ export function startTransaction(transactionContext: TransactionContext): Transaction { return callOnHub('startSpan', transactionContext); diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 1eaa95722000..bada7f09d7d9 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -174,12 +174,10 @@ export interface Hub { traceHeaders(): { [key: string]: string }; /** - * This functions starts a span. If there is already a `Span` on the Scope, - * the created Span with the SpanContext will have a reference to it and become it's child. - * Otherwise it'll crete a new `Span`. + * This functions starts either a Span or a Transaction (depending on the argument passed). + * If there is a Span on the Scope we use the `trace_id` for all other created Transactions / Spans as a reference. * - * @param spanContext Properties with which the span should be created - * TODO + * @param spanOrTransactionContext Properties with which the Transaction/Span should be created */ startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Span; } diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 00ac53a9c49d..304307f0715c 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -1,18 +1,18 @@ import { Span, SpanContext } from './span'; /** - * JSDoc TODO + * Interface holding Transaction specific properties */ export interface TransactionContext extends SpanContext { name: string; } /** - * JSDoc TODO + * Transaction "Class", inherits Span only has `setName` */ export interface Transaction extends TransactionContext, Span { /** - * JSDoc TODO + * Set the name of the transaction */ setName(name: string): void; } From b8dc363667a1e5ab2dd7e5394aa6b3bcdfa81827 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 20 May 2020 12:23:55 +0200 Subject: [PATCH 08/28] feat: Added new option `startTraceForUserSession` --- packages/apm/src/integrations/tracing.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 798702c16be0..2b53faaa74d0 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -79,6 +79,14 @@ interface TracingOptions { */ markBackgroundTransactions: boolean; + /** + * Tells the integration set a Span on the scope that is considered the "trace". All succeeding Transactions/Span + * will be part of the same Trace. Turn this off if you want the every Transaction is it's own Trace. + * + * Default: true + */ + startTraceForUserSession: boolean; + /** * This is only if you want to debug in prod. * writeAsBreadcrumbs: Instead of having console.log statements we log messages to breadcrumbs @@ -174,6 +182,7 @@ export class Tracing implements Integration { !isMatchingPattern(url, 'sentry_key') ); }, + startTraceForUserSession: true, startTransactionOnLocationChange: true, traceFetch: true, traceXHR: true, From d44c123f1577f4bda8f9781f0c6e1e51af41a8c2 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 20 May 2020 16:55:59 +0200 Subject: [PATCH 09/28] fix: Typing docs --- packages/apm/src/hubextensions.ts | 2 +- packages/apm/src/integrations/tracing.ts | 8 +++-- packages/apm/src/span.ts | 40 ++++++++++++------------ packages/apm/src/transaction.ts | 2 +- packages/hub/src/hub.ts | 7 +++-- packages/types/src/hub.ts | 4 +-- packages/types/src/span.ts | 6 ++++ 7 files changed, 39 insertions(+), 30 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 28ef8a666ca9..522bb5686853 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -27,7 +27,7 @@ function traceHeaders(): { [key: string]: string } { * * @param spanContext Properties with which the span should be created */ -function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Span { +function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span { // @ts-ignore const hub = this as Hub; const scope = hub.getScope(); diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 2b53faaa74d0..39b9a73a6d69 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -83,7 +83,7 @@ interface TracingOptions { * Tells the integration set a Span on the scope that is considered the "trace". All succeeding Transactions/Span * will be part of the same Trace. Turn this off if you want the every Transaction is it's own Trace. * - * Default: true + * Default: false */ startTraceForUserSession: boolean; @@ -182,7 +182,6 @@ export class Tracing implements Integration { !isMatchingPattern(url, 'sentry_key') ); }, - startTraceForUserSession: true, startTransactionOnLocationChange: true, traceFetch: true, traceXHR: true, @@ -211,7 +210,9 @@ export class Tracing implements Integration { logger.warn(`[Tracing] We added a reasonable default for you: ${defaultTracingOrigins}`); } - Tracing._startTrace(); + if (Tracing.options && Tracing.options.startTraceForUserSession) { + Tracing._startTrace(); + } // Starting pageload transaction if (global.location && global.location.href) { @@ -435,6 +436,7 @@ export class Tracing implements Integration { } const trace = hub.startSpan({}); + Tracing._log(`[Tracing] Starting Trace with id: ${trace.traceId}`); hub.configureScope((scope: Scope) => { scope.setSpan(trace); }); diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index c33951d63504..1c9582c14b27 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -19,22 +19,22 @@ export class Span implements SpanInterface, SpanContext { /** * @inheritDoc */ - private readonly _traceId: string = uuid4(); + public traceId: string = uuid4(); /** * @inheritDoc */ - private readonly _spanId: string = uuid4().substring(16); + public spanId: string = uuid4().substring(16); /** * @inheritDoc */ - private readonly _parentSpanId?: string; + public parentSpanId?: string; /** * Internal keeper of the status */ - private _status?: SpanStatus | string; + public status?: SpanStatus | string; /** * @inheritDoc @@ -87,13 +87,13 @@ export class Span implements SpanInterface, SpanContext { return this; } if (spanContext.traceId) { - this._traceId = spanContext.traceId; + this.traceId = spanContext.traceId; } if (spanContext.spanId) { - this._spanId = spanContext.spanId; + this.spanId = spanContext.spanId; } if (spanContext.parentSpanId) { - this._parentSpanId = spanContext.parentSpanId; + this.parentSpanId = spanContext.parentSpanId; } // We want to include booleans as well here if ('sampled' in spanContext) { @@ -112,7 +112,7 @@ export class Span implements SpanInterface, SpanContext { this.tags = spanContext.tags; } if (spanContext.status) { - this._status = spanContext.status; + this.status = spanContext.status; } if (spanContext.startTimestamp) { this.startTimestamp = spanContext.startTimestamp; @@ -130,9 +130,9 @@ export class Span implements SpanInterface, SpanContext { ): Span { const span = new Span({ ...spanContext, - parentSpanId: this._spanId, + parentSpanId: this.spanId, sampled: this.sampled, - traceId: this._traceId, + traceId: this.traceId, }); span.spanRecorder = this.spanRecorder; @@ -189,7 +189,7 @@ export class Span implements SpanInterface, SpanContext { * @inheritDoc */ public setStatus(value: SpanStatus): this { - this._status = value; + this.status = value; return this; } @@ -209,7 +209,7 @@ export class Span implements SpanInterface, SpanContext { * @inheritDoc */ public isSuccess(): boolean { - return this._status === SpanStatus.Ok; + return this.status === SpanStatus.Ok; } /** @@ -227,7 +227,7 @@ export class Span implements SpanInterface, SpanContext { if (this.sampled !== undefined) { sampledString = this.sampled ? '-1' : '-0'; } - return `${this._traceId}-${this._spanId}${sampledString}`; + return `${this.traceId}-${this.spanId}${sampledString}`; } /** @@ -247,11 +247,11 @@ export class Span implements SpanInterface, SpanContext { data: Object.keys(this.data).length > 0 ? this.data : undefined, description: this.description, op: this.op, - parent_span_id: this._parentSpanId, - span_id: this._spanId, - status: this._status, + parent_span_id: this.parentSpanId, + span_id: this.spanId, + status: this.status, tags: Object.keys(this.tags).length > 0 ? this.tags : undefined, - trace_id: this._traceId, + trace_id: this.traceId, }); } @@ -274,13 +274,13 @@ export class Span implements SpanInterface, SpanContext { data: Object.keys(this.data).length > 0 ? this.data : undefined, description: this.description, op: this.op, - parent_span_id: this._parentSpanId, + parent_span_id: this.parentSpanId, sampled: this.sampled, - span_id: this._spanId, + span_id: this.spanId, start_timestamp: this.startTimestamp, tags: Object.keys(this.tags).length > 0 ? this.tags : undefined, timestamp: this.endTimestamp, - trace_id: this._traceId, + trace_id: this.traceId, }); } } diff --git a/packages/apm/src/transaction.ts b/packages/apm/src/transaction.ts index 1a0bf1e2de0d..85ca35844835 100644 --- a/packages/apm/src/transaction.ts +++ b/packages/apm/src/transaction.ts @@ -96,7 +96,7 @@ export class Transaction extends SpanClass { if (this.sampled !== true) { // At this point if `sampled !== true` we want to discard the transaction. - logger.warn('Discarding transaction because it was not chosen to be sampled.); + logger.warn('Discarding transaction because it was not chosen to be sampled.'); return undefined; } diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 3d95c2c695c4..266607d28686 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -10,8 +10,9 @@ import { Severity, Span, SpanContext, - User, + Transaction, TransactionContext, + User, } from '@sentry/types'; import { consoleSandbox, getGlobalObject, isNodeEnv, logger, timestampWithMs, uuid4 } from '@sentry/utils'; @@ -367,8 +368,8 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Span { - return this._callExtensionMethod('startSpan', spanOrTransactionContext); + public startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span { + return this._callExtensionMethod('startSpan', spanOrTransactionContext); } /** diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index bada7f09d7d9..72ffa313074e 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -5,8 +5,8 @@ import { Integration, IntegrationClass } from './integration'; import { Scope } from './scope'; import { Severity } from './severity'; import { Span, SpanContext } from './span'; +import { Transaction, TransactionContext } from './transaction'; import { User } from './user'; -import { TransactionContext } from './transaction'; /** * Internal class used to make sure we always have the latest internal functions @@ -179,5 +179,5 @@ export interface Hub { * * @param spanOrTransactionContext Properties with which the Transaction/Span should be created */ - startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Span; + startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span; } diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index dec66fe0858e..c2c080e41653 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -4,27 +4,33 @@ export interface SpanContext { * Description of the Span. */ description?: string; + /** * Operation of the Span. */ op?: string; + /** * Completion status of the Span. * See: {@sentry/apm SpanStatus} for possible values */ status?: string; + /** * Parent Span ID */ parentSpanId?: string; + /** * Was this span chosen to be sent as part of the sample? */ sampled?: boolean; + /** * Span ID */ spanId?: string; + /** * Trace ID */ From c6338cf68a870c346f60786d1b8d91e9641a099c Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 20 May 2020 16:57:46 +0200 Subject: [PATCH 10/28] ref: Remove trace option --- packages/apm/src/integrations/tracing.ts | 33 ------------------------ 1 file changed, 33 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 39b9a73a6d69..4db7dfc0fff3 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -79,14 +79,6 @@ interface TracingOptions { */ markBackgroundTransactions: boolean; - /** - * Tells the integration set a Span on the scope that is considered the "trace". All succeeding Transactions/Span - * will be part of the same Trace. Turn this off if you want the every Transaction is it's own Trace. - * - * Default: false - */ - startTraceForUserSession: boolean; - /** * This is only if you want to debug in prod. * writeAsBreadcrumbs: Instead of having console.log statements we log messages to breadcrumbs @@ -210,10 +202,6 @@ export class Tracing implements Integration { logger.warn(`[Tracing] We added a reasonable default for you: ${defaultTracingOrigins}`); } - if (Tracing.options && Tracing.options.startTraceForUserSession) { - Tracing._startTrace(); - } - // Starting pageload transaction if (global.location && global.location.href) { // Use `${global.location.href}` as transaction name @@ -421,27 +409,6 @@ export class Tracing implements Integration { logger.log(args); } - /** - * Puts a span on the Scope as the "trace" - */ - private static _startTrace(): void { - const _getCurrentHub = Tracing._getCurrentHub; - if (!_getCurrentHub) { - return; - } - - const hub = _getCurrentHub(); - if (!hub) { - return; - } - - const trace = hub.startSpan({}); - Tracing._log(`[Tracing] Starting Trace with id: ${trace.traceId}`); - hub.configureScope((scope: Scope) => { - scope.setSpan(trace); - }); - } - /** * Starts a Transaction waiting for activity idle to finish */ From 6556ae7b6812b4737360bcc29051dcd1b656bbf3 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 09:14:03 +0200 Subject: [PATCH 11/28] Update packages/types/src/span.ts Co-authored-by: Katie Byers --- 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 c2c080e41653..375f618be36c 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -52,7 +52,7 @@ export interface SpanContext { startTimestamp?: number; /** - * Timestamp in seconds when the span ended. + * Timestamp in seconds (epoch time) indicating when the span ended. */ endTimestamp?: number; } From 0631c5a6e988bd24223cff6645649641906f4012 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 10:30:26 +0200 Subject: [PATCH 12/28] fix: Node, Add startChild --- packages/apm/src/integrations/tracing.ts | 2 +- packages/apm/src/span.ts | 10 +++++ packages/hub/src/scope.ts | 3 -- packages/node/src/handlers.ts | 50 +++++++++++++++++------- packages/node/src/integrations/http.ts | 15 +++++-- packages/types/src/span.ts | 10 ++++- 6 files changed, 66 insertions(+), 24 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 4db7dfc0fff3..6e6162bde477 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -1,4 +1,4 @@ -import { Hub, Scope } from '@sentry/hub'; +import { Hub } from '@sentry/hub'; import { Event, EventProcessor, Integration, Severity, Span, SpanContext } from '@sentry/types'; import { addInstrumentationHandler, diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 1c9582c14b27..113580edcdb1 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -124,9 +124,19 @@ export class Span implements SpanInterface, SpanContext { /** * @inheritDoc + * @deprecated */ public child( spanContext?: Pick>, + ): Span { + return this.startChild(spanContext); + } + + /** + * @inheritDoc + */ + public startChild( + spanContext?: Pick>, ): Span { const span = new Span({ ...spanContext, diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index c23d49318d92..b0daa943c248 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -185,9 +185,6 @@ export class Scope implements ScopeInterface { */ public setTransaction(transaction?: string): this { this._transaction = transaction; - if (this._span) { - (this._span as any).transaction = transaction; - } this._notifyScopeListeners(); return this; } diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index d5a1a2007fed..37e5721d6c00 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -1,5 +1,5 @@ import { Span } from '@sentry/apm'; -import { captureException, getCurrentHub, withScope } from '@sentry/core'; +import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core'; import { Event } from '@sentry/types'; import { forget, isPlainObject, isString, logger, normalize } from '@sentry/utils'; import * as cookie from 'cookie'; @@ -32,19 +32,42 @@ export function tracingHandler(): ( const reqMethod = (req.method || '').toUpperCase(); const reqUrl = req.url; - const hub = getCurrentHub(); - const transaction = hub.startSpan({ + let traceId; + let parentSpanId; + + // If there is a trace header set, we extract the data from it and set the span on the scope + // to be the origin an created transaction set the parent_span_id / trace_id + if (req.headers && isString(req.headers['sentry-trace'])) { + const span = Span.fromTraceparent(req.headers['sentry-trace'] as string); + if (span) { + const spanData = span.toJSON(); + traceId = spanData.trace_id; + parentSpanId = spanData.span_id; + } + } + + const transaction = startTransaction({ + name: `${reqMethod} ${reqUrl}`, op: 'http.server', - transaction: `${reqMethod} ${reqUrl}`, + parentSpanId, + traceId, }); - hub.configureScope(scope => { - scope.setSpan(transaction); - }); + if (transaction) { + // We put the transaction on the scope so users can attach children to it + getCurrentHub().configureScope(scope => { + scope.setSpan(transaction); + }); + // We also set a function getTransaction on the response so people can grab the transaction there to add + // spans to it + (res as any).getTransaction = () => transaction; + } res.once('finish', () => { - transaction.setHttpStatus(res.statusCode); - transaction.finish(); + if (transaction) { + transaction.setHttpStatus(res.statusCode); + transaction.finish(); + } }); next(); @@ -280,6 +303,7 @@ export function parseRequest( } if (options.transaction && !event.transaction) { + // TODO: Use the real transaction here const transaction = extractTransaction(req, options.transaction); if (transaction) { event.transaction = transaction; @@ -369,18 +393,14 @@ export function errorHandler(options?: { ) => void { return function sentryErrorMiddleware( error: MiddlewareError, - req: http.IncomingMessage, + _req: http.IncomingMessage, res: http.ServerResponse, next: (error: MiddlewareError) => void, ): void { const shouldHandleError = (options && options.shouldHandleError) || defaultShouldHandleError; if (shouldHandleError(error)) { - withScope(scope => { - if (req.headers && isString(req.headers['sentry-trace'])) { - const span = Span.fromTraceparent(req.headers['sentry-trace'] as string); - scope.setSpan(span); - } + withScope(_scope => { const eventId = captureException(error); (res as any).sentry = eventId; next(error); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 63b0a7ccb6ca..7c6f6b932252 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,5 +1,5 @@ import { getCurrentHub } from '@sentry/core'; -import { Integration, Span } from '@sentry/types'; +import { Integration, Span, Transaction } from '@sentry/types'; import { fill, parseSemver } from '@sentry/utils'; import * as http from 'http'; import * as https from 'https'; @@ -78,9 +78,16 @@ function createHandlerWrapper( return originalHandler.apply(this, arguments); } - let span: Span; - if (tracingEnabled) { - span = getCurrentHub().startSpan({ + let span: Span | undefined; + let transaction: Transaction | undefined; + + const scope = getCurrentHub().getScope(); + if (scope) { + transaction = scope.getSpan() as Transaction; + } + + if (tracingEnabled && transaction) { + span = transaction.startChild({ description: `${typeof options === 'string' || !options.method ? 'GET' : options.method} ${requestUrl}`, op: 'request', }); diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 375f618be36c..b2ab36f194f4 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -120,11 +120,19 @@ export interface Span extends SpanContext { */ setHttpStatus(httpStatus: number): this; + /** + * Use {@link startChild} + * @deprecated + */ + child( + spanContext?: Pick>, + ): Span; + /** * Creates a new `Span` while setting the current `Span.id` as `parentSpanId`. * Also the `sampled` decision will be inherited. */ - child( + startChild( spanContext?: Pick>, ): Span; From 20ec671e22b0e129e0168fc7ad090b3af05a88f8 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 12:26:48 +0200 Subject: [PATCH 13/28] fix: Tests --- packages/apm/src/index.ts | 1 + packages/apm/test/hub.test.ts | 27 +++-- packages/apm/test/span.test.ts | 181 ++++++++++++++------------------- 3 files changed, 90 insertions(+), 119 deletions(-) diff --git a/packages/apm/src/index.ts b/packages/apm/src/index.ts index 3e021fb5d210..9f59e3515f40 100644 --- a/packages/apm/src/index.ts +++ b/packages/apm/src/index.ts @@ -3,6 +3,7 @@ import * as ApmIntegrations from './integrations'; export { ApmIntegrations as Integrations }; export { Span, TRACEPARENT_REGEXP } from './span'; +export { Transaction } from './transaction'; export { SpanStatus } from './spanstatus'; diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index cd8f864ab9ee..a146ce2a5b51 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -15,23 +15,23 @@ describe('Hub', () => { 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); + 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); + const transaction = hub.startSpan({ name: 'foo' }) as any; + expect(transaction.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(); + const transaction = hub.startSpan({ name: 'foo' }) as any; + expect(transaction.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 }); + const transaction = hub.startSpan({ name: 'foo' }) as any; + const child = transaction.startChild({ op: 1 }); expect(child.sampled).toBeFalsy(); }); }); @@ -39,20 +39,19 @@ describe('Hub', () => { describe('start', () => { test('simple', () => { const hub = new Hub(new BrowserClient()); - const span = hub.startSpan() as any; - expect(span._spanId).toBeTruthy(); + const span = hub.startSpan({}) as any; + expect(span.spanId).toBeTruthy(); }); - test('inherits from parent span', () => { + test('transaction inherits trace_id from span on scope', () => { const myScope = new Scope(); const hub = new Hub(new BrowserClient(), myScope); const parentSpan = hub.startSpan({}) as any; - expect(parentSpan._parentId).toBeFalsy(); hub.configureScope(scope => { scope.setSpan(parentSpan); }); - const span = hub.startSpan({}) as any; - expect(span._parentSpanId).toBeTruthy(); + const span = hub.startSpan({ name: 'test' }) as any; + expect(span.trace_id).toEqual(parentSpan.trace_id); }); }); }); diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index cecca058a614..2bfa32a8ebe1 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -1,7 +1,7 @@ import { BrowserClient } from '@sentry/browser'; import { Hub, Scope } from '@sentry/hub'; -import { Span, SpanStatus, TRACEPARENT_REGEXP } from '../src'; +import { Span, SpanStatus, TRACEPARENT_REGEXP, Transaction } from '../src'; describe('Span', () => { let hub: Hub; @@ -11,20 +11,37 @@ describe('Span', () => { hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }), myScope); }); - describe('newSpan', () => { + describe('new Span', () => { test('simple', () => { const span = new Span({ sampled: true }); - const span2 = span.child(); - expect((span2 as any)._parentSpanId).toBe((span as any)._spanId); - expect((span2 as any)._traceId).toBe((span as any)._traceId); + const span2 = span.startChild(); + expect((span2 as any).parentSpanId).toBe((span as any).spanId); + expect((span2 as any).traceId).toBe((span as any).traceId); expect((span2 as any).sampled).toBe((span as any).sampled); }); + }); + + describe('new Transaction', () => { + test('simple', () => { + const transaction = new Transaction({ name: 'test', sampled: true }); + const span2 = transaction.startChild(); + expect((span2 as any).parentSpanId).toBe((transaction as any).spanId); + expect((span2 as any).traceId).toBe((transaction as any).traceId); + expect((span2 as any).sampled).toBe((transaction as any).sampled); + }); test('gets currentHub', () => { - const span = new Span({}); - const span2 = span.child(); - expect((span as any)._hub).toBeInstanceOf(Hub); - expect((span2 as any)._hub).toBeInstanceOf(Hub); + const transaction = new Transaction({ name: 'test' }); + expect((transaction as any)._hub).toBeInstanceOf(Hub); + }); + + test('inherit span list', () => { + const transaction = new Transaction({ name: 'test', sampled: true }); + const span2 = transaction.startChild(); + const span3 = span2.startChild(); + span3.finish(); + expect(transaction.spanRecorder).toBe(span2.spanRecorder); + expect(transaction.spanRecorder).toBe(span3.spanRecorder); }); }); @@ -74,32 +91,6 @@ describe('Span', () => { }); }); - describe('newSpan', () => { - test('simple', () => { - const span = new Span({ sampled: true }); - const span2 = span.child(); - expect((span2 as any)._parentSpanId).toBe((span as any)._spanId); - expect((span2 as any)._traceId).toBe((span as any)._traceId); - expect((span2 as any).sampled).toBe((span as any).sampled); - }); - - test('gets currentHub', () => { - const span = new Span({}); - const span2 = span.child(); - 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', () => { test('simple', () => { expect(new Span().toTraceparent()).toMatch(TRACEPARENT_REGEXP); @@ -113,9 +104,9 @@ describe('Span', () => { test('no sample', () => { const from = Span.fromTraceparent('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb') as any; - expect(from._parentSpanId).toEqual('bbbbbbbbbbbbbbbb'); - expect(from._traceId).toEqual('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'); - expect(from._spanId).not.toEqual('bbbbbbbbbbbbbbbb'); + expect(from.parentSpanId).toEqual('bbbbbbbbbbbbbbbb'); + expect(from.traceId).toEqual('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'); + expect(from.spanId).not.toEqual('bbbbbbbbbbbbbbbb'); expect(from.sampled).toBeUndefined(); }); test('sample true', () => { @@ -130,13 +121,13 @@ describe('Span', () => { test('just sample rate', () => { const from = Span.fromTraceparent('0') as any; - expect(from._traceId).toHaveLength(32); - expect(from._spanId).toHaveLength(16); + expect(from.traceId).toHaveLength(32); + expect(from.spanId).toHaveLength(16); expect(from.sampled).toBeFalsy(); const from2 = Span.fromTraceparent('1') as any; - expect(from2._traceId).toHaveLength(32); - expect(from2._spanId).toHaveLength(16); + expect(from2.traceId).toHaveLength(32); + expect(from2.spanId).toHaveLength(16); expect(from2.sampled).toBeTruthy(); }); @@ -156,7 +147,7 @@ describe('Span', () => { test('with parent', () => { const spanA = new Span({ traceId: 'a', spanId: 'b' }) as any; - const spanB = new Span({ traceId: 'c', spanId: 'd', sampled: false, parentSpanId: spanA._spanId }); + const spanB = new Span({ traceId: 'c', spanId: 'd', sampled: false, parentSpanId: spanA.spanId }); const serialized = JSON.parse(JSON.stringify(spanB)); expect(serialized).toHaveProperty('parent_span_id', 'b'); expect(serialized).toHaveProperty('span_id', 'd'); @@ -166,7 +157,7 @@ describe('Span', () => { test('should drop all `undefined` values', () => { const spanA = new Span({ traceId: 'a', spanId: 'b' }) as any; const spanB = new Span({ - parentSpanId: spanA._spanId, + parentSpanId: spanA.spanId, sampled: false, spanId: 'd', traceId: 'c', @@ -192,74 +183,54 @@ describe('Span', () => { }); describe('hub.startSpan', () => { - test('finish a span', () => { + test('finish a transaction', () => { const spy = jest.spyOn(hub as any, 'captureEvent') as any; - const span = hub.startSpan(); - span.finish(); + const transaction = hub.startSpan({ name: 'test' }); + transaction.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()); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext()); }); - test('finish a span with transaction + child span', () => { + test('finish a transaction + child span', () => { const spy = jest.spyOn(hub as any, 'captureEvent') as any; - const parentSpan = hub.startSpan(); - const childSpan = parentSpan.child(); + const transaction = hub.startSpan({ name: 'test' }); + const childSpan = transaction.startChild(); childSpan.finish(); - parentSpan.finish(); + transaction.finish(); expect(spy).toHaveBeenCalled(); expect(spy.mock.calls[0][0].spans).toHaveLength(1); - expect(spy.mock.calls[0][0].contexts.trace).toEqual(parentSpan.getTraceContext()); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.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(); + const transaction = hub.startSpan({ name: 'test' }); + const childSpan = transaction.startChild(); 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); - }); - - const spanTwo = hub.startSpan(); - spanTwo.finish(); - - 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.endTimestamp)).toHaveLength(2); - }); - 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(); + const transaction = hub.startSpan({ name: 'test' }); + const childSpanOne = transaction.startChild(); childSpanOne.finish(); hub.configureScope(scope => { - scope.setSpan(spanOne); + scope.setSpan(childSpanOne); }); - const spanTwo = hub.startSpan(); + const spanTwo = transaction.startChild(); spanTwo.finish(); - spanOne.finish(); + transaction.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].contexts.trace).toEqual(transaction.getTraceContext()); }); test('span child limit', () => { @@ -270,33 +241,33 @@ describe('Span', () => { }), ); const spy = jest.spyOn(_hub as any, 'captureEvent') as any; - const span = _hub.startSpan(); + const transaction = _hub.startSpan({ name: 'test' }); for (let i = 0; i < 10; i++) { - const child = span.child(); + const child = transaction.startChild(); child.finish(); } - span.finish(); + transaction.finish(); expect(spy.mock.calls[0][0].spans).toHaveLength(3); }); - test('if we sampled the parent (transaction) we do not want any childs', () => { + test('if we sampled the transaction we do not want any children', () => { const _hub = new Hub( new BrowserClient({ tracesSampleRate: 0, }), ); const spy = jest.spyOn(_hub as any, 'captureEvent') as any; - const span = _hub.startSpan(); + const transaction = _hub.startSpan({ name: 'test' }); for (let i = 0; i < 10; i++) { - const child = span.child(); + const child = transaction.startChild(); child.finish(); } - span.finish(); - expect((span as any).spanRecorder).toBeUndefined(); + transaction.finish(); + expect((transaction as any).spanRecorder).toBeUndefined(); expect(spy).not.toHaveBeenCalled(); }); - test('mixing hub.startSpan + span.child + maxSpans', () => { + test('mixing hub.startSpan(transaction) + span.startChild + maxSpans', () => { const _hub = new Hub( new BrowserClient({ _experiments: { maxSpans: 2 }, @@ -305,21 +276,21 @@ describe('Span', () => { ); const spy = jest.spyOn(_hub as any, 'captureEvent') as any; - const spanOne = _hub.startSpan(); - const childSpanOne = spanOne.child({ op: '1' }); + const transaction = _hub.startSpan({ name: 'test' }); + const childSpanOne = transaction.startChild({ op: '1' }); childSpanOne.finish(); _hub.configureScope(scope => { - scope.setSpan(spanOne); + scope.setSpan(transaction); }); - const spanTwo = _hub.startSpan({ op: '2' }); + const spanTwo = transaction.startChild({ op: '2' }); spanTwo.finish(); - const spanThree = _hub.startSpan({ op: '3' }); + const spanThree = transaction.startChild({ op: '3' }); spanThree.finish(); - spanOne.finish(); + transaction.finish(); expect(spy).toHaveBeenCalled(); expect(spy.mock.calls[0][0].spans).toHaveLength(2); @@ -328,28 +299,28 @@ describe('Span', () => { 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 transaction = hub.startSpan({ name: 'test' }); + const childSpanOne = transaction.startChild(); - const childSpanTwo = childSpanOne.child(); + const childSpanTwo = childSpanOne.startChild(); childSpanTwo.finish(); childSpanOne.finish(); hub.configureScope(scope => { - scope.setSpan(spanOne); + scope.setSpan(transaction); }); - const spanTwo = hub.startSpan(); + const spanTwo = transaction.startChild({}); spanTwo.finish(); - spanOne.finish(); + transaction.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(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext()); + expect(childSpanOne.toJSON().parent_span_id).toEqual(transaction.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); + expect(spanTwo.toJSON().parent_span_id).toEqual(transaction.toJSON().span_id); }); }); }); From da67297199c3bfd8dfe02334073ccd96ef27f5af Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 12:35:19 +0200 Subject: [PATCH 14/28] fix: Remove trace from apply to event --- packages/hub/src/scope.ts | 3 --- packages/hub/test/scope.test.ts | 32 -------------------------------- 2 files changed, 35 deletions(-) diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index b0daa943c248..d64923dbf19e 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -329,9 +329,6 @@ export class Scope implements ScopeInterface { if (this._transaction) { event.transaction = this._transaction; } - if (this._span && event.type === 'transaction') { - event.contexts = { trace: this._span.getTraceContext(), ...event.contexts }; - } this._applyFingerprint(event); diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index 73f4a48e7c4f..3628961d0409 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -245,38 +245,6 @@ describe('Scope', () => { }); }); - test('applyToEvent trace context', async () => { - expect.assertions(1); - const scope = new Scope(); - const span = { - fake: 'span', - getTraceContext: () => ({ a: 'b' }), - } as any; - scope.setSpan(span); - const event: Event = {}; - return scope.applyToEvent(event).then(processedEvent => { - expect((processedEvent!.contexts!.trace as any).a).toEqual('b'); - }); - }); - - test('applyToEvent existing trace context in event should be stronger', async () => { - expect.assertions(1); - const scope = new Scope(); - const span = { - fake: 'span', - getTraceContext: () => ({ a: 'b' }), - } as any; - scope.setSpan(span); - const event: Event = { - contexts: { - trace: { a: 'c' }, - }, - }; - return scope.applyToEvent(event).then(processedEvent => { - expect((processedEvent!.contexts!.trace as any).a).toEqual('c'); - }); - }); - test('clear', () => { const scope = new Scope(); scope.setExtra('a', 2); From b5f16d927b01175e026cfb01a5e729a8878b4acd Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 12:58:40 +0200 Subject: [PATCH 15/28] fix: Lint errors + Vue Integration --- packages/apm/src/integrations/express.ts | 84 +++++++++++++++--------- packages/apm/src/integrations/tracing.ts | 21 ++++-- packages/integrations/src/vue.ts | 15 +++-- 3 files changed, 78 insertions(+), 42 deletions(-) diff --git a/packages/apm/src/integrations/express.ts b/packages/apm/src/integrations/express.ts index 1caa3f681390..fc19feac1d0d 100644 --- a/packages/apm/src/integrations/express.ts +++ b/packages/apm/src/integrations/express.ts @@ -1,4 +1,4 @@ -import { EventProcessor, Hub, Integration } from '@sentry/types'; +import { Integration } from '@sentry/types'; import { logger } from '@sentry/utils'; // tslint:disable-next-line:no-implicit-dependencies import { Application, ErrorRequestHandler, NextFunction, Request, RequestHandler, Response } from 'express'; @@ -35,12 +35,12 @@ export class Express implements Integration { /** * @inheritDoc */ - public setupOnce(_addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { + public setupOnce(): void { if (!this._app) { logger.error('ExpressIntegration is missing an Express instance'); return; } - instrumentMiddlewares(this._app, getCurrentHub); + instrumentMiddlewares(this._app); } } @@ -56,42 +56,66 @@ export class Express implements Integration { * // error handler * app.use(function (err, req, res, next) { ... }) */ -function wrap(fn: Function, getCurrentHub: () => Hub): RequestHandler | ErrorRequestHandler { +function wrap(fn: Function): RequestHandler | ErrorRequestHandler { const arrity = fn.length; switch (arrity) { case 2: { return function(this: NodeJS.Global, _req: Request, res: Response): any { - const span = getCurrentHub().startSpan({ - description: fn.name, - op: 'middleware', - }); - res.once('finish', () => span.finish()); + // tslint:disable: no-unsafe-any + const transaction = (res as any).getTransaction && (res as any).getTransaction(); + if (transaction) { + const span = transaction.startChild({ + description: fn.name, + op: 'middleware', + }); + res.once('finish', () => { + span.finish(); + }); + } + // tslint:enable: no-unsafe-any return fn.apply(this, arguments); }; } case 3: { return function(this: NodeJS.Global, req: Request, res: Response, next: NextFunction): any { - const span = getCurrentHub().startSpan({ - description: fn.name, - op: 'middleware', - }); - fn.call(this, req, res, function(this: NodeJS.Global): any { - span.finish(); - return next.apply(this, arguments); - }); + // tslint:disable: no-unsafe-any + const transaction = (res as any).getTransaction && (res as any).getTransaction(); + if (transaction) { + const span = transaction.startChild({ + description: fn.name, + op: 'middleware', + }); + fn.call(this, req, res, function(this: NodeJS.Global): any { + span.finish(); + return next.apply(this, arguments); + }); + } else { + fn.call(this, req, res, function(this: NodeJS.Global): any { + return next.apply(this, arguments); + }); + } + // tslint:enable: no-unsafe-any }; } case 4: { return function(this: NodeJS.Global, err: any, req: Request, res: Response, next: NextFunction): any { - const span = getCurrentHub().startSpan({ - description: fn.name, - op: 'middleware', - }); - fn.call(this, err, req, res, function(this: NodeJS.Global): any { - span.finish(); - return next.apply(this, arguments); - }); + // tslint:disable: no-unsafe-any + const transaction = (res as any).getTransaction && (res as any).getTransaction(); + if (transaction) { + const span = transaction.startChild({ + description: fn.name, + op: 'middleware', + }); + fn.call(this, err, req, res, function(this: NodeJS.Global): any { + span.finish(); + return next.apply(this, arguments); + }); + } else { + fn.call(this, err, req, res, function(this: NodeJS.Global): any { + return next.apply(this, arguments); + }); + } }; } default: { @@ -110,16 +134,16 @@ function wrap(fn: Function, getCurrentHub: () => Hub): RequestHandler | ErrorReq * app.use([], , ...) * app.use([], ...[]) */ -function wrapUseArgs(args: IArguments, getCurrentHub: () => Hub): unknown[] { +function wrapUseArgs(args: IArguments): unknown[] { return Array.from(args).map((arg: unknown) => { if (typeof arg === 'function') { - return wrap(arg, getCurrentHub); + return wrap(arg); } if (Array.isArray(arg)) { return arg.map((a: unknown) => { if (typeof a === 'function') { - return wrap(a, getCurrentHub); + return wrap(a); } return a; }); @@ -132,10 +156,10 @@ function wrapUseArgs(args: IArguments, getCurrentHub: () => Hub): unknown[] { /** * Patches original app.use to utilize our tracing functionality */ -function instrumentMiddlewares(app: Application, getCurrentHub: () => Hub): Application { +function instrumentMiddlewares(app: Application): Application { const originalAppUse = app.use; app.use = function(): any { - return originalAppUse.apply(this, wrapUseArgs(arguments, getCurrentHub)); + return originalAppUse.apply(this, wrapUseArgs(arguments)); }; return app; } diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 6e6162bde477..fa859bb9d037 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -477,7 +477,7 @@ export class Tracing implements Integration { // tslint:disable-next-line: completed-docs function addPerformanceNavigationTiming(parent: SpanClass, entry: { [key: string]: number }, event: string): void { - const span = parent.child({ + const span = parent.startChild({ description: event, op: 'browser', }); @@ -487,14 +487,14 @@ export class Tracing implements Integration { // tslint:disable-next-line: completed-docs function addRequest(parent: SpanClass, entry: { [key: string]: number }): void { - const request = parent.child({ + const request = parent.startChild({ description: 'request', op: 'browser', }); request.startTimestamp = timeOrigin + Tracing._msToSec(entry.requestStart); request.endTimestamp = timeOrigin + Tracing._msToSec(entry.responseEnd); - const response = parent.child({ + const response = parent.startChild({ description: 'response', op: 'browser', }); @@ -544,7 +544,7 @@ export class Tracing implements Integration { case 'mark': case 'paint': case 'measure': - const mark = transactionSpan.child({ + const mark = transactionSpan.startChild({ description: entry.name, op: entry.entryType, }); @@ -567,7 +567,7 @@ export class Tracing implements Integration { }); } } else { - const resource = transactionSpan.child({ + const resource = transactionSpan.startChild({ description: `${entry.initiatorType} ${resourceName}`, op: `resource`, }); @@ -585,7 +585,7 @@ export class Tracing implements Integration { }); if (entryScriptStartEndTime !== undefined && tracingInitMarkStartTime !== undefined) { - const evaluation = transactionSpan.child({ + const evaluation = transactionSpan.startChild({ description: 'evaluation', op: `script`, }); @@ -609,6 +609,13 @@ export class Tracing implements Integration { } } + /** + * Returns the current active idle transaction if there is one + */ + public static getTransaction(): Transaction | undefined { + return Tracing._activeTransaction; + } + /** * Converts from milliseconds to seconds * @param time time in ms @@ -668,7 +675,7 @@ export class Tracing implements Integration { if (spanContext && _getCurrentHub) { const hub = _getCurrentHub(); if (hub) { - const span = activeTransaction.child(spanContext); + const span = activeTransaction.startChild(spanContext); Tracing._activities[Tracing._currentIndex] = { name, span, diff --git a/packages/integrations/src/vue.ts b/packages/integrations/src/vue.ts index 889a0e854516..3e10dc11fc22 100644 --- a/packages/integrations/src/vue.ts +++ b/packages/integrations/src/vue.ts @@ -238,11 +238,16 @@ export class Vue implements Integration { if (tracingIntegration) { // tslint:disable-next-line:no-unsafe-any this._tracingActivity = (tracingIntegration as any).constructor.pushActivity('Vue Application Render'); + // tslint:disable-next-line:no-unsafe-any + const transaction = (tracingIntegration as any).constructor.getTransaction(); + if (transaction) { + // tslint:disable-next-line:no-unsafe-any + this._rootSpan = transaction.startChild({ + description: 'Application Render', + op: 'Vue', + }); + } } - this._rootSpan = getCurrentHub().startSpan({ - description: 'Application Render', - op: 'Vue', - }); }); } }; @@ -269,7 +274,7 @@ export class Vue implements Integration { } else { vm.$once(`hook:${hook}`, () => { if (this._rootSpan) { - spans[op] = this._rootSpan.child({ + spans[op] = this._rootSpan.startChild({ description: `Vue <${name}>`, op, }); From 0fd1fabbd0e36c5ecea43e0d067f2f23efe27ec5 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 13:09:34 +0200 Subject: [PATCH 16/28] meta: Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17a5f8edae40..e74f311a3c6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - [core] fix: sent_at for envelope headers to use same clock #2597 - [apm] fix: Improve bundle size by moving span status to @sentry/apm #2589 - [apm] feat: No longer discard transactions instead mark them deadline exceeded #2588 +- [apm] feat: Introduce `Sentry.startTransaction` and `startChild` #2600 ## 5.15.5 From 78ecad8219d8ab6609ec0d37e1e3d4eca170a887 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 13:49:32 +0200 Subject: [PATCH 17/28] fix: Vue integration --- packages/apm/src/integrations/tracing.ts | 2 +- packages/integrations/src/vue.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index fa859bb9d037..498aa9ae3852 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -406,7 +406,7 @@ export class Tracing implements Integration { return; } } - logger.log(args); + logger.log(...args); } /** diff --git a/packages/integrations/src/vue.ts b/packages/integrations/src/vue.ts index 3e10dc11fc22..e2ed67739947 100644 --- a/packages/integrations/src/vue.ts +++ b/packages/integrations/src/vue.ts @@ -305,9 +305,6 @@ export class Vue implements Integration { } this._rootSpanTimer = setTimeout(() => { - if (this._rootSpan) { - ((this._rootSpan as unknown) as { timestamp: number }).timestamp = timestamp; - } if (this._tracingActivity) { // We do this whole dance with `TRACING_GETTER` to prevent `@sentry/apm` from becoming a peerDependency. // We also need to ask for the `.constructor`, as `pushActivity` and `popActivity` are static, not instance methods. @@ -315,6 +312,9 @@ export class Vue implements Integration { if (tracingIntegration) { // tslint:disable-next-line:no-unsafe-any (tracingIntegration as any).constructor.popActivity(this._tracingActivity); + if (this._rootSpan) { + this._rootSpan.finish(timestamp); + } } } }, this._options.tracingOptions.timeout); From f088aad24f3aebb7afa20dba3fca2fcbb09b9375 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 25 May 2020 16:56:55 +0200 Subject: [PATCH 18/28] fix: Transaction interface --- packages/apm/src/transaction.ts | 3 +++ packages/types/src/transaction.ts | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/packages/apm/src/transaction.ts b/packages/apm/src/transaction.ts index 85ca35844835..a994aadc4959 100644 --- a/packages/apm/src/transaction.ts +++ b/packages/apm/src/transaction.ts @@ -81,6 +81,9 @@ export class Transaction extends SpanClass { /** * Sets the finish timestamp on the current span. + * + * @inheritdoc + * * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming * the duration of the transaction. This is useful to discard extra time in the transaction that is not * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 304307f0715c..637439a3b2a8 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -15,4 +15,16 @@ export interface Transaction extends TransactionContext, Span { * Set the name of the transaction */ setName(name: string): void; + + /** + * Sets the finish timestamp on the current span. + * + * @inheritdoc + * + * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming + * the duration of the transaction. This is useful to discard extra time in the transaction that is not + * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the + * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. + */ + finish(endTimestamp?: number, trimEnd?: boolean): string | undefined; } From 46d25264f7224f75b633bb6c24647e7f21515eab Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 09:31:13 +0200 Subject: [PATCH 19/28] ref: CodeReview --- CHANGELOG.md | 2 +- packages/apm/src/hubextensions.ts | 14 ++-- packages/apm/src/integrations/express.ts | 74 ++++++++++++---------- packages/apm/src/integrations/tracing.ts | 20 +++--- packages/apm/src/transaction.ts | 19 +++--- packages/hub/src/hub.ts | 4 +- packages/node/src/handlers.ts | 23 +++---- packages/types/src/hub.ts | 4 +- packages/types/src/span.ts | 81 ++++++++++++++++-------- packages/types/src/transaction.ts | 42 ++++++++---- 10 files changed, 164 insertions(+), 119 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e74f311a3c6f..6e9d28681cdc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ - [core] fix: sent_at for envelope headers to use same clock #2597 - [apm] fix: Improve bundle size by moving span status to @sentry/apm #2589 - [apm] feat: No longer discard transactions instead mark them deadline exceeded #2588 -- [apm] feat: Introduce `Sentry.startTransaction` and `startChild` #2600 +- [apm] feat: Introduce `Sentry.startTransaction` and `Transaction.startChild` #2600 ## 5.15.5 diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 522bb5686853..9664e97b96eb 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -25,27 +25,25 @@ function traceHeaders(): { [key: string]: string } { * the created Span with the SpanContext will have a reference to it and become it's child. * Otherwise it'll crete a new `Span`. * - * @param spanContext Properties with which the span should be created + * @param context Properties with which the span should be created */ -function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span { +function startSpan(context: SpanContext | TransactionContext): Transaction | Span { // @ts-ignore const hub = this as Hub; const scope = hub.getScope(); const client = hub.getClient(); - let newSpanContext = spanOrTransactionContext; + let newSpanContext = context; - // We create an empty Span in case there is no scope on the hub - let parentSpan = new Span(); if (scope) { // If there is a Span on the Scope we use the span_id / trace_id // To define the parent <-> child relationship - parentSpan = scope.getSpan() as Span; + const parentSpan = scope.getSpan(); if (parentSpan) { const { trace_id } = parentSpan.getTraceContext(); newSpanContext = { traceId: trace_id, - ...spanOrTransactionContext, + ...context, }; } } @@ -57,7 +55,7 @@ function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): // We only roll the dice on sampling for root spans of transactions because all child spans inherit this state if (transaction.sampled === undefined) { const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; - transaction.sampled = Math.random() < sampleRate; + transaction.sampled = Math.random() <= sampleRate; } // We only want to create a span list if we sampled the transaction diff --git a/packages/apm/src/integrations/express.ts b/packages/apm/src/integrations/express.ts index fc19feac1d0d..ba189faaf21f 100644 --- a/packages/apm/src/integrations/express.ts +++ b/packages/apm/src/integrations/express.ts @@ -1,8 +1,12 @@ -import { Integration } from '@sentry/types'; +import { Integration, Transaction } from '@sentry/types'; import { logger } from '@sentry/utils'; // tslint:disable-next-line:no-implicit-dependencies import { Application, ErrorRequestHandler, NextFunction, Request, RequestHandler, Response } from 'express'; +interface SentryTracingResponse { + __sentry_transaction?: Transaction; +} + /** * Express integration * @@ -61,61 +65,65 @@ function wrap(fn: Function): RequestHandler | ErrorRequestHandler { switch (arrity) { case 2: { - return function(this: NodeJS.Global, _req: Request, res: Response): any { - // tslint:disable: no-unsafe-any - const transaction = (res as any).getTransaction && (res as any).getTransaction(); + return function(this: NodeJS.Global, _req: Request, res: Response & SentryTracingResponse): any { + const transaction = res.__sentry_transaction; if (transaction) { const span = transaction.startChild({ description: fn.name, op: 'middleware', }); res.once('finish', () => { - span.finish(); + if (span) { + span.finish(); + } }); } - // tslint:enable: no-unsafe-any return fn.apply(this, arguments); }; } case 3: { - return function(this: NodeJS.Global, req: Request, res: Response, next: NextFunction): any { - // tslint:disable: no-unsafe-any - const transaction = (res as any).getTransaction && (res as any).getTransaction(); - if (transaction) { - const span = transaction.startChild({ + return function( + this: NodeJS.Global, + req: Request, + res: Response & SentryTracingResponse, + next: NextFunction, + ): any { + const transaction = res.__sentry_transaction; + const span = + transaction && + transaction.startChild({ description: fn.name, op: 'middleware', }); - fn.call(this, req, res, function(this: NodeJS.Global): any { + fn.call(this, req, res, function(this: NodeJS.Global): any { + if (span) { span.finish(); - return next.apply(this, arguments); - }); - } else { - fn.call(this, req, res, function(this: NodeJS.Global): any { - return next.apply(this, arguments); - }); - } - // tslint:enable: no-unsafe-any + } + return next.apply(this, arguments); + }); }; } case 4: { - return function(this: NodeJS.Global, err: any, req: Request, res: Response, next: NextFunction): any { - // tslint:disable: no-unsafe-any - const transaction = (res as any).getTransaction && (res as any).getTransaction(); - if (transaction) { - const span = transaction.startChild({ + return function( + this: NodeJS.Global, + err: any, + req: Request, + res: Response & SentryTracingResponse, + next: NextFunction, + ): any { + const transaction = res.__sentry_transaction; + const span = + transaction && + transaction.startChild({ description: fn.name, op: 'middleware', }); - fn.call(this, err, req, res, function(this: NodeJS.Global): any { + fn.call(this, err, req, res, function(this: NodeJS.Global): any { + if (span) { span.finish(); - return next.apply(this, arguments); - }); - } else { - fn.call(this, err, req, res, function(this: NodeJS.Global): any { - return next.apply(this, arguments); - }); - } + } + return next.apply(this, arguments); + }); }; } default: { diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts index 498aa9ae3852..61a34f969aa8 100644 --- a/packages/apm/src/integrations/tracing.ts +++ b/packages/apm/src/integrations/tracing.ts @@ -1,5 +1,5 @@ import { Hub } from '@sentry/hub'; -import { Event, EventProcessor, Integration, Severity, Span, SpanContext } from '@sentry/types'; +import { Event, EventProcessor, Integration, Severity, Span, SpanContext, TransactionContext } from '@sentry/types'; import { addInstrumentationHandler, getGlobalObject, @@ -71,7 +71,7 @@ interface TracingOptions { maxTransactionDuration: number; /** - * Flag Transactions where tabs moved to background with "cancelled".Browser background tab timing is + * Flag Transactions where tabs moved to background with "cancelled". Browser background tab timing is * not suited towards doing precise measurements of operations. Background transaction can mess up your * statistics in non deterministic ways that's why we by default recommend leaving this opition enabled. * @@ -205,7 +205,8 @@ export class Tracing implements Integration { // Starting pageload transaction if (global.location && global.location.href) { // Use `${global.location.href}` as transaction name - Tracing.startIdleTransaction(global.location.href, { + Tracing.startIdleTransaction({ + name: global.location.href, op: 'pageload', }); } @@ -412,13 +413,13 @@ export class Tracing implements Integration { /** * Starts a Transaction waiting for activity idle to finish */ - public static startIdleTransaction(name: string, spanContext?: SpanContext): Span | undefined { + public static startIdleTransaction(transactionContext: TransactionContext): Transaction | 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 Tracing.finishIdleTransaction(); - Tracing._log('[Tracing] startIdleTransaction, name:', name); + Tracing._log('[Tracing] startIdleTransaction, name:', transactionContext.name); const _getCurrentHub = Tracing._getCurrentHub; if (!_getCurrentHub) { @@ -431,8 +432,8 @@ export class Tracing implements Integration { } Tracing._activeTransaction = hub.startSpan({ - ...spanContext, - name, + trimEnd: true, + ...transactionContext, }) as Transaction; // The reason we do this here is because of cached responses @@ -453,7 +454,7 @@ export class Tracing implements Integration { if (active) { Tracing._addPerformanceEntries(active); Tracing._log('[Tracing] finishIdleTransaction', active.name); - active.finish(undefined, /*trimEnd*/ true); + active.finish(); Tracing._resetActiveTransaction(); } } @@ -860,7 +861,8 @@ function fetchCallback(handlerData: { [key: string]: any }): void { */ function historyCallback(_: { [key: string]: any }): void { if (Tracing.options.startTransactionOnLocationChange && global && global.location) { - Tracing.startIdleTransaction(global.location.href, { + Tracing.startIdleTransaction({ + name: global.location.href, op: 'navigation', }); } diff --git a/packages/apm/src/transaction.ts b/packages/apm/src/transaction.ts index a994aadc4959..68d1d0d7dc48 100644 --- a/packages/apm/src/transaction.ts +++ b/packages/apm/src/transaction.ts @@ -26,7 +26,7 @@ export class SpanRecorder { * start_timestamp). */ public add(span: SpanClass): void { - if (this.spans.length > this._maxlen) { + if (this.spans.length >= this._maxlen) { span.spanRecorder = undefined; } else { this.spans.push(span); @@ -43,6 +43,8 @@ export class Transaction extends SpanClass { public name?: string; + private readonly _trimEnd?: boolean; + /** * This constructor should never be called manually. Those instrumenting tracing should use `Stentry.startTransaction()`, and internal methods should use `hub.startSpan()`. * @internal @@ -59,6 +61,8 @@ export class Transaction extends SpanClass { if (transactionContext.name) { this.name = transactionContext.name; } + + this._trimEnd = transactionContext.trimEnd; } /** @@ -80,16 +84,9 @@ export class Transaction extends SpanClass { } /** - * Sets the finish timestamp on the current span. - * - * @inheritdoc - * - * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming - * the duration of the transaction. This is useful to discard extra time in the transaction that is not - * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the - * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. + * @inheritDoc */ - public finish(endTimestamp?: number, trimEnd: boolean = false): string | undefined { + public finish(endTimestamp?: number): string | undefined { // This transaction is already finished, so we should not flush it again. if (this.endTimestamp !== undefined) { return undefined; @@ -105,7 +102,7 @@ export class Transaction extends SpanClass { const finishedSpans = this.spanRecorder ? this.spanRecorder.spans.filter(s => s !== this && s.endTimestamp) : []; - if (trimEnd && finishedSpans.length > 0) { + if (this._trimEnd && finishedSpans.length > 0) { this.endTimestamp = finishedSpans.reduce((prev: SpanClass, current: SpanClass) => { if (prev.endTimestamp && current.endTimestamp) { return prev.endTimestamp > current.endTimestamp ? prev : current; diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 266607d28686..32f82e8c25a6 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -368,8 +368,8 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span { - return this._callExtensionMethod('startSpan', spanOrTransactionContext); + public startSpan(context: SpanContext | TransactionContext): Transaction | Span { + return this._callExtensionMethod('startSpan', context); } /** diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 37e5721d6c00..dc0f477d6cdc 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -53,21 +53,17 @@ export function tracingHandler(): ( traceId, }); - if (transaction) { - // We put the transaction on the scope so users can attach children to it - getCurrentHub().configureScope(scope => { - scope.setSpan(transaction); - }); - // We also set a function getTransaction on the response so people can grab the transaction there to add - // spans to it - (res as any).getTransaction = () => transaction; - } + // We put the transaction on the scope so users can attach children to it + getCurrentHub().configureScope(scope => { + scope.setSpan(transaction); + }); + // We also set __sentry_transaction on the response so people can grab the transaction there to add + // spans to it later. + (res as any).__sentry_transaction = transaction; res.once('finish', () => { - if (transaction) { - transaction.setHttpStatus(res.statusCode); - transaction.finish(); - } + transaction.setHttpStatus(res.statusCode); + transaction.finish(); }); next(); @@ -303,7 +299,6 @@ export function parseRequest( } if (options.transaction && !event.transaction) { - // TODO: Use the real transaction here const transaction = extractTransaction(req, options.transaction); if (transaction) { event.transaction = transaction; diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 72ffa313074e..1db098d89c46 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -177,7 +177,7 @@ export interface Hub { * This functions starts either a Span or a Transaction (depending on the argument passed). * If there is a Span on the Scope we use the `trace_id` for all other created Transactions / Spans as a reference. * - * @param spanOrTransactionContext Properties with which the Transaction/Span should be created + * @param context Properties with which the Transaction/Span should be created */ - startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span; + startSpan(context: SpanContext | TransactionContext): Transaction | Span; } diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index b2ab36f194f4..c3685406d6e5 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -59,40 +59,37 @@ export interface SpanContext { /** Span holding trace_id, span_id */ export interface Span extends SpanContext { + /** + * @inheritDoc + */ + spanId: string; + + /** + * @inheritDoc + */ + traceId: string; + + /** + * @inheritDoc + */ + startTimestamp: number; + + /** + * @inheritDoc + */ + tags: { [key: string]: string }; + + /** + * @inheritDoc + */ + data: { [key: string]: any }; + /** * Sets the finish timestamp on the current span. * @param endTimestamp Takes an endTimestamp if the end should not be the time when you call this function. */ finish(endTimestamp?: number): void; - /** Return a traceparent compatible header string */ - toTraceparent(): string; - - /** Convert the object to JSON for w. spans array info only */ - 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(): { - 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; - }; - /** * Sets the tag attribute on the current span * @param key Tag key @@ -140,4 +137,32 @@ export interface Span extends SpanContext { * Determines whether span was successful (HTTP200) */ isSuccess(): boolean; + + /** Return a traceparent compatible header string */ + toTraceparent(): string; + + /** Convert the object to JSON for w. spans array info only */ + 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(): { + 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; + }; } diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 637439a3b2a8..9dbdfb3e523d 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -5,6 +5,13 @@ import { Span, SpanContext } from './span'; */ export interface TransactionContext extends SpanContext { name: string; + /** + * If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming + * the duration of the transaction. This is useful to discard extra time in the transaction that is not + * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the + * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. + */ + trimEnd?: boolean; } /** @@ -12,19 +19,32 @@ export interface TransactionContext extends SpanContext { */ export interface Transaction extends TransactionContext, Span { /** - * Set the name of the transaction + * @inheritDoc */ - setName(name: string): void; + spanId: string; /** - * Sets the finish timestamp on the current span. - * - * @inheritdoc - * - * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming - * the duration of the transaction. This is useful to discard extra time in the transaction that is not - * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the - * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. + * @inheritDoc + */ + traceId: string; + + /** + * @inheritDoc + */ + startTimestamp: number; + + /** + * @inheritDoc + */ + tags: { [key: string]: string }; + + /** + * @inheritDoc + */ + data: { [key: string]: any }; + + /** + * Set the name of the transaction */ - finish(endTimestamp?: number, trimEnd?: boolean): string | undefined; + setName(name: string): void; } From de78c969ed82a31440875dc6b045412b26184655 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 09:41:25 +0200 Subject: [PATCH 20/28] feat: Safeguard for name prop in transaction --- packages/apm/src/hubextensions.ts | 11 ++++++++++- packages/minimal/src/index.ts | 5 ++++- packages/types/src/transaction.ts | 7 +++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 9664e97b96eb..0ed9f74e9835 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -1,5 +1,6 @@ import { getMainCarrier, Hub } from '@sentry/hub'; import { SpanContext, TransactionContext } from '@sentry/types'; +import { logger } from '@sentry/utils'; import { Span } from './span'; import { Transaction } from './transaction'; @@ -48,8 +49,16 @@ function startSpan(context: SpanContext | TransactionContext): Transaction | Spa } } - // We are dealing with a Transaction + // This is our safeguard so people always get a Transaction in return. + // We set `_isTransaction: true` in {@link Sentry.startTransaction} to have a runtime check + // if the user really wanted to create a Transaction. + if ((context as TransactionContext)._isTransaction && !(context as TransactionContext).name) { + logger.warn('You are trying to start a Transaction but forgot to provide a `name` property.'); + logger.warn('Will fall back to , use `transaction.setName()` to change it.'); + } + if ((newSpanContext as TransactionContext).name) { + // We are dealing with a Transaction const transaction = new Transaction(newSpanContext as TransactionContext, hub); // We only roll the dice on sampling for root spans of transactions because all child spans inherit this state diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 8f06826e410e..6372d46d16d3 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -175,5 +175,8 @@ export function _callOnClient(method: string, ...args: any[]): void { * a transaction it will be sent to Sentry. */ export function startTransaction(transactionContext: TransactionContext): Transaction { - return callOnHub('startSpan', transactionContext); + return callOnHub('startSpan', { + ...transactionContext, + _isTransaction: true, + }); } diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 9dbdfb3e523d..3f0b91da3bf8 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -12,6 +12,13 @@ export interface TransactionContext extends SpanContext { * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. */ trimEnd?: boolean; + + /** + * This flag is internally used by {@link Sentry.startTransaction} and set there to determine this is really a + * Transaction. Since there is no type safety in JS we set it in the top level function to true to check later + * if the user really wanted to create a Transaction and we can log a warning if they forgot to set the name property. + */ + _isTransaction?: boolean; } /** From 6269bfd3b29384f82c6072576f415ef89c21e944 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 10:09:20 +0200 Subject: [PATCH 21/28] fix: SampleRate and BeforeSend for Transaction --- CHANGELOG.md | 1 + packages/core/src/baseclient.ts | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e9d28681cdc..a4be6aad18ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - [apm] fix: Improve bundle size by moving span status to @sentry/apm #2589 - [apm] feat: No longer discard transactions instead mark them deadline exceeded #2588 - [apm] feat: Introduce `Sentry.startTransaction` and `Transaction.startChild` #2600 +- [apm] feat: Transactions no longer go through `beforeSend` #2600 ## 5.15.5 diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 114b75e362ca..b4143317f8c9 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -392,9 +392,11 @@ export abstract class BaseClient implement return SyncPromise.reject('SDK not enabled, will not send event.'); } + const isTransaction = event.type === 'transaction'; // 1.0 === 100% events are sent // 0.0 === 0% events are sent - if (typeof sampleRate === 'number' && Math.random() > sampleRate) { + // Sampling for transaction happens somewhere else + if (!isTransaction && typeof sampleRate === 'number' && Math.random() > sampleRate) { return SyncPromise.reject('This event has been sampled, will not send event.'); } @@ -409,7 +411,8 @@ export abstract class BaseClient implement let finalEvent: Event | null = prepared; const isInternalException = hint && hint.data && (hint.data as { [key: string]: any }).__sentry__ === true; - if (isInternalException || !beforeSend) { + // We skip beforeSend in case of transactions + if (isInternalException || !beforeSend || isTransaction) { this._getBackend().sendEvent(finalEvent); resolve(finalEvent); return; From 19840c31b21da6e8e056875e6a1aae009695d806 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 12:57:46 +0200 Subject: [PATCH 22/28] ref: StartSpan creates a child of the span on scope --- packages/apm/src/hubextensions.ts | 35 +++++++++++------------- packages/apm/src/integrations/express.ts | 8 ++++-- packages/node/src/handlers.ts | 6 ++-- 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 0ed9f74e9835..902da7b2dad2 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -34,21 +34,6 @@ function startSpan(context: SpanContext | TransactionContext): Transaction | Spa const scope = hub.getScope(); const client = hub.getClient(); - let newSpanContext = context; - - if (scope) { - // If there is a Span on the Scope we use the span_id / trace_id - // To define the parent <-> child relationship - const parentSpan = scope.getSpan(); - if (parentSpan) { - const { trace_id } = parentSpan.getTraceContext(); - newSpanContext = { - traceId: trace_id, - ...context, - }; - } - } - // This is our safeguard so people always get a Transaction in return. // We set `_isTransaction: true` in {@link Sentry.startTransaction} to have a runtime check // if the user really wanted to create a Transaction. @@ -57,14 +42,17 @@ function startSpan(context: SpanContext | TransactionContext): Transaction | Spa logger.warn('Will fall back to , use `transaction.setName()` to change it.'); } - if ((newSpanContext as TransactionContext).name) { + if ((context as TransactionContext).name) { // We are dealing with a Transaction - const transaction = new Transaction(newSpanContext as TransactionContext, hub); + const transaction = new Transaction(context as TransactionContext, hub); // We only roll the dice on sampling for root spans of transactions because all child spans inherit this state if (transaction.sampled === undefined) { const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; - transaction.sampled = Math.random() <= sampleRate; + // if true = we want to have the transaction + // if false = we don't want to have it + // Math.random (inclusive of 0, but not 1) + transaction.sampled = Math.random() < sampleRate; } // We only want to create a span list if we sampled the transaction @@ -77,7 +65,16 @@ function startSpan(context: SpanContext | TransactionContext): Transaction | Spa return transaction; } - return new Span(newSpanContext); + if (scope) { + // If there is a Span on the Scope we start a child and return that instead + const parentSpan = scope.getSpan(); + if (parentSpan) { + return parentSpan.startChild(context); + } + } + + // Otherwise we return a new Span + return new Span(context); } /** diff --git a/packages/apm/src/integrations/express.ts b/packages/apm/src/integrations/express.ts index ba189faaf21f..113d41423dcd 100644 --- a/packages/apm/src/integrations/express.ts +++ b/packages/apm/src/integrations/express.ts @@ -3,6 +3,10 @@ import { logger } from '@sentry/utils'; // tslint:disable-next-line:no-implicit-dependencies import { Application, ErrorRequestHandler, NextFunction, Request, RequestHandler, Response } from 'express'; +/** + * Internal helper for `__sentry_transaction` + * @hidden + */ interface SentryTracingResponse { __sentry_transaction?: Transaction; } @@ -73,9 +77,7 @@ function wrap(fn: Function): RequestHandler | ErrorRequestHandler { op: 'middleware', }); res.once('finish', () => { - if (span) { - span.finish(); - } + span.finish(); }); } return fn.apply(this, arguments); diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index dc0f477d6cdc..baab3f0a1753 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -40,9 +40,8 @@ export function tracingHandler(): ( if (req.headers && isString(req.headers['sentry-trace'])) { const span = Span.fromTraceparent(req.headers['sentry-trace'] as string); if (span) { - const spanData = span.toJSON(); - traceId = spanData.trace_id; - parentSpanId = spanData.span_id; + traceId = span.traceId; + parentSpanId = span.parentSpanId; } } @@ -57,6 +56,7 @@ export function tracingHandler(): ( getCurrentHub().configureScope(scope => { scope.setSpan(transaction); }); + // We also set __sentry_transaction on the response so people can grab the transaction there to add // spans to it later. (res as any).__sentry_transaction = transaction; From 171e2a1a724efdf089593ffbaf66ac1dd2893822 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 13:31:53 +0200 Subject: [PATCH 23/28] ref: Set unlabled transaction name --- packages/apm/src/hubextensions.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 902da7b2dad2..1827bca84617 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -40,6 +40,7 @@ function startSpan(context: SpanContext | TransactionContext): Transaction | Spa if ((context as TransactionContext)._isTransaction && !(context as TransactionContext).name) { logger.warn('You are trying to start a Transaction but forgot to provide a `name` property.'); logger.warn('Will fall back to , use `transaction.setName()` to change it.'); + (context as TransactionContext).name = ''; } if ((context as TransactionContext).name) { From 39312e8cd9e7c2f371218b70f5c9cedaa08509cc Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 13:49:31 +0200 Subject: [PATCH 24/28] ref: Slight refactor of startSpan --- packages/apm/src/hubextensions.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 1827bca84617..6e4182cd25c3 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -28,12 +28,7 @@ function traceHeaders(): { [key: string]: string } { * * @param context Properties with which the span should be created */ -function startSpan(context: SpanContext | TransactionContext): Transaction | Span { - // @ts-ignore - const hub = this as Hub; - const scope = hub.getScope(); - const client = hub.getClient(); - +function startSpan(this: Hub, context: SpanContext | TransactionContext): Transaction | Span { // This is our safeguard so people always get a Transaction in return. // We set `_isTransaction: true` in {@link Sentry.startTransaction} to have a runtime check // if the user really wanted to create a Transaction. @@ -45,8 +40,9 @@ function startSpan(context: SpanContext | TransactionContext): Transaction | Spa if ((context as TransactionContext).name) { // We are dealing with a Transaction - const transaction = new Transaction(context as TransactionContext, hub); + const transaction = new Transaction(context as TransactionContext, this); + const client = this.getClient(); // We only roll the dice on sampling for root spans of transactions because all child spans inherit this state if (transaction.sampled === undefined) { const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; @@ -66,6 +62,7 @@ function startSpan(context: SpanContext | TransactionContext): Transaction | Spa return transaction; } + const scope = this.getScope(); if (scope) { // If there is a Span on the Scope we start a child and return that instead const parentSpan = scope.getSpan(); From c288ad44f3c1c3a31de276bf271c70197940b67f Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 13:52:38 +0200 Subject: [PATCH 25/28] ref: Also fix traceHeaders --- packages/apm/src/hubextensions.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 6e4182cd25c3..9d1f3fb4847d 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -6,10 +6,8 @@ import { Span } from './span'; import { Transaction } from './transaction'; /** Returns all trace headers that are currently on the top scope. */ -function traceHeaders(): { [key: string]: string } { - // @ts-ignore - const that = this as Hub; - const scope = that.getScope(); +function traceHeaders(this: Hub): { [key: string]: string } { + const scope = this.getScope(); if (scope) { const span = scope.getSpan(); if (span) { From b077373234abe0b37096bc1b0e421604ae7b6f32 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 14:05:06 +0200 Subject: [PATCH 26/28] feat: Add additional tests --- packages/apm/test/hub.test.ts | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index a146ce2a5b51..4172684e7f7a 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -1,5 +1,6 @@ import { BrowserClient } from '@sentry/browser'; import { Hub, Scope } from '@sentry/hub'; +import { Span, Transaction } from '@sentry/types'; import { addExtensionMethods } from '../src/hubextensions'; @@ -36,14 +37,22 @@ describe('Hub', () => { }); }); - describe('start', () => { - test('simple', () => { + describe('startSpan', () => { + test('simple standalone Span', () => { const hub = new Hub(new BrowserClient()); const span = hub.startSpan({}) as any; expect(span.spanId).toBeTruthy(); }); - test('transaction inherits trace_id from span on scope', () => { + test('simple standalone Transaction', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const transaction = hub.startSpan({ name: 'transaction' }) as Transaction; + expect(transaction.spanId).toBeTruthy(); + // tslint:disable-next-line: no-unbound-method + expect(transaction.setName).toBeTruthy(); + }); + + test('Transaction inherits trace_id from span on scope', () => { const myScope = new Scope(); const hub = new Hub(new BrowserClient(), myScope); const parentSpan = hub.startSpan({}) as any; @@ -53,6 +62,24 @@ describe('Hub', () => { const span = hub.startSpan({ name: 'test' }) as any; expect(span.trace_id).toEqual(parentSpan.trace_id); }); + + test('create a child if there is a Span already on the scope', () => { + const myScope = new Scope(); + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }), myScope); + const transaction = hub.startSpan({ name: 'transaction' }) as Transaction; + hub.configureScope(scope => { + scope.setSpan(transaction); + }); + const span = hub.startSpan({}) as Span; + expect(span.traceId).toEqual(transaction.traceId); + expect(span.parentSpanId).toEqual(transaction.spanId); + hub.configureScope(scope => { + scope.setSpan(span); + }); + const span2 = hub.startSpan({}) as Span; + expect(span2.traceId).toEqual(span.traceId); + expect(span2.parentSpanId).toEqual(span.spanId); + }); }); }); }); From 43940d750adddc7641a89fab993af3d64f0deb89 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 14:10:03 +0200 Subject: [PATCH 27/28] fix: Small typo in tests --- packages/apm/test/hub.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index 4172684e7f7a..b067ea77d46e 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -14,7 +14,7 @@ describe('Hub', () => { describe('spans', () => { describe('sampling', () => { - test('set tracesSampleRate 0 root span', () => { + test('set tracesSampleRate 0 on span', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); const span = hub.startSpan({}) as any; expect(span.sampled).toBeUndefined(); From 5446d82bcdc5bae92cc551bd5d02904e11acf720 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 14:26:47 +0200 Subject: [PATCH 28/28] fix: One off tests --- packages/apm/src/transaction.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apm/src/transaction.ts b/packages/apm/src/transaction.ts index 68d1d0d7dc48..fb3cdc63589f 100644 --- a/packages/apm/src/transaction.ts +++ b/packages/apm/src/transaction.ts @@ -26,7 +26,7 @@ export class SpanRecorder { * start_timestamp). */ public add(span: SpanClass): void { - if (this.spans.length >= this._maxlen) { + if (this.spans.length > this._maxlen) { span.spanRecorder = undefined; } else { this.spans.push(span);