From 5728da3f884bc9625c0c985f6d9201cfe4da5ebc Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 11 Jun 2020 11:46:20 +0200 Subject: [PATCH 01/13] feat: Add Sentry.getTransaction --- packages/apm/src/hubextensions.ts | 16 ++++++++++++++++ packages/apm/test/hub.test.ts | 22 ++++++++++++++++++++++ packages/hub/src/hub.ts | 7 +++++++ packages/minimal/src/index.ts | 8 ++++++++ packages/types/src/hub.ts | 6 ++++++ 5 files changed, 59 insertions(+) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 445225ab6622..7ee123289b2a 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -19,6 +19,19 @@ function traceHeaders(this: Hub): { [key: string]: string } { return {}; } +/** + * {@see Hub.getTransaction} + */ +function getTransaction(this: Hub, callback: (transaction: Transaction) => void): void { + const scope = this.getScope(); + if (scope) { + const span = scope.getSpan() as Transaction; + if (span) { + callback(span); + } + } +} + /** * {@see Hub.startTransaction} */ @@ -96,6 +109,9 @@ export function addExtensionMethods(): void { if (!carrier.__SENTRY__.extensions.startSpan) { carrier.__SENTRY__.extensions.startSpan = startSpan; } + if (!carrier.__SENTRY__.extensions.getTransaction) { + carrier.__SENTRY__.extensions.getTransaction = getTransaction; + } if (!carrier.__SENTRY__.extensions.traceHeaders) { carrier.__SENTRY__.extensions.traceHeaders = traceHeaders; } diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index 6d9d464f5ace..c88dabe3880e 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -11,6 +11,28 @@ describe('Hub', () => { jest.useRealTimers(); }); + describe('getTransaction', () => { + test('simple invoke', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const transaction = hub.startTransaction({ name: 'foo' }); + hub.configureScope(scope => { + scope.setSpan(transaction); + }); + hub.getTransaction(t => { + expect(t).toBe(transaction); + }); + }); + + test('not invoke', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const transaction = hub.startTransaction({ name: 'foo' }); + hub.getTransaction(_ => { + expect(true).toBe(false); + }); + transaction.finish(); + }); + }); + describe('spans', () => { describe('sampling', () => { test('set tracesSampleRate 0 on span', () => { diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index a6bf56bcb45c..92e0470a8415 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -380,6 +380,13 @@ export class Hub implements HubInterface { return this._callExtensionMethod('startTransaction', context); } + /** + * @inheritDoc + */ + public getTransaction(callback: (transaction: Transaction) => void): void { + this._callExtensionMethod('getTransaction', callback); + } + /** * @inheritDoc */ diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 83cd70c27eff..5bbd82b5a065 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -196,3 +196,11 @@ export function _callOnClient(method: string, ...args: any[]): void { export function startTransaction(context: TransactionContext): Transaction { return callOnHub('startTransaction', { ...context }); } + +/** + * Callback to retrieve an ongoing Transaction in case there is one. + * @param callback Will only be invoked in case there is an active transaction + */ +export function getTransaction(callback: (transaction: Transaction) => void): void { + callOnHub('getTransaction', callback); +} diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 694e531c4d87..289c78aef2ec 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -199,4 +199,10 @@ export interface Hub { * @param context Properties of the new `Transaction`. */ startTransaction(context: TransactionContext): Transaction; + + /** + * Callback to retrieve an ongoing Transaction in case there is one. + * @param callback Will only be invoked in case there is an active transaction + */ + getTransaction(callback: (transaction: Transaction) => void): void; } From 4cd4146309ae6fafeed50fbbf838a4336883a66e Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 11 Jun 2020 14:17:10 +0200 Subject: [PATCH 02/13] fix: Linter --- packages/hub/src/hub.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 92e0470a8415..a2a96448c7a1 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -384,7 +384,7 @@ export class Hub implements HubInterface { * @inheritDoc */ public getTransaction(callback: (transaction: Transaction) => void): void { - this._callExtensionMethod('getTransaction', callback); + this._callExtensionMethod('getTransaction', callback); } /** From a116d0f9f58d8755c9112b8b15dddc0120a6fcf8 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 15 Jun 2020 11:53:05 +0200 Subject: [PATCH 03/13] ref: Introduce Sentry.getSpan --- CHANGELOG.md | 1 + packages/apm/src/hubextensions.ts | 10 +++++----- packages/apm/src/span.ts | 17 +++++++++++++++-- packages/apm/test/hub.test.ts | 6 +++--- packages/apm/test/span.test.ts | 13 +++++++++++++ packages/hub/src/hub.ts | 4 ++-- packages/minimal/src/index.ts | 19 ++++++++++++++----- packages/types/src/hub.ts | 6 +++--- packages/types/src/span.ts | 7 +++++++ 9 files changed, 63 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8367d07c8a62..49412ea368c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - [core] feat: Export `makeMain` (#2665) - [core] fix: Call `bindClient` when creating new `Hub` to make integrations work automatically (#2665) - [gatsby] feat: Add @sentry/gatsby package (#2652) +- [apm] feat: Add `Sentry.getSpan` to return the Span on the Scope (#2668) ## 5.17.0 diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index 7ee123289b2a..b1cff0350212 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -20,12 +20,12 @@ function traceHeaders(this: Hub): { [key: string]: string } { } /** - * {@see Hub.getTransaction} + * {@see Hub.getSpan} */ -function getTransaction(this: Hub, callback: (transaction: Transaction) => void): void { +function getSpan(this: Hub, callback: (span: Span) => void): void { const scope = this.getScope(); if (scope) { - const span = scope.getSpan() as Transaction; + const span = scope.getSpan(); if (span) { callback(span); } @@ -109,8 +109,8 @@ export function addExtensionMethods(): void { if (!carrier.__SENTRY__.extensions.startSpan) { carrier.__SENTRY__.extensions.startSpan = startSpan; } - if (!carrier.__SENTRY__.extensions.getTransaction) { - carrier.__SENTRY__.extensions.getTransaction = getTransaction; + if (!carrier.__SENTRY__.extensions.getSpan) { + carrier.__SENTRY__.extensions.getSpan = getSpan; } if (!carrier.__SENTRY__.extensions.traceHeaders) { carrier.__SENTRY__.extensions.traceHeaders = traceHeaders; diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index eb6c49d42bab..5214ef86126c 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -1,5 +1,5 @@ -import { Span as SpanInterface, SpanContext } from '@sentry/types'; -import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils'; +import { Span as SpanInterface, SpanContext, Transaction } from '@sentry/types'; +import { dropUndefinedKeys, logger, timestampWithMs, uuid4 } from '@sentry/utils'; import { SpanStatus } from './spanstatus'; import { SpanRecorder } from './transaction'; @@ -153,6 +153,19 @@ export class Span implements SpanInterface, SpanContext { return span; } + /** + * @inheritDoc + */ + public getTransaction(): Transaction { + const recorder = this.spanRecorder; + if (!recorder) { + logger.warn('This Span has no reference to a Transaction. Returning an instance to itself.'); + logger.warn('It means that the Transaction has been sampled or the Span did not originate from a Transaction.'); + return (this as unknown) as Transaction; + } + return recorder.spans[0] as Transaction; + } + /** * Continues a trace from a string (usually the header). * @param traceparent Traceparent string diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index c88dabe3880e..cb1b80a53391 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -11,14 +11,14 @@ describe('Hub', () => { jest.useRealTimers(); }); - describe('getTransaction', () => { + describe('getSpan', () => { test('simple invoke', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const transaction = hub.startTransaction({ name: 'foo' }); hub.configureScope(scope => { scope.setSpan(transaction); }); - hub.getTransaction(t => { + hub.getSpan(t => { expect(t).toBe(transaction); }); }); @@ -26,7 +26,7 @@ describe('Hub', () => { test('not invoke', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const transaction = hub.startTransaction({ name: 'foo' }); - hub.getTransaction(_ => { + hub.getSpan(_ => { expect(true).toBe(false); }); transaction.finish(); diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index 0fb373174259..117aa60004ef 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -366,6 +366,19 @@ describe('Span', () => { }); }); + test('getTransaction on Span from Transaction', () => { + const transaction = hub.startTransaction({ name: 'test' }); + const childSpan = transaction.startChild(); + childSpan.finish(); + transaction.finish(); + expect(childSpan.getTransaction()).toBe(transaction); + }); + + test('getTransaction on new Span()', () => { + const span = new Span({}); + expect(span.getTransaction()).toBe(span); + }); + describe('getTraceContext', () => { test('should have status attribute undefined if no status tag is available', () => { const span = new Span({}); diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index a2a96448c7a1..7c31fa13516d 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -383,8 +383,8 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public getTransaction(callback: (transaction: Transaction) => void): void { - this._callExtensionMethod('getTransaction', callback); + public getSpan(callback: (span: Span) => void): void { + this._callExtensionMethod('getSpan', callback); } /** diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 5bbd82b5a065..b629b5aeefee 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -1,5 +1,14 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub'; -import { Breadcrumb, CaptureContext, Event, Severity, Transaction, TransactionContext, User } from '@sentry/types'; +import { + Breadcrumb, + CaptureContext, + Event, + Severity, + Span, + Transaction, + TransactionContext, + User, +} from '@sentry/types'; /** * This calls a function on the current hub. @@ -198,9 +207,9 @@ export function startTransaction(context: TransactionContext): Transaction { } /** - * Callback to retrieve an ongoing Transaction in case there is one. - * @param callback Will only be invoked in case there is an active transaction + * Callback that receives a Span if there is one on the Scope. + * @param callback Will only be invoked in case there is a Span on the Scope */ -export function getTransaction(callback: (transaction: Transaction) => void): void { - callOnHub('getTransaction', callback); +export function getSpan(callback: (span: Span) => void): void { + callOnHub('getSpan', callback); } diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 289c78aef2ec..6fe017770e7f 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -201,8 +201,8 @@ export interface Hub { startTransaction(context: TransactionContext): Transaction; /** - * Callback to retrieve an ongoing Transaction in case there is one. - * @param callback Will only be invoked in case there is an active transaction + * Callback that receives a Span if there is one on the Scope. + * @param callback Will only be invoked in case there is a Span on the Scope */ - getTransaction(callback: (transaction: Transaction) => void): void; + getSpan(callback: (span: Span) => void): void; } diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index c3685406d6e5..c4e3840ee205 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -1,3 +1,5 @@ +import { Transaction } from './transaction'; + /** Interface holding all properties that can be set on a Span on creation. */ export interface SpanContext { /** @@ -133,6 +135,11 @@ export interface Span extends SpanContext { spanContext?: Pick>, ): Span; + /** + * Retruns the reference to the root Span (Transaction). + */ + getTransaction(): Transaction; + /** * Determines whether span was successful (HTTP200) */ From 9a9a3600d985dbbadfd952395b7c63129ace922d Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 15 Jun 2020 20:41:14 +0200 Subject: [PATCH 04/13] ref: Remove getSpan and use scope.getTransaction --- packages/apm/src/hubextensions.ts | 16 ---------------- packages/apm/src/span.ts | 8 ++++---- packages/apm/test/hub.test.ts | 12 ++++++++---- packages/apm/test/span.test.ts | 13 ------------- packages/hub/src/hub.ts | 7 ------- packages/hub/src/scope.ts | 13 +++++++++++++ packages/minimal/src/index.ts | 19 +------------------ packages/node/src/integrations/http.ts | 15 ++++++--------- packages/types/src/hub.ts | 6 ------ packages/types/src/scope.ts | 12 ++++++++++++ packages/types/src/span.ts | 5 +++-- 11 files changed, 47 insertions(+), 79 deletions(-) diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts index b1cff0350212..445225ab6622 100644 --- a/packages/apm/src/hubextensions.ts +++ b/packages/apm/src/hubextensions.ts @@ -19,19 +19,6 @@ function traceHeaders(this: Hub): { [key: string]: string } { return {}; } -/** - * {@see Hub.getSpan} - */ -function getSpan(this: Hub, callback: (span: Span) => void): void { - const scope = this.getScope(); - if (scope) { - const span = scope.getSpan(); - if (span) { - callback(span); - } - } -} - /** * {@see Hub.startTransaction} */ @@ -109,9 +96,6 @@ export function addExtensionMethods(): void { if (!carrier.__SENTRY__.extensions.startSpan) { carrier.__SENTRY__.extensions.startSpan = startSpan; } - if (!carrier.__SENTRY__.extensions.getSpan) { - carrier.__SENTRY__.extensions.getSpan = getSpan; - } if (!carrier.__SENTRY__.extensions.traceHeaders) { carrier.__SENTRY__.extensions.traceHeaders = traceHeaders; } diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 5214ef86126c..8f02e72be37d 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -156,14 +156,14 @@ export class Span implements SpanInterface, SpanContext { /** * @inheritDoc */ - public getTransaction(): Transaction { + public getTransaction(callback: (transaction: Transaction) => void): void { const recorder = this.spanRecorder; - if (!recorder) { + if (!recorder || !recorder.spans[0]) { logger.warn('This Span has no reference to a Transaction. Returning an instance to itself.'); logger.warn('It means that the Transaction has been sampled or the Span did not originate from a Transaction.'); - return (this as unknown) as Transaction; + return; } - return recorder.spans[0] as Transaction; + callback(recorder.spans[0] as Transaction); } /** diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index cb1b80a53391..1dee3b0d42e5 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -18,16 +18,20 @@ describe('Hub', () => { hub.configureScope(scope => { scope.setSpan(transaction); }); - hub.getSpan(t => { - expect(t).toBe(transaction); + hub.configureScope(s => { + s.getTransaction(t => { + expect(t).toBe(transaction); + }); }); }); test('not invoke', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const transaction = hub.startTransaction({ name: 'foo' }); - hub.getSpan(_ => { - expect(true).toBe(false); + hub.configureScope(s => { + s.getTransaction(_ => { + expect(true).toBe(false); + }); }); transaction.finish(); }); diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index 117aa60004ef..0fb373174259 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -366,19 +366,6 @@ describe('Span', () => { }); }); - test('getTransaction on Span from Transaction', () => { - const transaction = hub.startTransaction({ name: 'test' }); - const childSpan = transaction.startChild(); - childSpan.finish(); - transaction.finish(); - expect(childSpan.getTransaction()).toBe(transaction); - }); - - test('getTransaction on new Span()', () => { - const span = new Span({}); - expect(span.getTransaction()).toBe(span); - }); - describe('getTraceContext', () => { test('should have status attribute undefined if no status tag is available', () => { const span = new Span({}); diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 7c31fa13516d..a6bf56bcb45c 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -380,13 +380,6 @@ export class Hub implements HubInterface { return this._callExtensionMethod('startTransaction', context); } - /** - * @inheritDoc - */ - public getSpan(callback: (span: Span) => void): void { - this._callExtensionMethod('getSpan', callback); - } - /** * @inheritDoc */ diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index 194595bc6133..0fcffe0a1bf1 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -8,6 +8,7 @@ import { ScopeContext, Severity, Span, + Transaction, User, } from '@sentry/types'; import { getGlobalObject, isPlainObject, isThenable, SyncPromise, timestampWithMs } from '@sentry/utils'; @@ -217,6 +218,18 @@ export class Scope implements ScopeInterface { return this._span; } + /** + * @inheritDoc + */ + public getTransaction(callback: (transaction: Transaction) => void): void { + const span = this.getSpan() as Span & { spanRecorder: { spans: Span[] } }; + if (span) { + if (span.spanRecorder && span.spanRecorder.spans[0]) { + callback(span.spanRecorder.spans[0] as Transaction); + } + } + } + /** * Inherit values from the parent scope. * @param scope to clone. diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index b629b5aeefee..83cd70c27eff 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -1,14 +1,5 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub'; -import { - Breadcrumb, - CaptureContext, - Event, - Severity, - Span, - Transaction, - TransactionContext, - User, -} from '@sentry/types'; +import { Breadcrumb, CaptureContext, Event, Severity, Transaction, TransactionContext, User } from '@sentry/types'; /** * This calls a function on the current hub. @@ -205,11 +196,3 @@ export function _callOnClient(method: string, ...args: any[]): void { export function startTransaction(context: TransactionContext): Transaction { return callOnHub('startTransaction', { ...context }); } - -/** - * Callback that receives a Span if there is one on the Scope. - * @param callback Will only be invoked in case there is a Span on the Scope - */ -export function getSpan(callback: (span: Span) => void): void { - callOnHub('getSpan', callback); -} diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 7c6f6b932252..d0b1fa6bcba1 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -79,17 +79,14 @@ function createHandlerWrapper( } 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', + if (scope && tracingEnabled) { + scope.getTransaction(transaction => { + span = transaction.startChild({ + description: `${typeof options === 'string' || !options.method ? 'GET' : options.method} ${requestUrl}`, + op: 'request', + }); }); } diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 6fe017770e7f..694e531c4d87 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -199,10 +199,4 @@ export interface Hub { * @param context Properties of the new `Transaction`. */ startTransaction(context: TransactionContext): Transaction; - - /** - * Callback that receives a Span if there is one on the Scope. - * @param callback Will only be invoked in case there is a Span on the Scope - */ - getSpan(callback: (span: Span) => void): void; } diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts index 5af0c689f33d..d7a8d3e27547 100644 --- a/packages/types/src/scope.ts +++ b/packages/types/src/scope.ts @@ -2,6 +2,7 @@ import { Breadcrumb } from './breadcrumb'; import { EventProcessor } from './eventprocessor'; import { Severity } from './severity'; import { Span } from './span'; +import { Transaction } from './transaction'; import { User } from './user'; /** JSDocs */ @@ -89,6 +90,17 @@ export interface Scope { */ setSpan(span?: Span): this; + /** + * Returns the current set span if there is one + */ + getSpan(): Span | undefined; + + /** + * Callback to retrieve an ongoing Transaction in case there is one. + * @param callback Will only be invoked in case there is an active transaction + */ + getTransaction(callback: (transaction: Transaction) => void): void; + /** * Updates the scope with provided data. Can work in three variations: * - plain object containing updatable attributes diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index c4e3840ee205..7efaff8f8218 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -136,9 +136,10 @@ export interface Span extends SpanContext { ): Span; /** - * Retruns the reference to the root Span (Transaction). + * Callback to retrieve the root Span (Transaction) in case there is one. + * @param callback Will only be invoked in case there is an active transaction */ - getTransaction(): Transaction; + getTransaction(callback: (transaction: Transaction) => void): void; /** * Determines whether span was successful (HTTP200) From 24e249416825e60c25b650310c2ca05c423163cf Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 15 Jun 2020 20:41:41 +0200 Subject: [PATCH 05/13] fix: Linter --- packages/node/src/integrations/http.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index d0b1fa6bcba1..01e4b0011f13 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, Transaction } from '@sentry/types'; +import { Integration, Span } from '@sentry/types'; import { fill, parseSemver } from '@sentry/utils'; import * as http from 'http'; import * as https from 'https'; From 0faac9bc4ea0ae2d026feb924633090fa9df505f Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 16 Jun 2020 10:20:23 +0200 Subject: [PATCH 06/13] ref: Rename setTransaction --- CHANGELOG.md | 3 ++- packages/hub/src/scope.ts | 24 ++++++++++++++++-------- packages/hub/test/scope.test.ts | 18 +++++++++--------- packages/types/src/scope.ts | 5 ++--- 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78a4c1194e27..a6952f50f12f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,8 @@ - [core] feat: Export `makeMain` (#2665) - [core] fix: Call `bindClient` when creating new `Hub` to make integrations work automatically (#2665) - [gatsby] feat: Add @sentry/gatsby package (#2652) -- [apm] feat: Add `Sentry.getSpan` to return the Span on the Scope (#2668) +- [apm] feat: Add `scope.getTransaction` to return a Transaction if it exists (#2668) +- [apm] ref: Depracate `scope.setTransaction` in favor of `scope.setTransactionName` (#2668) - [core] ref: Rename `whitelistUrls/blacklistUrls` to `allowUrls/denyUrls` (#2671) ## 5.17.0 diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index 0fcffe0a1bf1..7b5b42551641 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -48,8 +48,8 @@ export class Scope implements ScopeInterface { /** Severity */ protected _level?: Severity; - /** Transaction */ - protected _transaction?: string; + /** Transaction Name */ + protected _transactionName?: string; /** Span */ protected _span?: Span; @@ -186,12 +186,20 @@ export class Scope implements ScopeInterface { /** * @inheritDoc */ - public setTransaction(transaction?: string): this { - this._transaction = transaction; + public setTransactionName(name?: string): this { + this._transactionName = name; this._notifyScopeListeners(); return this; } + /** + * Can be removed in major version. + * @deprecated in favor of {@link this.setTransactionName} + */ + public setTransaction(name?: string): this { + return this.setTransactionName(name); + } + /** * @inheritDoc */ @@ -244,7 +252,7 @@ export class Scope implements ScopeInterface { newScope._user = scope._user; newScope._level = scope._level; newScope._span = scope._span; - newScope._transaction = scope._transaction; + newScope._transactionName = scope._transactionName; newScope._fingerprint = scope._fingerprint; newScope._eventProcessors = [...scope._eventProcessors]; } @@ -307,7 +315,7 @@ export class Scope implements ScopeInterface { this._user = {}; this._contexts = {}; this._level = undefined; - this._transaction = undefined; + this._transactionName = undefined; this._fingerprint = undefined; this._span = undefined; this._notifyScopeListeners(); @@ -387,8 +395,8 @@ export class Scope implements ScopeInterface { if (this._level) { event.level = this._level; } - if (this._transaction) { - event.transaction = this._transaction; + if (this._transactionName) { + event.transaction = this._transactionName; } // We want to set the trace context for normal events only if there isn't already // a trace context on the event. There is a product feature in place where we link diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index f49da6cce595..ecaffb29e485 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -73,17 +73,17 @@ describe('Scope', () => { expect((scope as any)._level).toEqual(Severity.Critical); }); - test('setTransaction', () => { + test('setTransactionName', () => { const scope = new Scope(); - scope.setTransaction('/abc'); - expect((scope as any)._transaction).toEqual('/abc'); + scope.setTransactionName('/abc'); + expect((scope as any)._transactionName).toEqual('/abc'); }); - test('setTransaction with no value unsets it', () => { + test('setTransactionName with no value unsets it', () => { const scope = new Scope(); - scope.setTransaction('/abc'); - scope.setTransaction(); - expect((scope as any)._transaction).toBeUndefined(); + scope.setTransactionName('/abc'); + scope.setTransactionName(); + expect((scope as any)._transactionName).toBeUndefined(); }); test('setContext', () => { @@ -157,7 +157,7 @@ describe('Scope', () => { scope.setUser({ id: '1' }); scope.setFingerprint(['abcd']); scope.setLevel(Severity.Warning); - scope.setTransaction('/abc'); + scope.setTransactionName('/abc'); scope.addBreadcrumb({ message: 'test' }, 100); scope.setContext('os', { id: '1' }); const event: Event = {}; @@ -256,7 +256,7 @@ describe('Scope', () => { test('scope transaction should have priority over event transaction', () => { expect.assertions(1); const scope = new Scope(); - scope.setTransaction('/abc'); + scope.setTransactionName('/abc'); const event: Event = {}; event.transaction = '/cdf'; return scope.applyToEvent(event).then(processedEvent => { diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts index d7a8d3e27547..064be581a8a0 100644 --- a/packages/types/src/scope.ts +++ b/packages/types/src/scope.ts @@ -72,10 +72,9 @@ export interface Scope { setLevel(level: Severity): this; /** - * Sets the transaction on the scope for future events. - * @param transaction string This will be converted in a tag in Sentry + * Sets the transaction name on the scope for future events. */ - setTransaction(transaction?: string): this; + setTransactionName(name?: string): this; /** * Sets context data with the given name. From c327c030a631dad87921095a81c2266b0234b840 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 16 Jun 2020 15:28:10 +0200 Subject: [PATCH 07/13] Update CHANGELOG.md Co-authored-by: Abhijeet Prasad --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6952f50f12f..6e7ee1991a0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ - [core] fix: Call `bindClient` when creating new `Hub` to make integrations work automatically (#2665) - [gatsby] feat: Add @sentry/gatsby package (#2652) - [apm] feat: Add `scope.getTransaction` to return a Transaction if it exists (#2668) -- [apm] ref: Depracate `scope.setTransaction` in favor of `scope.setTransactionName` (#2668) +- [apm] ref: Deprecate `scope.setTransaction` in favor of `scope.setTransactionName` (#2668) - [core] ref: Rename `whitelistUrls/blacklistUrls` to `allowUrls/denyUrls` (#2671) ## 5.17.0 From 045fd884d0c9b1f51491e47bb7aafbe564514e7b Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 17 Jun 2020 12:27:37 +0200 Subject: [PATCH 08/13] ref: CodeReview --- packages/apm/test/hub.test.ts | 10 +++------- packages/hub/src/scope.ts | 5 +++-- packages/types/src/scope.ts | 7 +++---- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index 1dee3b0d42e5..e24f1fc6638d 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -11,7 +11,7 @@ describe('Hub', () => { jest.useRealTimers(); }); - describe('getSpan', () => { + describe('getTransaction', () => { test('simple invoke', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const transaction = hub.startTransaction({ name: 'foo' }); @@ -19,9 +19,7 @@ describe('Hub', () => { scope.setSpan(transaction); }); hub.configureScope(s => { - s.getTransaction(t => { - expect(t).toBe(transaction); - }); + expect(s.getTransaction()).toBe(transaction); }); }); @@ -29,9 +27,7 @@ describe('Hub', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const transaction = hub.startTransaction({ name: 'foo' }); hub.configureScope(s => { - s.getTransaction(_ => { - expect(true).toBe(false); - }); + expect(s.getTransaction()).toBeUndefined(); }); transaction.finish(); }); diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index 7b5b42551641..ddda42425c7e 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -229,13 +229,14 @@ export class Scope implements ScopeInterface { /** * @inheritDoc */ - public getTransaction(callback: (transaction: Transaction) => void): void { + public getTransaction(): Transaction | undefined { const span = this.getSpan() as Span & { spanRecorder: { spans: Span[] } }; if (span) { if (span.spanRecorder && span.spanRecorder.spans[0]) { - callback(span.spanRecorder.spans[0] as Transaction); + return span.spanRecorder.spans[0] as Transaction; } } + return undefined; } /** diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts index 064be581a8a0..f500c444025c 100644 --- a/packages/types/src/scope.ts +++ b/packages/types/src/scope.ts @@ -90,15 +90,14 @@ export interface Scope { setSpan(span?: Span): this; /** - * Returns the current set span if there is one + * Returns the `Span` if there is one */ getSpan(): Span | undefined; /** - * Callback to retrieve an ongoing Transaction in case there is one. - * @param callback Will only be invoked in case there is an active transaction + * Returns the `Transaction` if there is one */ - getTransaction(callback: (transaction: Transaction) => void): void; + getTransaction(): Transaction | undefined; /** * Updates the scope with provided data. Can work in three variations: From 2d7ccba481f1a44d4c5762abd1fa3e6f7b657b8e Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 17 Jun 2020 12:29:03 +0200 Subject: [PATCH 09/13] fix: Node --- packages/node/src/integrations/http.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 01e4b0011f13..19b051843813 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'; @@ -79,15 +79,17 @@ function createHandlerWrapper( } let span: Span | undefined; + let transaction: Transaction | undefined; const scope = getCurrentHub().getScope(); if (scope && tracingEnabled) { - scope.getTransaction(transaction => { + transaction = scope.getTransaction(); + if (transaction) { span = transaction.startChild({ description: `${typeof options === 'string' || !options.method ? 'GET' : options.method} ${requestUrl}`, op: 'request', }); - }); + } } return originalHandler From 6779197024ecd8156adab93eec7418bcf7667d12 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 18 Jun 2020 15:46:35 +0200 Subject: [PATCH 10/13] ref: CR --- packages/apm/src/span.ts | 13 ------------- packages/hub/src/scope.ts | 3 +-- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index 8f02e72be37d..c7e700cf36f6 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -153,19 +153,6 @@ export class Span implements SpanInterface, SpanContext { return span; } - /** - * @inheritDoc - */ - public getTransaction(callback: (transaction: Transaction) => void): void { - const recorder = this.spanRecorder; - if (!recorder || !recorder.spans[0]) { - logger.warn('This Span has no reference to a Transaction. Returning an instance to itself.'); - logger.warn('It means that the Transaction has been sampled or the Span did not originate from a Transaction.'); - return; - } - callback(recorder.spans[0] as Transaction); - } - /** * Continues a trace from a string (usually the header). * @param traceparent Traceparent string diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index ddda42425c7e..1e86d1cfaafc 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -219,8 +219,7 @@ export class Scope implements ScopeInterface { } /** - * Internal getter for Span, used in Hub. - * @hidden + * @inheritDoc */ public getSpan(): Span | undefined { return this._span; From e0273adfe991f8e50f742deb3adebb428a40ab64 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 18 Jun 2020 16:06:29 +0200 Subject: [PATCH 11/13] ref: Remove unused code --- packages/apm/src/span.ts | 4 ++-- packages/types/src/span.ts | 8 -------- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index c7e700cf36f6..eb6c49d42bab 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -1,5 +1,5 @@ -import { Span as SpanInterface, SpanContext, Transaction } from '@sentry/types'; -import { dropUndefinedKeys, logger, timestampWithMs, uuid4 } from '@sentry/utils'; +import { Span as SpanInterface, SpanContext } from '@sentry/types'; +import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils'; import { SpanStatus } from './spanstatus'; import { SpanRecorder } from './transaction'; diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 7efaff8f8218..c3685406d6e5 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -1,5 +1,3 @@ -import { Transaction } from './transaction'; - /** Interface holding all properties that can be set on a Span on creation. */ export interface SpanContext { /** @@ -135,12 +133,6 @@ export interface Span extends SpanContext { spanContext?: Pick>, ): Span; - /** - * Callback to retrieve the root Span (Transaction) in case there is one. - * @param callback Will only be invoked in case there is an active transaction - */ - getTransaction(callback: (transaction: Transaction) => void): void; - /** * Determines whether span was successful (HTTP200) */ From ebd36809c724188f16c55218df81ebd34cf192b0 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 22 Jun 2020 10:46:25 +0200 Subject: [PATCH 12/13] ref: Code Review --- packages/apm/test/hub.test.ts | 22 --------------------- packages/apm/test/scope.test.ts | 35 +++++++++++++++++++++++++++++++++ packages/hub/src/scope.ts | 6 ++---- 3 files changed, 37 insertions(+), 26 deletions(-) create mode 100644 packages/apm/test/scope.test.ts diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index e24f1fc6638d..6d9d464f5ace 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -11,28 +11,6 @@ describe('Hub', () => { jest.useRealTimers(); }); - describe('getTransaction', () => { - test('simple invoke', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); - const transaction = hub.startTransaction({ name: 'foo' }); - hub.configureScope(scope => { - scope.setSpan(transaction); - }); - hub.configureScope(s => { - expect(s.getTransaction()).toBe(transaction); - }); - }); - - test('not invoke', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); - const transaction = hub.startTransaction({ name: 'foo' }); - hub.configureScope(s => { - expect(s.getTransaction()).toBeUndefined(); - }); - transaction.finish(); - }); - }); - describe('spans', () => { describe('sampling', () => { test('set tracesSampleRate 0 on span', () => { diff --git a/packages/apm/test/scope.test.ts b/packages/apm/test/scope.test.ts new file mode 100644 index 000000000000..ac00cffc9ece --- /dev/null +++ b/packages/apm/test/scope.test.ts @@ -0,0 +1,35 @@ +import { BrowserClient } from '@sentry/browser'; +import { Hub } from '@sentry/hub'; + +import { addExtensionMethods } from '../src/hubextensions'; + +addExtensionMethods(); + +describe('Scope', () => { + afterEach(() => { + jest.resetAllMocks(); + jest.useRealTimers(); + }); + + describe('getTransaction', () => { + test('simple invoke', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const transaction = hub.startTransaction({ name: 'foo' }); + hub.configureScope(scope => { + scope.setSpan(transaction); + }); + hub.configureScope(s => { + expect(s.getTransaction()).toBe(transaction); + }); + }); + + test('not invoke', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const transaction = hub.startTransaction({ name: 'foo' }); + hub.configureScope(s => { + expect(s.getTransaction()).toBeUndefined(); + }); + transaction.finish(); + }); + }); +}); diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index 1e86d1cfaafc..8e3abf4372e9 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -230,10 +230,8 @@ export class Scope implements ScopeInterface { */ public getTransaction(): Transaction | undefined { const span = this.getSpan() as Span & { spanRecorder: { spans: Span[] } }; - if (span) { - if (span.spanRecorder && span.spanRecorder.spans[0]) { - return span.spanRecorder.spans[0] as Transaction; - } + if (span && span.spanRecorder && span.spanRecorder.spans[0]) { + return span.spanRecorder.spans[0] as Transaction; } return undefined; } From fd231479c2512fc1262db74a354e8fd70c53cbf3 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Mon, 22 Jun 2020 10:57:32 +0200 Subject: [PATCH 13/13] ref: Revert tests --- packages/apm/test/hub.test.ts | 22 +++++++++++++++++++++ packages/apm/test/scope.test.ts | 35 --------------------------------- 2 files changed, 22 insertions(+), 35 deletions(-) delete mode 100644 packages/apm/test/scope.test.ts diff --git a/packages/apm/test/hub.test.ts b/packages/apm/test/hub.test.ts index 6d9d464f5ace..e24f1fc6638d 100644 --- a/packages/apm/test/hub.test.ts +++ b/packages/apm/test/hub.test.ts @@ -11,6 +11,28 @@ describe('Hub', () => { jest.useRealTimers(); }); + describe('getTransaction', () => { + test('simple invoke', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const transaction = hub.startTransaction({ name: 'foo' }); + hub.configureScope(scope => { + scope.setSpan(transaction); + }); + hub.configureScope(s => { + expect(s.getTransaction()).toBe(transaction); + }); + }); + + test('not invoke', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const transaction = hub.startTransaction({ name: 'foo' }); + hub.configureScope(s => { + expect(s.getTransaction()).toBeUndefined(); + }); + transaction.finish(); + }); + }); + describe('spans', () => { describe('sampling', () => { test('set tracesSampleRate 0 on span', () => { diff --git a/packages/apm/test/scope.test.ts b/packages/apm/test/scope.test.ts deleted file mode 100644 index ac00cffc9ece..000000000000 --- a/packages/apm/test/scope.test.ts +++ /dev/null @@ -1,35 +0,0 @@ -import { BrowserClient } from '@sentry/browser'; -import { Hub } from '@sentry/hub'; - -import { addExtensionMethods } from '../src/hubextensions'; - -addExtensionMethods(); - -describe('Scope', () => { - afterEach(() => { - jest.resetAllMocks(); - jest.useRealTimers(); - }); - - describe('getTransaction', () => { - test('simple invoke', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); - const transaction = hub.startTransaction({ name: 'foo' }); - hub.configureScope(scope => { - scope.setSpan(transaction); - }); - hub.configureScope(s => { - expect(s.getTransaction()).toBe(transaction); - }); - }); - - test('not invoke', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); - const transaction = hub.startTransaction({ name: 'foo' }); - hub.configureScope(s => { - expect(s.getTransaction()).toBeUndefined(); - }); - transaction.finish(); - }); - }); -});