From 0d1b6cc96b068261d564bf65d63aad265c6fb5d0 Mon Sep 17 00:00:00 2001 From: Outsider Date: Sun, 2 Oct 2022 18:21:19 +0900 Subject: [PATCH 1/7] feat(browser): make heartbeat interval configurable Signed-off-by: Outsider --- .../tracing/src/browser/browsertracing.ts | 14 +++++- packages/tracing/src/hubextensions.ts | 3 +- packages/tracing/src/idletransaction.ts | 5 +- .../test/browser/browsertracing.test.ts | 9 +++- packages/tracing/test/idletransaction.test.ts | 49 ++++++++++++------- 5 files changed, 57 insertions(+), 23 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 8a06f029d53f..c336e67a5da0 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -4,7 +4,7 @@ import { EventProcessor, Integration, Transaction, TransactionContext } from '@s import { baggageHeaderToDynamicSamplingContext, getDomElement, getGlobalObject, logger } from '@sentry/utils'; import { startIdleTransaction } from '../hubextensions'; -import { DEFAULT_FINAL_TIMEOUT, DEFAULT_IDLE_TIMEOUT } from '../idletransaction'; +import { DEFAULT_FINAL_TIMEOUT, DEFAULT_HEARTBEAT_INTERVAL, DEFAULT_IDLE_TIMEOUT } from '../idletransaction'; import { extractTraceparentData } from '../utils'; import { registerBackgroundTabDetection } from './backgroundtab'; import { addPerformanceEntries, startTrackingLongTasks, startTrackingWebVitals } from './metrics'; @@ -40,6 +40,14 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { */ finalTimeout: number; + /** + * The heartbeat interval. If activities don't change in 3 heartbeats, a transaction will be finished. + * Time is in ms. + * + * Default: 5000 + */ + heartbeatInterval: number; + /** * Flag to enable/disable creation of `navigation` transaction on history changes. * @@ -105,6 +113,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { const DEFAULT_BROWSER_TRACING_OPTIONS = { idleTimeout: DEFAULT_IDLE_TIMEOUT, finalTimeout: DEFAULT_FINAL_TIMEOUT, + heartbeatInterval: DEFAULT_HEARTBEAT_INTERVAL, markBackgroundTransactions: true, routingInstrumentation: instrumentRoutingWithDefaults, startTransactionOnLocationChange: true, @@ -213,7 +222,7 @@ export class BrowserTracing implements Integration { } // eslint-disable-next-line @typescript-eslint/unbound-method - const { beforeNavigate, idleTimeout, finalTimeout } = this.options; + const { beforeNavigate, idleTimeout, finalTimeout, heartbeatInterval } = this.options; const isPageloadTransaction = context.op === 'pageload'; @@ -262,6 +271,7 @@ export class BrowserTracing implements Integration { finalContext, idleTimeout, finalTimeout, + heartbeatInterval, true, { location }, // for use in the tracesSampler ); diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index dbb9254054e9..8b8add56b469 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -186,13 +186,14 @@ export function startIdleTransaction( transactionContext: TransactionContext, idleTimeout: number, finalTimeout: number, + heartbeatInterval: number, onScope?: boolean, customSamplingContext?: CustomSamplingContext, ): IdleTransaction { const client = hub.getClient(); const options: Partial = (client && client.getOptions()) || {}; - let transaction = new IdleTransaction(transactionContext, hub, idleTimeout, finalTimeout, onScope); + let transaction = new IdleTransaction(transactionContext, hub, idleTimeout, finalTimeout, heartbeatInterval, onScope); transaction = sample(transaction, options, { parentSampled: transactionContext.parentSampled, transactionContext, diff --git a/packages/tracing/src/idletransaction.ts b/packages/tracing/src/idletransaction.ts index b12ba4b34601..c19f187fd00a 100644 --- a/packages/tracing/src/idletransaction.ts +++ b/packages/tracing/src/idletransaction.ts @@ -8,7 +8,7 @@ import { Transaction } from './transaction'; export const DEFAULT_IDLE_TIMEOUT = 1000; export const DEFAULT_FINAL_TIMEOUT = 30000; -export const HEARTBEAT_INTERVAL = 5000; +export const DEFAULT_HEARTBEAT_INTERVAL = 5000; /** * @inheritDoc @@ -85,6 +85,7 @@ export class IdleTransaction extends Transaction { * The final value in ms that a transaction cannot exceed */ private readonly _finalTimeout: number = DEFAULT_FINAL_TIMEOUT, + private readonly _heartbeatInterval: number = DEFAULT_HEARTBEAT_INTERVAL, // 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, ) { @@ -287,7 +288,7 @@ export class IdleTransaction extends Transaction { __DEBUG_BUILD__ && logger.log(`pinging Heartbeat -> current counter: ${this._heartbeatCounter}`); setTimeout(() => { this._beat(); - }, HEARTBEAT_INTERVAL); + }, this._heartbeatInterval); } } diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 2fc24fa1d670..55f1ace22119 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -8,7 +8,12 @@ import { BrowserTracing, BrowserTracingOptions, getMetaContent } from '../../src import { defaultRequestInstrumentationOptions } from '../../src/browser/request'; import { instrumentRoutingWithDefaults } from '../../src/browser/router'; import * as hubExtensions from '../../src/hubextensions'; -import { DEFAULT_FINAL_TIMEOUT, DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../../src/idletransaction'; +import { + DEFAULT_FINAL_TIMEOUT, + DEFAULT_HEARTBEAT_INTERVAL, + DEFAULT_IDLE_TIMEOUT, + IdleTransaction, +} from '../../src/idletransaction'; import { getActiveTransaction } from '../../src/utils'; import { getDefaultBrowserClientOptions } from '../testutils'; @@ -83,6 +88,7 @@ describe('BrowserTracing', () => { }, idleTimeout: DEFAULT_IDLE_TIMEOUT, finalTimeout: DEFAULT_FINAL_TIMEOUT, + heartbeatInterval: DEFAULT_HEARTBEAT_INTERVAL, markBackgroundTransactions: true, routingInstrumentation: instrumentRoutingWithDefaults, startTransactionOnLocationChange: true, @@ -266,6 +272,7 @@ describe('BrowserTracing', () => { }), expect.any(Number), expect.any(Number), + expect.any(Number), expect.any(Boolean), expect.any(Object), ); diff --git a/packages/tracing/test/idletransaction.test.ts b/packages/tracing/test/idletransaction.test.ts index 956de96b5ea5..1e7af1d78c86 100644 --- a/packages/tracing/test/idletransaction.test.ts +++ b/packages/tracing/test/idletransaction.test.ts @@ -3,8 +3,8 @@ import { Hub } from '@sentry/hub'; import { DEFAULT_FINAL_TIMEOUT, + DEFAULT_HEARTBEAT_INTERVAL, DEFAULT_IDLE_TIMEOUT, - HEARTBEAT_INTERVAL, IdleTransaction, IdleTransactionSpanRecorder, } from '../src/idletransaction'; @@ -21,7 +21,14 @@ beforeEach(() => { describe('IdleTransaction', () => { describe('onScope', () => { it('sets the transaction on the scope on creation if onScope is true', () => { - const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT, DEFAULT_FINAL_TIMEOUT, true); + const transaction = new IdleTransaction( + { name: 'foo' }, + hub, + DEFAULT_IDLE_TIMEOUT, + DEFAULT_FINAL_TIMEOUT, + DEFAULT_HEARTBEAT_INTERVAL, + true, + ); transaction.initSpanRecorder(10); hub.configureScope(s => { @@ -39,7 +46,14 @@ describe('IdleTransaction', () => { }); it('removes sampled transaction from scope on finish if onScope is true', () => { - const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT, DEFAULT_FINAL_TIMEOUT, true); + const transaction = new IdleTransaction( + { name: 'foo' }, + hub, + DEFAULT_IDLE_TIMEOUT, + DEFAULT_FINAL_TIMEOUT, + DEFAULT_HEARTBEAT_INTERVAL, + true, + ); transaction.initSpanRecorder(10); transaction.finish(); @@ -56,6 +70,7 @@ describe('IdleTransaction', () => { hub, DEFAULT_IDLE_TIMEOUT, DEFAULT_FINAL_TIMEOUT, + DEFAULT_HEARTBEAT_INTERVAL, true, ); @@ -248,17 +263,17 @@ describe('IdleTransaction', () => { expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 1 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(transaction.status).not.toEqual('deadline_exceeded'); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(transaction.status).not.toEqual('deadline_exceeded'); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 3 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(transaction.status).not.toEqual('deadline_exceeded'); expect(mockFinish).toHaveBeenCalledTimes(0); }); @@ -272,15 +287,15 @@ describe('IdleTransaction', () => { transaction.startChild({}); // Beat 1 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 3 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(1); }); @@ -293,42 +308,42 @@ describe('IdleTransaction', () => { transaction.startChild({}); // Beat 1 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); const span = transaction.startChild(); // push activity // Beat 1 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); transaction.startChild(); // push activity transaction.startChild(); // push activity // Beat 1 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); span.finish(); // pop activity // Beat 1 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 2 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(0); // Beat 3 - jest.advanceTimersByTime(HEARTBEAT_INTERVAL); + jest.advanceTimersByTime(DEFAULT_HEARTBEAT_INTERVAL); expect(mockFinish).toHaveBeenCalledTimes(1); // Heartbeat does not keep going after finish has been called From a7b0cfc97b0395e3dd7acea6b5aede8c2bfff51a Mon Sep 17 00:00:00 2001 From: Outsider Date: Mon, 3 Oct 2022 23:18:01 +0900 Subject: [PATCH 2/7] feat(browser): make heartbeatInterval optional Signed-off-by: Outsider --- packages/tracing/src/hubextensions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 8b8add56b469..e19610224aca 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -186,7 +186,7 @@ export function startIdleTransaction( transactionContext: TransactionContext, idleTimeout: number, finalTimeout: number, - heartbeatInterval: number, + heartbeatInterval?: number, onScope?: boolean, customSamplingContext?: CustomSamplingContext, ): IdleTransaction { From 7294db131b67b8ce5d0c7e1b1c7d5ded08f0489a Mon Sep 17 00:00:00 2001 From: Outsider Date: Mon, 3 Oct 2022 23:18:50 +0900 Subject: [PATCH 3/7] feat(browser): test for heartbeatInterval Signed-off-by: Outsider --- .../tracing/test/browser/browsertracing.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 55f1ace22119..07d0ca230900 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -307,6 +307,23 @@ describe('BrowserTracing', () => { expect(mockFinish).toHaveBeenCalledTimes(1); }); }); + + describe('heartbeatInterval', () => { + it('can be a custom value', () => { + const interval = 200; + createBrowserTracing(true, { heartbeatInterval: interval, routingInstrumentation: customInstrumentRouting }); + const mockFinish = jest.fn(); + const transaction = getActiveTransaction(hub) as IdleTransaction; + transaction.finish = mockFinish; + + const span = transaction.startChild(); // activities = 1 + span.finish(); // activities = 0 + + expect(mockFinish).toHaveBeenCalledTimes(0); + jest.advanceTimersByTime(interval * 3); + expect(mockFinish).toHaveBeenCalledTimes(1); + }); + }); }); // Integration tests for the default routing instrumentation From 60ab0759f96ae754886b74920b42adc4c6df8071 Mon Sep 17 00:00:00 2001 From: Outsider Date: Tue, 4 Oct 2022 00:55:32 +0900 Subject: [PATCH 4/7] Update packages/tracing/src/browser/browsertracing.ts Co-authored-by: Lukas Stracke --- packages/tracing/src/browser/browsertracing.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index c336e67a5da0..130ffe132499 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -41,7 +41,8 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { finalTimeout: number; /** - * The heartbeat interval. If activities don't change in 3 heartbeats, a transaction will be finished. + * The heartbeat interval. If no new spans are started or open spans are finished within 3 heartbeats, + * the transaction will be finished. * Time is in ms. * * Default: 5000 From aa1b794c5ff48f62be1dc0dbe5b39b99ca212452 Mon Sep 17 00:00:00 2001 From: Outsider Date: Tue, 4 Oct 2022 20:13:23 +0900 Subject: [PATCH 5/7] feat(browser): fix lint errors Signed-off-by: Outsider --- packages/tracing/src/browser/browsertracing.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 130ffe132499..eec34d4b28db 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -41,8 +41,8 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { finalTimeout: number; /** - * The heartbeat interval. If no new spans are started or open spans are finished within 3 heartbeats, - * the transaction will be finished. + * The heartbeat interval. If no new spans are started or open spans are finished within 3 heartbeats, + * the transaction will be finished. * Time is in ms. * * Default: 5000 From 8f7ca316c1c9dd2430039104f65ba4c8ee1c46a5 Mon Sep 17 00:00:00 2001 From: Outsider Date: Thu, 6 Oct 2022 00:36:01 +0900 Subject: [PATCH 6/7] Update packages/tracing/src/hubextensions.ts Co-authored-by: Lukas Stracke --- packages/tracing/src/hubextensions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index e19610224aca..e3f953805697 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -186,9 +186,9 @@ export function startIdleTransaction( transactionContext: TransactionContext, idleTimeout: number, finalTimeout: number, - heartbeatInterval?: number, onScope?: boolean, customSamplingContext?: CustomSamplingContext, + heartbeatInterval?: number, ): IdleTransaction { const client = hub.getClient(); const options: Partial = (client && client.getOptions()) || {}; From fafb53398a1f471f734321766dd6178657de4ecd Mon Sep 17 00:00:00 2001 From: Outsider Date: Thu, 6 Oct 2022 00:40:37 +0900 Subject: [PATCH 7/7] fix tests as startIdleTransaction signature Signed-off-by: Outsider --- packages/tracing/src/browser/browsertracing.ts | 2 +- packages/tracing/test/browser/browsertracing.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index eec34d4b28db..f0924b543450 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -272,9 +272,9 @@ export class BrowserTracing implements Integration { finalContext, idleTimeout, finalTimeout, - heartbeatInterval, true, { location }, // for use in the tracesSampler + heartbeatInterval, ); idleTransaction.registerBeforeFinishCallback(transaction => { addPerformanceEntries(transaction); diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 07d0ca230900..325078d05acf 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -272,9 +272,9 @@ describe('BrowserTracing', () => { }), expect.any(Number), expect.any(Number), - expect.any(Number), expect.any(Boolean), expect.any(Object), + expect.any(Number), ); });