From 69270cd38b4fea68f0432c075b9222724e1cc6da Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 9 Feb 2022 14:18:43 -0500 Subject: [PATCH 01/12] switch to using window.setTimeout --- packages/tracing/src/idletransaction.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/tracing/src/idletransaction.ts b/packages/tracing/src/idletransaction.ts index 7e876bb18660..8ec02ec7f1e9 100644 --- a/packages/tracing/src/idletransaction.ts +++ b/packages/tracing/src/idletransaction.ts @@ -1,6 +1,6 @@ import { Hub } from '@sentry/hub'; import { TransactionContext } from '@sentry/types'; -import { logger, timestampWithMs } from '@sentry/utils'; +import { getGlobalObject, logger, timestampWithMs } from '@sentry/utils'; import { FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS } from './constants'; import { Span, SpanRecorder } from './span'; @@ -9,6 +9,8 @@ import { Transaction } from './transaction'; export const DEFAULT_IDLE_TIMEOUT = 1000; export const HEARTBEAT_INTERVAL = 5000; +const global = getGlobalObject(); + /** * @inheritDoc */ @@ -71,7 +73,7 @@ export class IdleTransaction extends Transaction { * If a transaction is created and no activities are added, we want to make sure that * it times out properly. This is cleared and not used when activities are added. */ - private _initTimeout: ReturnType | undefined; + private _initTimeout: ReturnType | undefined; public constructor( transactionContext: TransactionContext, @@ -96,7 +98,7 @@ export class IdleTransaction extends Transaction { _idleHub.configureScope(scope => scope.setSpan(this)); } - this._initTimeout = setTimeout(() => { + this._initTimeout = global.setTimeout(() => { if (!this._finished) { this.finish(); } @@ -221,7 +223,7 @@ export class IdleTransaction extends Transaction { // Remember timestampWithMs is in seconds, timeout is in ms const end = timestampWithMs() + timeout / 1000; - setTimeout(() => { + global.setTimeout(() => { if (!this._finished) { this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[1]); this.finish(end); @@ -265,7 +267,7 @@ export class IdleTransaction extends Transaction { */ private _pingHeartbeat(): void { logger.log(`pinging Heartbeat -> current counter: ${this._heartbeatCounter}`); - setTimeout(() => { + global.setTimeout(() => { this._beat(); }, HEARTBEAT_INTERVAL); } From 3dacee46af39feb5a95b0ad778d246e32637ee37 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 9 Feb 2022 15:38:06 -0500 Subject: [PATCH 02/12] feat(tracing): Reset IdleTimeout based on activities count --- .../tracing/src/browser/browsertracing.ts | 14 ++++- packages/tracing/src/constants.ts | 7 ++- packages/tracing/src/idletransaction.ts | 57 ++++++++++++++----- .../test/browser/browsertracing.test.ts | 3 +- packages/tracing/test/idletransaction.test.ts | 37 +++++++----- 5 files changed, 87 insertions(+), 31 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 4218f8e84fd7..7a530559b799 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -3,7 +3,7 @@ import { EventProcessor, Integration, Transaction, TransactionContext } from '@s import { getGlobalObject, logger } from '@sentry/utils'; import { startIdleTransaction } from '../hubextensions'; -import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction'; +import { DEFAULT_FINAL_TIMEOUT, DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction'; import { extractTraceparentData, secToMs } from '../utils'; import { registerBackgroundTabDetection } from './backgroundtab'; import { MetricsInstrumentation } from './metrics'; @@ -21,12 +21,23 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { /** * The time to wait in ms until the transaction will be finished. The transaction will use the end timestamp of * the last finished span as the endtime for the transaction. + * * Time is in ms. * * Default: 1000 */ idleTimeout: number; + /** + * The max transaction duration for a transaction. If a transaction duration hits the `finalTimeout` value, it + * will be finished. + * + * Time is in ms. + * + * Default: 30000 + */ + finalTimeout: number; + /** * Flag to enable/disable creation of `navigation` transaction on history changes. * @@ -93,6 +104,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { const DEFAULT_BROWSER_TRACING_OPTIONS = { idleTimeout: DEFAULT_IDLE_TIMEOUT, + finalTimeout: DEFAULT_FINAL_TIMEOUT, markBackgroundTransactions: true, maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS, routingInstrumentation: instrumentRoutingWithDefaults, diff --git a/packages/tracing/src/constants.ts b/packages/tracing/src/constants.ts index 3f01982c3614..2d02f3099571 100644 --- a/packages/tracing/src/constants.ts +++ b/packages/tracing/src/constants.ts @@ -2,4 +2,9 @@ // Readonly type should enforce that this is not mutated. export const FINISH_REASON_TAG = 'finishReason'; -export const IDLE_TRANSACTION_FINISH_REASONS = ['heartbeatFailed', 'idleTimeout', 'documentHidden'] as const; +export const IDLE_TRANSACTION_FINISH_REASONS = [ + 'heartbeatFailed', + 'idleTimeout', + 'documentHidden', + 'finalTimeout', +] as const; diff --git a/packages/tracing/src/idletransaction.ts b/packages/tracing/src/idletransaction.ts index 8ec02ec7f1e9..94492db7ba46 100644 --- a/packages/tracing/src/idletransaction.ts +++ b/packages/tracing/src/idletransaction.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines */ import { Hub } from '@sentry/hub'; import { TransactionContext } from '@sentry/types'; import { getGlobalObject, logger, timestampWithMs } from '@sentry/utils'; @@ -7,6 +8,7 @@ import { Span, SpanRecorder } from './span'; import { Transaction } from './transaction'; export const DEFAULT_IDLE_TIMEOUT = 1000; +export const DEFAULT_FINAL_TIMEOUT = 30000; export const HEARTBEAT_INTERVAL = 5000; const global = getGlobalObject(); @@ -73,7 +75,7 @@ export class IdleTransaction extends Transaction { * If a transaction is created and no activities are added, we want to make sure that * it times out properly. This is cleared and not used when activities are added. */ - private _initTimeout: ReturnType | undefined; + private _idleTimeoutID: ReturnType | undefined; public constructor( transactionContext: TransactionContext, @@ -81,10 +83,21 @@ export class IdleTransaction extends Transaction { /** * The time to wait in ms until the idle transaction will be finished. * @default 1000 + * + * TODO: Make _idleTimeout and _finalTimeout required to reduce duplication when setting the options + * in `BrowserTracing`. This is considered a breaking change to the IdleTransaction API, + * so we need to make sure we communicate it with react native. */ private readonly _idleTimeout: number = DEFAULT_IDLE_TIMEOUT, // Whether or not the transaction should put itself on the scope when it starts and pop itself off when it ends private readonly _onScope: boolean = false, + /** + * The final value that a transaction cannot exceed + * @default 15000 + * @experimental + * @internal + */ + private readonly _finalTimeout: number = DEFAULT_FINAL_TIMEOUT, ) { super(transactionContext, _idleHub); @@ -98,11 +111,13 @@ export class IdleTransaction extends Transaction { _idleHub.configureScope(scope => scope.setSpan(this)); } - this._initTimeout = global.setTimeout(() => { + this._startIdleTimeout(); + global.setTimeout(() => { if (!this._finished) { + this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[3]); this.finish(); } - }, this._idleTimeout); + }, this._finalTimeout); } /** {@inheritDoc} */ @@ -191,15 +206,35 @@ export class IdleTransaction extends Transaction { this.spanRecorder.add(this); } + /** + * Creates an idletimeout + */ + private _cancelIdleTimeout(): void { + if (this._idleTimeoutID) { + global.clearTimeout(this._idleTimeoutID); + this._idleTimeoutID = undefined; + } + } + + /** + * Creates an idletimeout + */ + private _startIdleTimeout(end?: Parameters[0]): void { + this._cancelIdleTimeout(); + this._idleTimeoutID = global.setTimeout(() => { + if (!this._finished && Object.keys(this.activities).length === 0) { + this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[1]); + this.finish(end); + } + }, this._idleTimeout); + } + /** * Start tracking a specific activity. * @param spanId The span id that represents the activity */ private _pushActivity(spanId: string): void { - if (this._initTimeout) { - clearTimeout(this._initTimeout); - this._initTimeout = undefined; - } + this._cancelIdleTimeout(); logger.log(`[Tracing] pushActivity: ${spanId}`); this.activities[spanId] = true; logger.log('[Tracing] new activities count', Object.keys(this.activities).length); @@ -222,13 +257,7 @@ export class IdleTransaction extends Transaction { // We need to add the timeout here to have the real endtimestamp of the transaction // Remember timestampWithMs is in seconds, timeout is in ms const end = timestampWithMs() + timeout / 1000; - - global.setTimeout(() => { - if (!this._finished) { - this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[1]); - this.finish(end); - } - }, timeout); + this._startIdleTimeout(end); } } diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 5658dd34858c..fcfc3138cf13 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -14,7 +14,7 @@ import { MetricsInstrumentation } from '../../src/browser/metrics'; import { defaultRequestInstrumentationOptions } from '../../src/browser/request'; import { instrumentRoutingWithDefaults } from '../../src/browser/router'; import * as hubExtensions from '../../src/hubextensions'; -import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../../src/idletransaction'; +import { DEFAULT_IDLE_TIMEOUT, IdleTransaction, DEFAULT_FINAL_TIMEOUT } from '../../src/idletransaction'; import { getActiveTransaction, secToMs } from '../../src/utils'; let mockChangeHistory: ({ to, from }: { to: string; from?: string }) => void = () => undefined; @@ -83,6 +83,7 @@ describe('BrowserTracing', () => { expect(browserTracing.options).toEqual({ idleTimeout: DEFAULT_IDLE_TIMEOUT, + finalTimeout: DEFAULT_FINAL_TIMEOUT, markBackgroundTransactions: true, maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS, routingInstrumentation: instrumentRoutingWithDefaults, diff --git a/packages/tracing/test/idletransaction.test.ts b/packages/tracing/test/idletransaction.test.ts index b60fec726c54..b229ae349902 100644 --- a/packages/tracing/test/idletransaction.test.ts +++ b/packages/tracing/test/idletransaction.test.ts @@ -9,7 +9,16 @@ import { } from '../src/idletransaction'; import { Span } from '../src/span'; -export class SimpleTransport extends Transports.BaseTransport {} +type TransportSendRequest = Transports.BaseTransport['_sendRequest']; + +export class SimpleTransport extends Transports.BaseTransport { + protected _sendRequest( + _req: Parameters[0], + _payload: Parameters[1], + ): ReturnType { + throw new Error('Method not implemented.'); + } +} const dsn = 'https://123@sentry.io/42'; let hub: Hub; @@ -103,7 +112,7 @@ describe('IdleTransaction', () => { expect(transaction.activities).toMatchObject({ [span.spanId]: true, [childSpan.spanId]: true }); span.finish(); - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(DEFAULT_IDLE_TIMEOUT + 1); expect(mockFinish).toHaveBeenCalledTimes(0); expect(transaction.activities).toMatchObject({ [childSpan.spanId]: true }); @@ -229,20 +238,20 @@ describe('IdleTransaction', () => { transaction.startChild({}); // Beat 1 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 3 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(1); }); it('resets after new activities are added', () => { - const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT); + const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT, false, 50000); const mockFinish = jest.spyOn(transaction, 'finish'); transaction.initSpanRecorder(10); @@ -250,42 +259,42 @@ describe('IdleTransaction', () => { transaction.startChild({}); // Beat 1 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); const span = transaction.startChild(); // push activity // Beat 1 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); transaction.startChild(); // push activity transaction.startChild(); // push activity // Beat 1 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); span.finish(); // pop activity // Beat 1 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 3 - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(1); // Heartbeat does not keep going after finish has been called From 52e642b19dd9a11cbba17b898973a28b178b2193 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 9 Feb 2022 15:45:08 -0500 Subject: [PATCH 03/12] update default --- packages/tracing/src/idletransaction.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/idletransaction.ts b/packages/tracing/src/idletransaction.ts index 94492db7ba46..4e29feb56d8f 100644 --- a/packages/tracing/src/idletransaction.ts +++ b/packages/tracing/src/idletransaction.ts @@ -93,7 +93,7 @@ export class IdleTransaction extends Transaction { private readonly _onScope: boolean = false, /** * The final value that a transaction cannot exceed - * @default 15000 + * @default 30000 * @experimental * @internal */ From 8c35b6dbae9997fbd6b15035f89573b9e665c748 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 9 Feb 2022 18:53:59 -0500 Subject: [PATCH 04/12] prettier fix --- packages/tracing/test/browser/browsertracing.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index fcfc3138cf13..b2117877690e 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -14,7 +14,7 @@ import { MetricsInstrumentation } from '../../src/browser/metrics'; import { defaultRequestInstrumentationOptions } from '../../src/browser/request'; import { instrumentRoutingWithDefaults } from '../../src/browser/router'; import * as hubExtensions from '../../src/hubextensions'; -import { DEFAULT_IDLE_TIMEOUT, IdleTransaction, DEFAULT_FINAL_TIMEOUT } from '../../src/idletransaction'; +import { DEFAULT_FINAL_TIMEOUT, DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../../src/idletransaction'; import { getActiveTransaction, secToMs } from '../../src/utils'; let mockChangeHistory: ({ to, from }: { to: string; from?: string }) => void = () => undefined; From e0f9ca0c1b59b99314502052d39d9c99c455cc99 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 28 Feb 2022 13:22:47 -0500 Subject: [PATCH 05/12] make sure all finish reasons are defined --- .../tracing/src/browser/browsertracing.ts | 2 +- packages/tracing/src/constants.ts | 3 ++- packages/tracing/src/idletransaction.ts | 26 +++++++++++++------ 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 7a530559b799..d5f23f9395c0 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -29,7 +29,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { idleTimeout: number; /** - * The max transaction duration for a transaction. If a transaction duration hits the `finalTimeout` value, it + * The max duration for a transaction. If a transaction duration hits the `finalTimeout` value, it * will be finished. * * Time is in ms. diff --git a/packages/tracing/src/constants.ts b/packages/tracing/src/constants.ts index 2d02f3099571..801e1d315e3a 100644 --- a/packages/tracing/src/constants.ts +++ b/packages/tracing/src/constants.ts @@ -1,10 +1,11 @@ // Store finish reasons in tuple to save on bundle size // Readonly type should enforce that this is not mutated. -export const FINISH_REASON_TAG = 'finishReason'; +export const FINISH_REASON_TAG = 'finishReason' as const; export const IDLE_TRANSACTION_FINISH_REASONS = [ 'heartbeatFailed', 'idleTimeout', 'documentHidden', 'finalTimeout', + 'externalFinish', ] as const; diff --git a/packages/tracing/src/idletransaction.ts b/packages/tracing/src/idletransaction.ts index 4e29feb56d8f..9130644732b3 100644 --- a/packages/tracing/src/idletransaction.ts +++ b/packages/tracing/src/idletransaction.ts @@ -77,6 +77,14 @@ export class IdleTransaction extends Transaction { */ private _idleTimeoutID: ReturnType | undefined; + /** + * + * Defaults to `externalFinish`, which means transaction.finish() was called outside of the idle transaction (for example, + * by a navigation transaction ending the previous pageload/navigation in some routing instrumentation). + * @default 'externalFinish' + */ + private _finishReason: typeof IDLE_TRANSACTION_FINISH_REASONS[number] = IDLE_TRANSACTION_FINISH_REASONS[4]; + public constructor( transactionContext: TransactionContext, private readonly _idleHub?: Hub, @@ -114,7 +122,7 @@ export class IdleTransaction extends Transaction { this._startIdleTimeout(); global.setTimeout(() => { if (!this._finished) { - this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[3]); + this._finishReason = IDLE_TRANSACTION_FINISH_REASONS[3]; /* 'finalTimeout' */ this.finish(); } }, this._finalTimeout); @@ -125,6 +133,8 @@ export class IdleTransaction extends Transaction { this._finished = true; this.activities = {}; + this.setTag(FINISH_REASON_TAG, this._finishReason); + if (this.spanRecorder) { logger.log('[Tracing] finishing IdleTransaction', new Date(endTimestamp * 1000).toISOString(), this.op); @@ -207,7 +217,7 @@ export class IdleTransaction extends Transaction { } /** - * Creates an idletimeout + * Cancels the existing idletimeout, if there is one */ private _cancelIdleTimeout(): void { if (this._idleTimeoutID) { @@ -219,12 +229,12 @@ export class IdleTransaction extends Transaction { /** * Creates an idletimeout */ - private _startIdleTimeout(end?: Parameters[0]): void { + private _startIdleTimeout(endTimestamp?: Parameters[0]): void { this._cancelIdleTimeout(); this._idleTimeoutID = global.setTimeout(() => { if (!this._finished && Object.keys(this.activities).length === 0) { - this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[1]); - this.finish(end); + this._finishReason = IDLE_TRANSACTION_FINISH_REASONS[1]; /* 'idleTimeout' */ + this.finish(endTimestamp); } }, this._idleTimeout); } @@ -256,8 +266,8 @@ export class IdleTransaction extends Transaction { const timeout = this._idleTimeout; // We need to add the timeout here to have the real endtimestamp of the transaction // Remember timestampWithMs is in seconds, timeout is in ms - const end = timestampWithMs() + timeout / 1000; - this._startIdleTimeout(end); + const endTimestamp = timestampWithMs() + timeout / 1000; + this._startIdleTimeout(endTimestamp); } } @@ -284,7 +294,7 @@ export class IdleTransaction extends Transaction { if (this._heartbeatCounter >= 3) { logger.log(`[Tracing] Transaction finished because of no change for 3 heart beats`); this.setStatus('deadline_exceeded'); - this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[0]); + this._finishReason = IDLE_TRANSACTION_FINISH_REASONS[0]; /* 'heartbeatFailed' */ this.finish(); } else { this._pingHeartbeat(); From e68a672e9d7c1d5dc4ab32f5e91388a53dbe1dbb Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 28 Feb 2022 14:33:34 -0500 Subject: [PATCH 06/12] force report lcp on finish --- packages/tracing/src/browser/metrics.ts | 9 ++++++++- packages/tracing/src/browser/web-vitals/getLCP.ts | 13 ++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 93253fcf2d0f..555a508df385 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -21,6 +21,7 @@ export class MetricsInstrumentation { private _performanceCursor: number = 0; private _lcpEntry: LargestContentfulPaint | undefined; private _clsEntry: LayoutShift | undefined; + private _reportLCP: ReturnType; public constructor(private _reportAllChanges: boolean = false) { if (!isNodeEnv() && global && global.performance && global.document) { @@ -43,6 +44,12 @@ export class MetricsInstrumentation { logger.log('[Tracing] Adding & adjusting spans using Performance API'); + // `addPerformanceEntries` should be called on transaction.finish + // If we are finishing a transaction, we should force report the LCP. + if (this._reportLCP) { + this._reportLCP(true); + } + const timeOrigin = msToSec(browserPerformanceTimeOrigin); let responseStartTimestamp: number | undefined; @@ -223,7 +230,7 @@ export class MetricsInstrumentation { /** Starts tracking the Largest Contentful Paint on the current page. */ private _trackLCP(): void { - getLCP(metric => { + this._reportLCP = getLCP(metric => { const entry = metric.entries.pop(); if (!entry) { return; diff --git a/packages/tracing/src/browser/web-vitals/getLCP.ts b/packages/tracing/src/browser/web-vitals/getLCP.ts index 8b00dc417746..415f53523e99 100644 --- a/packages/tracing/src/browser/web-vitals/getLCP.ts +++ b/packages/tracing/src/browser/web-vitals/getLCP.ts @@ -34,10 +34,13 @@ export interface LargestContentfulPaint extends PerformanceEntry { const reportedMetricIDs: Record = {}; -export const getLCP = (onReport: ReportHandler, reportAllChanges?: boolean): void => { +export const getLCP = ( + onReport: ReportHandler, + reportAllChanges?: boolean, +): ReturnType | undefined => { const visibilityWatcher = getVisibilityWatcher(); const metric = initMetric('LCP'); - let report: ReturnType; + let report: ReturnType | undefined; const entryHandler = (entry: PerformanceEntry): void => { // The startTime attribute returns the value of the renderTime if it is not 0, @@ -66,7 +69,9 @@ export const getLCP = (onReport: ReportHandler, reportAllChanges?: boolean): voi po.takeRecords().map(entryHandler as PerformanceEntryHandler); po.disconnect(); reportedMetricIDs[metric.id] = true; - report(true); + if (report) { + report(true); + } } }; @@ -79,4 +84,6 @@ export const getLCP = (onReport: ReportHandler, reportAllChanges?: boolean): voi onHidden(stopListening, true); } + + return report; }; From c6eddb3f6af9731e01e1c0a5e2ca67d2df22ba03 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 1 Mar 2022 12:49:04 -0500 Subject: [PATCH 07/12] Revert "force report lcp on finish" This reverts commit e68a672e9d7c1d5dc4ab32f5e91388a53dbe1dbb. --- packages/tracing/src/browser/metrics.ts | 9 +-------- packages/tracing/src/browser/web-vitals/getLCP.ts | 13 +++---------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 555a508df385..93253fcf2d0f 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -21,7 +21,6 @@ export class MetricsInstrumentation { private _performanceCursor: number = 0; private _lcpEntry: LargestContentfulPaint | undefined; private _clsEntry: LayoutShift | undefined; - private _reportLCP: ReturnType; public constructor(private _reportAllChanges: boolean = false) { if (!isNodeEnv() && global && global.performance && global.document) { @@ -44,12 +43,6 @@ export class MetricsInstrumentation { logger.log('[Tracing] Adding & adjusting spans using Performance API'); - // `addPerformanceEntries` should be called on transaction.finish - // If we are finishing a transaction, we should force report the LCP. - if (this._reportLCP) { - this._reportLCP(true); - } - const timeOrigin = msToSec(browserPerformanceTimeOrigin); let responseStartTimestamp: number | undefined; @@ -230,7 +223,7 @@ export class MetricsInstrumentation { /** Starts tracking the Largest Contentful Paint on the current page. */ private _trackLCP(): void { - this._reportLCP = getLCP(metric => { + getLCP(metric => { const entry = metric.entries.pop(); if (!entry) { return; diff --git a/packages/tracing/src/browser/web-vitals/getLCP.ts b/packages/tracing/src/browser/web-vitals/getLCP.ts index 415f53523e99..8b00dc417746 100644 --- a/packages/tracing/src/browser/web-vitals/getLCP.ts +++ b/packages/tracing/src/browser/web-vitals/getLCP.ts @@ -34,13 +34,10 @@ export interface LargestContentfulPaint extends PerformanceEntry { const reportedMetricIDs: Record = {}; -export const getLCP = ( - onReport: ReportHandler, - reportAllChanges?: boolean, -): ReturnType | undefined => { +export const getLCP = (onReport: ReportHandler, reportAllChanges?: boolean): void => { const visibilityWatcher = getVisibilityWatcher(); const metric = initMetric('LCP'); - let report: ReturnType | undefined; + let report: ReturnType; const entryHandler = (entry: PerformanceEntry): void => { // The startTime attribute returns the value of the renderTime if it is not 0, @@ -69,9 +66,7 @@ export const getLCP = ( po.takeRecords().map(entryHandler as PerformanceEntryHandler); po.disconnect(); reportedMetricIDs[metric.id] = true; - if (report) { - report(true); - } + report(true); } }; @@ -84,6 +79,4 @@ export const getLCP = ( onHidden(stopListening, true); } - - return report; }; From 33e3f5da827dd1f6ac4ec469a82a636579b48b87 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 1 Mar 2022 13:16:45 -0500 Subject: [PATCH 08/12] fix tests --- packages/tracing/test/browser/browsertracing.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index b2117877690e..18d32dd285af 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -236,9 +236,8 @@ describe('BrowserTracing', () => { describe('idleTimeout', () => { it('is created by default', () => { createBrowserTracing(true, { routingInstrumentation: customInstrumentRouting }); - const mockFinish = jest.fn(); const transaction = getActiveTransaction(hub) as IdleTransaction; - transaction.finish = mockFinish; + const mockFinish = jest.spyOn(transaction, 'finish'); const span = transaction.startChild(); // activities = 1 span.finish(); // activities = 0 @@ -252,9 +251,8 @@ describe('BrowserTracing', () => { it('can be a custom value', () => { createBrowserTracing(true, { idleTimeout: 2000, routingInstrumentation: customInstrumentRouting }); - const mockFinish = jest.fn(); const transaction = getActiveTransaction(hub) as IdleTransaction; - transaction.finish = mockFinish; + const mockFinish = jest.spyOn(transaction, 'finish'); const span = transaction.startChild(); // activities = 1 span.finish(); // activities = 0 From a6fc32fae4c93857875ab2d88f2f8375f9cfd3a1 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 1 Mar 2022 13:33:05 -0500 Subject: [PATCH 09/12] make finishReason public --- packages/tracing/src/browser/backgroundtab.ts | 2 +- packages/tracing/src/idletransaction.ts | 24 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/tracing/src/browser/backgroundtab.ts b/packages/tracing/src/browser/backgroundtab.ts index 3701404b5d26..de974136f3da 100644 --- a/packages/tracing/src/browser/backgroundtab.ts +++ b/packages/tracing/src/browser/backgroundtab.ts @@ -27,7 +27,7 @@ export function registerBackgroundTabDetection(): void { activeTransaction.setStatus(statusType); } activeTransaction.setTag('visibilitychange', 'document.hidden'); - activeTransaction.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[2]); + activeTransaction.finishReason = IDLE_TRANSACTION_FINISH_REASONS[2]; /* 'documentHidden' */ activeTransaction.finish(); } }); diff --git a/packages/tracing/src/idletransaction.ts b/packages/tracing/src/idletransaction.ts index 9130644732b3..d60b7af70c6f 100644 --- a/packages/tracing/src/idletransaction.ts +++ b/packages/tracing/src/idletransaction.ts @@ -60,6 +60,14 @@ export class IdleTransaction extends Transaction { // Activities store a list of active spans public activities: Record = {}; + /** + * + * Defaults to `externalFinish`, which means transaction.finish() was called outside of the idle transaction (for example, + * by a navigation transaction ending the previous pageload/navigation in some routing instrumentation). + * @default 'externalFinish' + */ + public finishReason: typeof IDLE_TRANSACTION_FINISH_REASONS[number] = IDLE_TRANSACTION_FINISH_REASONS[4]; + // Track state of activities in previous heartbeat private _prevHeartbeatString: string | undefined; @@ -77,14 +85,6 @@ export class IdleTransaction extends Transaction { */ private _idleTimeoutID: ReturnType | undefined; - /** - * - * Defaults to `externalFinish`, which means transaction.finish() was called outside of the idle transaction (for example, - * by a navigation transaction ending the previous pageload/navigation in some routing instrumentation). - * @default 'externalFinish' - */ - private _finishReason: typeof IDLE_TRANSACTION_FINISH_REASONS[number] = IDLE_TRANSACTION_FINISH_REASONS[4]; - public constructor( transactionContext: TransactionContext, private readonly _idleHub?: Hub, @@ -122,7 +122,7 @@ export class IdleTransaction extends Transaction { this._startIdleTimeout(); global.setTimeout(() => { if (!this._finished) { - this._finishReason = IDLE_TRANSACTION_FINISH_REASONS[3]; /* 'finalTimeout' */ + this.finishReason = IDLE_TRANSACTION_FINISH_REASONS[3]; /* 'finalTimeout' */ this.finish(); } }, this._finalTimeout); @@ -133,7 +133,7 @@ export class IdleTransaction extends Transaction { this._finished = true; this.activities = {}; - this.setTag(FINISH_REASON_TAG, this._finishReason); + this.setTag(FINISH_REASON_TAG, this.finishReason); if (this.spanRecorder) { logger.log('[Tracing] finishing IdleTransaction', new Date(endTimestamp * 1000).toISOString(), this.op); @@ -233,7 +233,7 @@ export class IdleTransaction extends Transaction { this._cancelIdleTimeout(); this._idleTimeoutID = global.setTimeout(() => { if (!this._finished && Object.keys(this.activities).length === 0) { - this._finishReason = IDLE_TRANSACTION_FINISH_REASONS[1]; /* 'idleTimeout' */ + this.finishReason = IDLE_TRANSACTION_FINISH_REASONS[1]; /* 'idleTimeout' */ this.finish(endTimestamp); } }, this._idleTimeout); @@ -294,7 +294,7 @@ export class IdleTransaction extends Transaction { if (this._heartbeatCounter >= 3) { logger.log(`[Tracing] Transaction finished because of no change for 3 heart beats`); this.setStatus('deadline_exceeded'); - this._finishReason = IDLE_TRANSACTION_FINISH_REASONS[0]; /* 'heartbeatFailed' */ + this.finishReason = IDLE_TRANSACTION_FINISH_REASONS[0]; /* 'heartbeatFailed' */ this.finish(); } else { this._pingHeartbeat(); From 5b8d70141d64ccee2258c64791584c452614730b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 1 Mar 2022 13:40:09 -0500 Subject: [PATCH 10/12] remove import --- packages/tracing/src/browser/backgroundtab.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/browser/backgroundtab.ts b/packages/tracing/src/browser/backgroundtab.ts index de974136f3da..2d9a147e8bfa 100644 --- a/packages/tracing/src/browser/backgroundtab.ts +++ b/packages/tracing/src/browser/backgroundtab.ts @@ -1,6 +1,6 @@ import { getGlobalObject, logger } from '@sentry/utils'; -import { FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS } from '../constants'; +import { IDLE_TRANSACTION_FINISH_REASONS } from '../constants'; import { IdleTransaction } from '../idletransaction'; import { SpanStatusType } from '../span'; import { getActiveTransaction } from '../utils'; From a7cbb92503fff7769a7f5d12a5a15f4dfc2c0afb Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 1 Mar 2022 14:12:21 -0500 Subject: [PATCH 11/12] remove finish reason --- .../suites/tracing/browsertracing/backgroundtab-custom/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/integration-tests/suites/tracing/browsertracing/backgroundtab-custom/test.ts b/packages/integration-tests/suites/tracing/browsertracing/backgroundtab-custom/test.ts index a21cff2f1831..8a50d643e0c9 100644 --- a/packages/integration-tests/suites/tracing/browsertracing/backgroundtab-custom/test.ts +++ b/packages/integration-tests/suites/tracing/browsertracing/backgroundtab-custom/test.ts @@ -36,5 +36,5 @@ sentryTest('should finish a custom transaction when the page goes background', a expect(id_before).toBe(id_after); expect(name_after).toBe(name_before); expect(status_after).toBe('cancelled'); - expect(tags_after).toStrictEqual({ finishReason: 'documentHidden', visibilitychange: 'document.hidden' }); + expect(tags_after).toStrictEqual({ visibilitychange: 'document.hidden' }); }); From ce398d97795052dbab539ea36d49ca19ab64d223 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 1 Mar 2022 12:56:35 -0500 Subject: [PATCH 12/12] [DO NOT MERGE] 6.18.1-beta.0 branch This branch tracks the beta for `6.18.1-beta.0`. For more information see: https://github.com/getsentry/sentry-javascript/pull/4531. Prev beta: https://github.com/getsentry/sentry-javascript/pull/4554 --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8b579c25cc5..d2805f3340d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 6.18.1-beta.0 + +This patch builds on top of the https://github.com/getsentry/sentry-javascript/releases/tag/6.17.8-beta.0 beta. +It adds an additional finish reason that covers the case where a transaction was getting externally finished. + +- feat(tracing): Reset IdleTimeout based on activities count ([#4531](https://github.com/getsentry/sentry-javascript/pull/4531)) + ## 6.18.1 - fix(ember): use _backburner if it exists ([#4603](https://github.com/getsentry/sentry-javascript/pull/4603))