From 67380a90a8fb64214761b1654056f2b082d9527b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 8 Dec 2021 12:24:30 -0500 Subject: [PATCH 1/2] feat(tracing): Add metadata around idleTimeout - Add tag if user sets custom idleTimeout - Add tag if user sets reportAllChanges option - Add finish reason tag to idleTransactions (tracked in enum) --- packages/tracing/src/browser/backgroundtab.ts | 2 ++ .../tracing/src/browser/browsertracing.ts | 28 +++++++++++-------- packages/tracing/src/browser/metrics.ts | 21 ++++---------- packages/tracing/src/constants.ts | 5 ++++ packages/tracing/src/idletransaction.ts | 4 ++- .../test/browser/browsertracing.test.ts | 12 ++++---- 6 files changed, 39 insertions(+), 33 deletions(-) create mode 100644 packages/tracing/src/constants.ts diff --git a/packages/tracing/src/browser/backgroundtab.ts b/packages/tracing/src/browser/backgroundtab.ts index b4fe490ab1b8..d2c3d55eeb86 100644 --- a/packages/tracing/src/browser/backgroundtab.ts +++ b/packages/tracing/src/browser/backgroundtab.ts @@ -1,5 +1,6 @@ import { getGlobalObject, logger } from '@sentry/utils'; +import { FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS } from '../constants'; import { IdleTransaction } from '../idletransaction'; import { SpanStatus } from '../spanstatus'; import { getActiveTransaction } from '../utils'; @@ -24,6 +25,7 @@ export function registerBackgroundTabDetection(): void { activeTransaction.setStatus(SpanStatus.Cancelled); } activeTransaction.setTag('visibilitychange', 'document.hidden'); + activeTransaction.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[2]); activeTransaction.finish(); } }); diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 31aa00fb735d..5fda84855208 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -7,7 +7,7 @@ import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction'; import { SpanStatus } from '../spanstatus'; import { extractTraceparentData, secToMs } from '../utils'; import { registerBackgroundTabDetection } from './backgroundtab'; -import { DEFAULT_METRICS_INSTR_OPTIONS, MetricsInstrumentation, MetricsInstrumentationOptions } from './metrics'; +import { MetricsInstrumentation } from './metrics'; import { defaultRequestInstrumentationOptions, instrumentOutgoingRequests, @@ -67,7 +67,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { * * Default: undefined */ - _metricOptions?: Partial; + _metricOptions?: Partial<{ _reportAllChanges: boolean }>; /** * beforeNavigate is called before a pageload/navigation transaction is created and allows users to modify transaction @@ -129,18 +129,19 @@ export class BrowserTracing implements Integration { private readonly _emitOptionsWarning: boolean = false; + /** Store configured idle timeout so that it can be added as a tag to transactions */ + private _configuredIdleTimeout: BrowserTracingOptions['idleTimeout'] | undefined = undefined; + public constructor(_options?: Partial) { let tracingOrigins = defaultRequestInstrumentationOptions.tracingOrigins; // NOTE: Logger doesn't work in constructors, as it's initialized after integrations instances - if ( - _options && - _options.tracingOrigins && - Array.isArray(_options.tracingOrigins) && - _options.tracingOrigins.length !== 0 - ) { - tracingOrigins = _options.tracingOrigins; - } else { - this._emitOptionsWarning = true; + if (_options) { + this._configuredIdleTimeout = _options.idleTimeout; + if (_options.tracingOrigins && Array.isArray(_options.tracingOrigins) && _options.tracingOrigins.length !== 0) { + tracingOrigins = _options.tracingOrigins; + } else { + this._emitOptionsWarning = true; + } } this.options = { @@ -149,7 +150,8 @@ export class BrowserTracing implements Integration { tracingOrigins, }; - this._metrics = new MetricsInstrumentation({ ...DEFAULT_METRICS_INSTR_OPTIONS, ...this.options._metricOptions }); + const { _metricOptions } = this.options; + this._metrics = new MetricsInstrumentation(_metricOptions && _metricOptions._reportAllChanges); } /** @@ -236,6 +238,8 @@ export class BrowserTracing implements Integration { adjustTransactionDuration(secToMs(maxTransactionDuration), transaction, endTimestamp); }); + idleTransaction.setTag('idleTimeout', this._configuredIdleTimeout); + return idleTransaction as Transaction; } } diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 4d1c9d244bce..b38194646217 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -14,17 +14,6 @@ import { NavigatorDeviceMemory, NavigatorNetworkInformation } from './web-vitals const global = getGlobalObject(); -/** - * Exports a way to add options to our metric collection. Currently experimental. - */ -export interface MetricsInstrumentationOptions { - _reportAllChanges: boolean; -} - -export const DEFAULT_METRICS_INSTR_OPTIONS: MetricsInstrumentationOptions = { - _reportAllChanges: false, -}; - /** Class tracking metrics */ export class MetricsInstrumentation { private _measurements: Measurements = {}; @@ -33,14 +22,14 @@ export class MetricsInstrumentation { private _lcpEntry: LargestContentfulPaint | undefined; private _clsEntry: LayoutShift | undefined; - public constructor(_options: MetricsInstrumentationOptions) { + public constructor(private _reportAllChanges: boolean = false) { if (!isNodeEnv() && global?.performance && global?.document) { if (global.performance.mark) { global.performance.mark('sentry-tracing-init'); } this._trackCLS(); - this._trackLCP(_options._reportAllChanges); + this._trackLCP(); this._trackFID(); } } @@ -206,6 +195,8 @@ export class MetricsInstrumentation { transaction.setMeasurements(this._measurements); this._tagMetricInfo(transaction); + + transaction.setTag('sentry_reportAllChanges', this._reportAllChanges); } } @@ -296,7 +287,7 @@ export class MetricsInstrumentation { } /** Starts tracking the Largest Contentful Paint on the current page. */ - private _trackLCP(reportAllChanges: boolean): void { + private _trackLCP(): void { getLCP(metric => { const entry = metric.entries.pop(); @@ -310,7 +301,7 @@ export class MetricsInstrumentation { this._measurements['lcp'] = { value: metric.value }; this._measurements['mark.lcp'] = { value: timeOrigin + startTime }; this._lcpEntry = entry as LargestContentfulPaint; - }, reportAllChanges); + }, this._reportAllChanges); } /** Starts tracking the First Input Delay on the current page. */ diff --git a/packages/tracing/src/constants.ts b/packages/tracing/src/constants.ts new file mode 100644 index 000000000000..3f01982c3614 --- /dev/null +++ b/packages/tracing/src/constants.ts @@ -0,0 +1,5 @@ +// 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 IDLE_TRANSACTION_FINISH_REASONS = ['heartbeatFailed', 'idleTimeout', 'documentHidden'] as const; diff --git a/packages/tracing/src/idletransaction.ts b/packages/tracing/src/idletransaction.ts index 81319bcc91e0..c14e079b50c2 100644 --- a/packages/tracing/src/idletransaction.ts +++ b/packages/tracing/src/idletransaction.ts @@ -2,6 +2,7 @@ import { Hub } from '@sentry/hub'; import { TransactionContext } from '@sentry/types'; import { logger, timestampWithMs } from '@sentry/utils'; +import { FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS } from './constants'; import { Span, SpanRecorder } from './span'; import { SpanStatus } from './spanstatus'; import { Transaction } from './transaction'; @@ -223,6 +224,7 @@ export class IdleTransaction extends Transaction { setTimeout(() => { if (!this._finished) { + this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[1]); this.finish(end); } }, timeout); @@ -252,7 +254,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(SpanStatus.DeadlineExceeded); - this.setTag('heartbeat', 'failed'); + this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[0]); this.finish(); } else { this._pingHeartbeat(); diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index bc94f7ed73bb..567c6edcd95b 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -11,7 +11,7 @@ import { getHeaderContext, getMetaContent, } from '../../src/browser/browsertracing'; -import { DEFAULT_METRICS_INSTR_OPTIONS, MetricsInstrumentation } from '../../src/browser/metrics'; +import { MetricsInstrumentation } from '../../src/browser/metrics'; import { defaultRequestInstrumentationOptions } from '../../src/browser/request'; import { instrumentRoutingWithDefaults } from '../../src/browser/router'; import * as hubExtensions from '../../src/hubextensions'; @@ -246,6 +246,8 @@ describe('BrowserTracing', () => { expect(mockFinish).toHaveBeenCalledTimes(0); jest.advanceTimersByTime(DEFAULT_IDLE_TIMEOUT); expect(mockFinish).toHaveBeenCalledTimes(1); + + expect(transaction.tags).toEqual({ idleTimeout: undefined }); }); it('can be a custom value', () => { @@ -260,6 +262,8 @@ describe('BrowserTracing', () => { expect(mockFinish).toHaveBeenCalledTimes(0); jest.advanceTimersByTime(2000); expect(mockFinish).toHaveBeenCalledTimes(1); + + expect(transaction.tags).toEqual({ idleTimeout: 2000 }); }); }); @@ -507,7 +511,7 @@ describe('BrowserTracing', () => { createBrowserTracing(true, {}); expect(MetricsInstrumentation).toHaveBeenCalledTimes(1); - expect(MetricsInstrumentation).toHaveBeenLastCalledWith(DEFAULT_METRICS_INSTR_OPTIONS); + expect(MetricsInstrumentation).toHaveBeenLastCalledWith(undefined); }); it('creates metrics instrumentation with custom options', () => { @@ -518,9 +522,7 @@ describe('BrowserTracing', () => { }); expect(MetricsInstrumentation).toHaveBeenCalledTimes(1); - expect(MetricsInstrumentation).toHaveBeenLastCalledWith({ - _reportAllChanges: true, - }); + expect(MetricsInstrumentation).toHaveBeenLastCalledWith(true); }); }); }); From 64700fc04debf67ace3739a258cd9f1da7cc4348 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 8 Dec 2021 15:06:07 -0500 Subject: [PATCH 2/2] add finish reason for tests --- packages/tracing/test/browser/browsertracing.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 567c6edcd95b..e8a123eeb93c 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -247,7 +247,7 @@ describe('BrowserTracing', () => { jest.advanceTimersByTime(DEFAULT_IDLE_TIMEOUT); expect(mockFinish).toHaveBeenCalledTimes(1); - expect(transaction.tags).toEqual({ idleTimeout: undefined }); + expect(transaction.tags).toEqual({ finishReason: 'idleTimeout', idleTimeout: undefined }); }); it('can be a custom value', () => { @@ -263,7 +263,7 @@ describe('BrowserTracing', () => { jest.advanceTimersByTime(2000); expect(mockFinish).toHaveBeenCalledTimes(1); - expect(transaction.tags).toEqual({ idleTimeout: 2000 }); + expect(transaction.tags).toEqual({ finishReason: 'idleTimeout', idleTimeout: 2000 }); }); });