Skip to content

feat[apm]: Add discardBackgroundSpans option #2471

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

Merged
merged 2 commits into from
Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott

- [apm] feat: Add `discardBackgroundSpans` to discard background spans by default

## 5.13.1

- [node] fix: Restore engines back to `>= 6`
Expand Down
50 changes: 38 additions & 12 deletions packages/apm/src/integrations/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ interface TracingOptions {
* Default: 600
*/
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
* statistics in non deterministic ways that's why we by default recommend leaving this opition enabled.
*
* Default: true
*/
discardBackgroundSpans: boolean;
}

/** JSDoc */
Expand Down Expand Up @@ -114,9 +124,9 @@ export class Tracing implements Integration {

private static _activeTransaction?: Span;

private static _currentIndex: number = 0;
private static _currentIndex: number = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change from 0 to 1?

Copy link
Member

Choose a reason for hiding this comment

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

@rhcarvalho it looks like 0 is a magic constant returned by pushActivity() in certain cases; primarily to indicate that the activity was not pushed.


public static readonly _activities: { [key: number]: Activity } = {};
public static _activities: { [key: number]: Activity } = {};

private static _debounce: number = 0;

Expand All @@ -127,8 +137,9 @@ export class Tracing implements Integration {
*
* @param _options TracingOptions
*/
public constructor(private readonly _options?: Partial<TracingOptions>) {
public constructor(_options?: Partial<TracingOptions>) {
const defaults = {
discardBackgroundSpans: true,
idleTimeout: 500,
maxTransactionDuration: 600,
shouldCreateSpanForRequest(url: string): boolean {
Expand All @@ -148,7 +159,7 @@ export class Tracing implements Integration {
if (!_options || !Array.isArray(_options.tracingOrigins) || _options.tracingOrigins.length === 0) {
this._emitOptionsWarning = true;
}
Tracing.options = this._options = {
Tracing.options = {
...defaults,
..._options,
};
Expand All @@ -171,23 +182,21 @@ export class Tracing implements Integration {
return;
}

// tslint:disable-next-line: no-non-null-assertion
if (this._options!.traceXHR !== false) {
if (Tracing.options.traceXHR) {
addInstrumentationHandler({
callback: xhrCallback,
type: 'xhr',
});
}
// tslint:disable-next-line: no-non-null-assertion
if (this._options!.traceFetch !== false && supportsNativeFetch()) {

if (Tracing.options.traceFetch && supportsNativeFetch()) {
addInstrumentationHandler({
callback: fetchCallback,
type: 'fetch',
});
}

// tslint:disable-next-line: no-non-null-assertion
if (this._options!.startTransactionOnLocationChange) {
if (Tracing.options.startTransactionOnLocationChange) {
addInstrumentationHandler({
callback: historyCallback,
type: 'history',
Expand All @@ -202,6 +211,16 @@ export class Tracing implements Integration {
});
}

if (Tracing.options.discardBackgroundSpans && global.document) {
document.addEventListener('visibilitychange', () => {
if (document.hidden && Tracing._activeTransaction) {
logger.log('[Tracing] Discarded active transaction incl. activities since tab moved to the background');
Tracing._activeTransaction = undefined;
Tracing._activities = {};
}
});
Copy link
Member

@dashed dashed Mar 4, 2020

Choose a reason for hiding this comment

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

@HazAT Should startIdleTransaction() be called whenever document.hidden === false? e.g. user focuses the tab.

Copy link
Member

Choose a reason for hiding this comment

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

@dashed What would that be measuring? Normally our auto-created transactions are about pageload, but if the page is already loaded, and you just refocus the tab...

}

// This EventProcessor makes sure that the transaction is not longer than maxTransactionDuration
addGlobalEventProcessor((event: Event) => {
const self = getCurrentHub().getIntegration(Tracing);
Expand Down Expand Up @@ -351,6 +370,10 @@ export class Tracing implements Integration {
// Tracing is not enabled
return 0;
}
if (!Tracing._activeTransaction) {
logger.log(`[Tracing] Not pushing activity ${name} since there is no active transaction`);
return 0;
}

// We want to clear the timeout also here since we push a new activity
clearTimeout(Tracing._debounce);
Expand Down Expand Up @@ -389,7 +412,10 @@ export class Tracing implements Integration {
* Removes activity and finishes the span in case there is one
*/
public static popActivity(id: number, spanData?: { [key: string]: any }): void {
if (!Tracing._isEnabled()) {
// The !id is on purpose to also fail with 0
// Since 0 is returned by push activity in case tracing is not enabled
Comment on lines +415 to +416
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this case already covered by !Tracing._isEnabled()?

Copy link
Member

Choose a reason for hiding this comment

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

@rhcarvalho there are cases when popActivity() is called within a setTimeout()

// or there is no active transaction
if (!Tracing._isEnabled() || !id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!Tracing._isEnabled() || !id) {
if (!Tracing._isEnabled() || id == null) {

or typeof id === 'undefined' since !0 is truthy.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is on purpose, will add a code comment.

Copy link
Member

Choose a reason for hiding this comment

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

id <= 0 would also work 👍

// Tracing is not enabled
return;
}
Expand Down Expand Up @@ -422,7 +448,7 @@ export class Tracing implements Integration {

logger.log('[Tracing] activies count', count);

if (count === 0) {
if (count === 0 && Tracing._activeTransaction) {
const timeout = Tracing.options && Tracing.options.idleTimeout;
logger.log(`[Tracing] Flushing Transaction in ${timeout}ms`);
Tracing._debounce = (setTimeout(() => {
Expand Down