Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add scope.getTransaction, rename scope.setTransaction -> setTransactionName #2668

Merged
merged 16 commits into from Jun 22, 2020
4 changes: 3 additions & 1 deletion CHANGELOG.md
Expand Up @@ -9,7 +9,9 @@
- [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)
- [core] ref: Rename `whitelistUrls/blacklistUrls` to `allowUrls/denyUrls`
- [apm] feat: Add `scope.getTransaction` to return a Transaction if it exists (#2668)
HazAT marked this conversation as resolved.
Show resolved Hide resolved
- [apm] ref: Deprecate `scope.setTransaction` in favor of `scope.setTransactionName` (#2668)
- [core] ref: Rename `whitelistUrls/blacklistUrls` to `allowUrls/denyUrls` (#2671)

## 5.17.0

Expand Down
17 changes: 15 additions & 2 deletions 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';
Expand Down Expand Up @@ -153,6 +153,19 @@ export class Span implements SpanInterface, SpanContext {
return span;
}

/**
* @inheritDoc
*/
public getTransaction(callback: (transaction: Transaction) => void): void {
HazAT marked this conversation as resolved.
Show resolved Hide resolved
HazAT marked this conversation as resolved.
Show resolved Hide resolved
const recorder = this.spanRecorder;
if (!recorder || !recorder.spans[0]) {
logger.warn('This Span has no reference to a Transaction. Returning an instance to itself.');
HazAT marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
22 changes: 22 additions & 0 deletions packages/apm/test/hub.test.ts
Expand Up @@ -11,6 +11,28 @@ describe('Hub', () => {
jest.useRealTimers();
});

describe('getTransaction', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are not technically "correct". defined/undefined test should be moved to scope.test.ts, as this functionality is owned by the Scope, and tests below should only assert that a given method was called on the scope. expect(s.getTransaction).toBeCalled().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to scope.test.ts

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', () => {
Expand Down
38 changes: 30 additions & 8 deletions packages/hub/src/scope.ts
Expand Up @@ -8,6 +8,7 @@ import {
ScopeContext,
Severity,
Span,
Transaction,
User,
} from '@sentry/types';
import { getGlobalObject, isPlainObject, isThenable, SyncPromise, timestampWithMs } from '@sentry/utils';
Expand Down Expand Up @@ -47,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;
Expand Down Expand Up @@ -185,12 +186,20 @@ export class Scope implements ScopeInterface {
/**
* @inheritDoc
*/
public setTransaction(transaction?: string): this {
this._transaction = transaction;
public setTransactionName(name?: string): this {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offtopic: This whole transaction thing will be very confused (and possibly broken) if someone uses Sentry.Integrations.Transaction in node env.

this._transactionName = name;
this._notifyScopeListeners();
return this;
}

/**
* Can be removed in major version.
HazAT marked this conversation as resolved.
Show resolved Hide resolved
* @deprecated in favor of {@link this.setTransactionName}
*/
public setTransaction(name?: string): this {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kilobytes, kilobytes 😶

return this.setTransactionName(name);
}

/**
* @inheritDoc
*/
Expand All @@ -217,6 +226,19 @@ export class Scope implements ScopeInterface {
return this._span;
}

/**
* @inheritDoc
*/
public getTransaction(): Transaction | undefined {
HazAT marked this conversation as resolved.
Show resolved Hide resolved
const span = this.getSpan() as Span & { spanRecorder: { spans: Span[] } };
if (span) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be inline in one check, as there're no other if branches

if (span.spanRecorder && span.spanRecorder.spans[0]) {
return span.spanRecorder.spans[0] as Transaction;
}
}
return undefined;
}

/**
* Inherit values from the parent scope.
* @param scope to clone.
Expand All @@ -231,7 +253,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];
}
Expand Down Expand Up @@ -294,7 +316,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();
Expand Down Expand Up @@ -374,8 +396,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
Expand Down
18 changes: 9 additions & 9 deletions packages/hub/test/scope.test.ts
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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 = {};
Expand Down Expand Up @@ -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 => {
Expand Down
17 changes: 8 additions & 9 deletions packages/node/src/integrations/http.ts
Expand Up @@ -82,15 +82,14 @@ function createHandlerWrapper(
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) {
transaction = scope.getTransaction();
if (transaction) {
span = transaction.startChild({
description: `${typeof options === 'string' || !options.method ? 'GET' : options.method} ${requestUrl}`,
op: 'request',
});
}
}

return originalHandler
Expand Down
16 changes: 13 additions & 3 deletions packages/types/src/scope.ts
Expand Up @@ -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 */
Expand Down Expand Up @@ -71,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.
Expand All @@ -89,6 +89,16 @@ export interface Scope {
*/
setSpan(span?: Span): this;

/**
* Returns the `Span` if there is one
*/
getSpan(): Span | undefined;
HazAT marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns the `Transaction` if there is one
*/
getTransaction(): Transaction | undefined;

/**
* Updates the scope with provided data. Can work in three variations:
* - plain object containing updatable attributes
Expand Down
8 changes: 8 additions & 0 deletions 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 {
/**
Expand Down Expand Up @@ -133,6 +135,12 @@ export interface Span extends SpanContext {
spanContext?: Pick<SpanContext, Exclude<keyof SpanContext, 'spanId' | 'sampled' | 'traceId' | 'parentSpanId'>>,
): 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;
HazAT marked this conversation as resolved.
Show resolved Hide resolved

/**
* Determines whether span was successful (HTTP200)
*/
Expand Down