-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 */ | ||||||
|
@@ -114,9 +124,9 @@ export class Tracing implements Integration { | |||||
|
||||||
private static _activeTransaction?: Span; | ||||||
|
||||||
private static _currentIndex: number = 0; | ||||||
private static _currentIndex: number = 1; | ||||||
|
||||||
public static readonly _activities: { [key: number]: Activity } = {}; | ||||||
public static _activities: { [key: number]: Activity } = {}; | ||||||
|
||||||
private static _debounce: number = 0; | ||||||
|
||||||
|
@@ -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 { | ||||||
|
@@ -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, | ||||||
}; | ||||||
|
@@ -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', | ||||||
|
@@ -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 = {}; | ||||||
} | ||||||
}); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HazAT Should There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||
|
@@ -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); | ||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this case already covered by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rhcarvalho there are cases when |
||||||
// or there is no active transaction | ||||||
if (!Tracing._isEnabled() || !id) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is on purpose, will add a code comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
// Tracing is not enabled | ||||||
return; | ||||||
} | ||||||
|
@@ -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(() => { | ||||||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 bypushActivity()
in certain cases; primarily to indicate that the activity was not pushed.