feat[apm]: Add discardBackgroundSpans option #2471
Conversation
| @@ -389,7 +414,7 @@ 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()) { | |||
| if (!Tracing._isEnabled() || !id) { | |||
There was a problem hiding this comment.
| if (!Tracing._isEnabled() || !id) { | |
| if (!Tracing._isEnabled() || id == null) { |
or typeof id === 'undefined' since !0 is truthy.
There was a problem hiding this comment.
This is on purpose, will add a code comment.
| @@ -202,6 +213,16 @@ export class Tracing implements Integration { | |||
| }); | |||
| } | |||
|
|
|||
| // tslint:disable-next-line: no-non-null-assertion | |||
| if (this._options!.discardBackgroundSpans && global.document) { | |||
There was a problem hiding this comment.
Why do we have this._options! in the first place? We should allow this._options to be undefined by this point.
HazAT
changed the title
| @@ -114,9 +124,9 @@ export class Tracing implements Integration { | |||
|
|
|||
| private static _activeTransaction?: Span; | |||
|
|
|||
| private static _currentIndex: number = 0; | |||
| private static _currentIndex: number = 1; | |||
There was a problem hiding this comment.
Why did this change from 0 to 1?
There was a problem hiding this comment.
@rhcarvalho it looks like 0 is a magic constant returned by pushActivity() in certain cases; primarily to indicate that the activity was not pushed.
| // The !id is on purpose to also fail with 0 | ||
| // Since 0 is returned by push activity in case tracing is not enabled |
There was a problem hiding this comment.
Isn't this case already covered by !Tracing._isEnabled()?
There was a problem hiding this comment.
@rhcarvalho there are cases when popActivity() is called within a setTimeout()
| Tracing._activeTransaction = undefined; | ||
| Tracing._activities = {}; | ||
| } | ||
| }); |
There was a problem hiding this comment.
@HazAT Should startIdleTransaction() be called whenever document.hidden === false? e.g. user focuses the tab.
There was a problem hiding this comment.
@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 PR discards spans/transactions (also does not add the
sentry-traceheader for xhr & fetch) if the tab is/gets moved to the background by using the visibility API https://developers.google.com/web/updates/2017/03/background_tabs#optimising_an_application_for_backgroundTiming in background is off, but the code comment for the option is explaining why it's there.