From 605fe422aa7e070aff379f0e379c24a2e85fb14e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 12 Apr 2022 11:55:47 +0000 Subject: [PATCH 1/7] ref(measurements): Update `setMeasurements` to only set a single measurement --- packages/tracing/src/browser/metrics.ts | 15 +++++++++------ packages/tracing/src/transaction.ts | 10 +++++++--- packages/types/src/index.ts | 1 + packages/types/src/transaction.ts | 3 ++- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 8386b608f247..9f691b8e340e 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -1,6 +1,6 @@ /* eslint-disable max-lines */ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Measurements, SpanContext } from '@sentry/types'; +import { SpanContext } from '@sentry/types'; import { browserPerformanceTimeOrigin, getGlobalObject, htmlTreeAsString, isNodeEnv, logger } from '@sentry/utils'; import { IS_DEBUG_BUILD } from '../flags'; @@ -17,7 +17,7 @@ const global = getGlobalObject(); /** Class tracking metrics */ export class MetricsInstrumentation { - private _measurements: Measurements = {}; + private _measurements: Record = {}; private _performanceCursor: number = 0; private _lcpEntry: LargestContentfulPaint | undefined; @@ -162,7 +162,10 @@ export class MetricsInstrumentation { delete this._measurements.cls; } - transaction.setMeasurements(this._measurements); + Object.keys(this._measurements).forEach(measurementName => { + transaction.setMeasurement(measurementName, this._measurements[measurementName].value); + }); + tagMetricInfo(transaction, this._lcpEntry, this._clsEntry); transaction.setTag('sentry_reportAllChanges', this._reportAllChanges); } @@ -189,11 +192,11 @@ export class MetricsInstrumentation { } if (isMeasurementValue(connection.rtt)) { - this._measurements['connection.rtt'] = { value: connection.rtt as number }; + this._measurements['connection.rtt'] = { value: connection.rtt }; } if (isMeasurementValue(connection.downlink)) { - this._measurements['connection.downlink'] = { value: connection.downlink as number }; + this._measurements['connection.downlink'] = { value: connection.downlink }; } } @@ -392,7 +395,7 @@ export function _startChild(transaction: Transaction, { startTimestamp, ...ctx } /** * Checks if a given value is a valid measurement value. */ -function isMeasurementValue(value: any): boolean { +function isMeasurementValue(value: unknown): value is number { return typeof value === 'number' && isFinite(value); } diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 6cbda0e04223..8f3ee0a9ca5e 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -2,6 +2,7 @@ import { getCurrentHub, Hub } from '@sentry/hub'; import { Event, Measurements, + MeasurementUnit, Transaction as TransactionInterface, TransactionContext, TransactionMetadata, @@ -68,11 +69,14 @@ export class Transaction extends SpanClass implements TransactionInterface { } /** - * Set observed measurements for this transaction. + * Set observed measurement for this transaction. + * @param name Name of the measurement + * @param value Value of the measurement + * @param unit Unit of the measurement. (Defaults to 'ms' - milliseconds) * @hidden */ - public setMeasurements(measurements: Measurements): void { - this._measurements = { ...measurements }; + public setMeasurement(name: string, value: number, unit: MeasurementUnit = 'ms'): void { + this._measurements[name] = { value, unit }; } /** diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 13651b7cd78e..3ea871e84763 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -54,6 +54,7 @@ export { Stacktrace } from './stacktrace'; export { CustomSamplingContext, Measurements, + MeasurementUnit, SamplingContext, TraceparentData, Transaction, diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index d2a8744cd335..fd67c6bcc53c 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -115,7 +115,8 @@ export interface SamplingContext extends CustomSamplingContext { request?: ExtractedNodeRequestData; } -export type Measurements = Record; +export type MeasurementUnit = 'ns' | 'ms' | 's'; +export type Measurements = Record; export type TransactionSamplingMethod = 'explicitly_set' | 'client_sampler' | 'client_rate' | 'inheritance'; From dd9f8c80a4a222a0a5750483821977ab52fcf099 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 12 Apr 2022 12:33:39 +0000 Subject: [PATCH 2/7] Remove restrictions on `unit` --- packages/tracing/src/transaction.ts | 5 ++--- packages/types/src/index.ts | 1 - packages/types/src/transaction.ts | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 8f3ee0a9ca5e..3a87a49f0d44 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -2,7 +2,6 @@ import { getCurrentHub, Hub } from '@sentry/hub'; import { Event, Measurements, - MeasurementUnit, Transaction as TransactionInterface, TransactionContext, TransactionMetadata, @@ -72,10 +71,10 @@ export class Transaction extends SpanClass implements TransactionInterface { * Set observed measurement for this transaction. * @param name Name of the measurement * @param value Value of the measurement - * @param unit Unit of the measurement. (Defaults to 'ms' - milliseconds) + * @param unit Unit of the measurement. (Defaults to an empty string) * @hidden */ - public setMeasurement(name: string, value: number, unit: MeasurementUnit = 'ms'): void { + public setMeasurement(name: string, value: number, unit: string = ''): void { this._measurements[name] = { value, unit }; } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 3ea871e84763..13651b7cd78e 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -54,7 +54,6 @@ export { Stacktrace } from './stacktrace'; export { CustomSamplingContext, Measurements, - MeasurementUnit, SamplingContext, TraceparentData, Transaction, diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index fd67c6bcc53c..0367eafbb941 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -115,8 +115,7 @@ export interface SamplingContext extends CustomSamplingContext { request?: ExtractedNodeRequestData; } -export type MeasurementUnit = 'ns' | 'ms' | 's'; -export type Measurements = Record; +export type Measurements = Record; export type TransactionSamplingMethod = 'explicitly_set' | 'client_sampler' | 'client_rate' | 'inheritance'; From e617f7ff703fbe882642357d35da0ac34138bf7e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 12 Apr 2022 13:48:47 +0000 Subject: [PATCH 3/7] Add function to include multiple measurements at once --- packages/tracing/src/transaction.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 3a87a49f0d44..8c7125e2185b 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -69,6 +69,7 @@ export class Transaction extends SpanClass implements TransactionInterface { /** * Set observed measurement for this transaction. + * * @param name Name of the measurement * @param value Value of the measurement * @param unit Unit of the measurement. (Defaults to an empty string) @@ -78,6 +79,17 @@ export class Transaction extends SpanClass implements TransactionInterface { this._measurements[name] = { value, unit }; } + /** + * Set observed measurments for this transaction. This is a convenience function + * instead of multiple `setMeasurement` calls. + * + * @param measurements Measurements to set. Keys represent measurement names. + * Existing measurements with matching names will be overwritten. + */ + public setMeasurements(measurements: Measurements): void { + this._measurements = { ...this._measurements, ...measurements }; + } + /** * Set metadata for this transaction. * @hidden From fad13a148c0ce4aeabb5f0e2870c9ec0a1f9382e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 12 Apr 2022 13:54:49 +0000 Subject: [PATCH 4/7] Add integration tests for transaction measurements --- .../startTransaction/setMeasurement/subject.js | 8 ++++++++ .../startTransaction/setMeasurement/test.ts | 18 ++++++++++++++++++ .../setMeasurements/subject.js | 14 ++++++++++++++ .../startTransaction/setMeasurements/test.ts | 18 ++++++++++++++++++ 4 files changed, 58 insertions(+) create mode 100644 packages/integration-tests/suites/public-api/startTransaction/setMeasurement/subject.js create mode 100644 packages/integration-tests/suites/public-api/startTransaction/setMeasurement/test.ts create mode 100644 packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js create mode 100644 packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts diff --git a/packages/integration-tests/suites/public-api/startTransaction/setMeasurement/subject.js b/packages/integration-tests/suites/public-api/startTransaction/setMeasurement/subject.js new file mode 100644 index 000000000000..036e86201b18 --- /dev/null +++ b/packages/integration-tests/suites/public-api/startTransaction/setMeasurement/subject.js @@ -0,0 +1,8 @@ +const transaction = Sentry.startTransaction({ name: 'some_transaction' }); + +transaction.setMeasurement('metric.foo', 42, 'ms'); +transaction.setMeasurement('metric.bar', 1337, 'nanoseconds'); +transaction.setMeasurement('metric.baz', 99, 's'); +transaction.setMeasurement('metric.baz', 1); + +transaction.finish(); diff --git a/packages/integration-tests/suites/public-api/startTransaction/setMeasurement/test.ts b/packages/integration-tests/suites/public-api/startTransaction/setMeasurement/test.ts new file mode 100644 index 000000000000..e91231093bf3 --- /dev/null +++ b/packages/integration-tests/suites/public-api/startTransaction/setMeasurement/test.ts @@ -0,0 +1,18 @@ +import { expect } from '@playwright/test'; +import { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should attach measurement to transaction', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + const event = await getFirstSentryEnvelopeRequest(page, url); + + expect(event.measurements?.['metric.foo'].value).toBe(42); + expect(event.measurements?.['metric.bar'].value).toBe(1337); + expect(event.measurements?.['metric.baz'].value).toBe(1); + + expect(event.measurements?.['metric.foo'].unit).toBe('ms'); + expect(event.measurements?.['metric.bar'].unit).toBe('nanoseconds'); + expect(event.measurements?.['metric.baz'].unit).toBe(''); +}); diff --git a/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js b/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js new file mode 100644 index 000000000000..191a7cf1e318 --- /dev/null +++ b/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js @@ -0,0 +1,14 @@ +const transaction = Sentry.startTransaction({ name: 'some_transaction' }); + +transaction.setMeasurements({ + 'metric.foo': { value: 42, unit: 'ms' }, + 'metric.bar': { value: 1337, unit: 'nanoseconds' }, + 'metric.baz': { value: 99, unit: 's' }, +}); + +transaction.setMeasurements({ + 'metric.bar': { value: 1337, unit: 'nanoseconds' }, + 'metric.baz': { value: 1, unit: '' }, +}); + +transaction.finish(); diff --git a/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts b/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts new file mode 100644 index 000000000000..9c1a7cc82df1 --- /dev/null +++ b/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts @@ -0,0 +1,18 @@ +import { expect } from '@playwright/test'; +import { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should attach measurements to transaction', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + const event = await getFirstSentryEnvelopeRequest(page, url); + + expect(event.measurements?.['metric.foo'].value).toBe(42); + expect(event.measurements?.['metric.bar'].value).toBe(1337); + expect(event.measurements?.['metric.baz'].value).toBe(1); + + expect(event.measurements?.['metric.foo'].unit).toBe('ms'); + expect(event.measurements?.['metric.bar'].unit).toBe('nanoseconds'); + expect(event.measurements?.['metric.baz'].unit).toBe(''); +}); From 14c62ff2d609a8541acac51c29a6d0f36b4aa909 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 12 Apr 2022 14:48:57 +0000 Subject: [PATCH 5/7] Remove `setMeasurements` method --- .../setMeasurements/subject.js | 14 -------------- .../startTransaction/setMeasurements/test.ts | 18 ------------------ packages/tracing/src/transaction.ts | 11 ----------- 3 files changed, 43 deletions(-) delete mode 100644 packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js delete mode 100644 packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts diff --git a/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js b/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js deleted file mode 100644 index 191a7cf1e318..000000000000 --- a/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/subject.js +++ /dev/null @@ -1,14 +0,0 @@ -const transaction = Sentry.startTransaction({ name: 'some_transaction' }); - -transaction.setMeasurements({ - 'metric.foo': { value: 42, unit: 'ms' }, - 'metric.bar': { value: 1337, unit: 'nanoseconds' }, - 'metric.baz': { value: 99, unit: 's' }, -}); - -transaction.setMeasurements({ - 'metric.bar': { value: 1337, unit: 'nanoseconds' }, - 'metric.baz': { value: 1, unit: '' }, -}); - -transaction.finish(); diff --git a/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts b/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts deleted file mode 100644 index 9c1a7cc82df1..000000000000 --- a/packages/integration-tests/suites/public-api/startTransaction/setMeasurements/test.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { expect } from '@playwright/test'; -import { Event } from '@sentry/types'; - -import { sentryTest } from '../../../../utils/fixtures'; -import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; - -sentryTest('should attach measurements to transaction', async ({ getLocalTestPath, page }) => { - const url = await getLocalTestPath({ testDir: __dirname }); - const event = await getFirstSentryEnvelopeRequest(page, url); - - expect(event.measurements?.['metric.foo'].value).toBe(42); - expect(event.measurements?.['metric.bar'].value).toBe(1337); - expect(event.measurements?.['metric.baz'].value).toBe(1); - - expect(event.measurements?.['metric.foo'].unit).toBe('ms'); - expect(event.measurements?.['metric.bar'].unit).toBe('nanoseconds'); - expect(event.measurements?.['metric.baz'].unit).toBe(''); -}); diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 8c7125e2185b..5384f15a6105 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -79,17 +79,6 @@ export class Transaction extends SpanClass implements TransactionInterface { this._measurements[name] = { value, unit }; } - /** - * Set observed measurments for this transaction. This is a convenience function - * instead of multiple `setMeasurement` calls. - * - * @param measurements Measurements to set. Keys represent measurement names. - * Existing measurements with matching names will be overwritten. - */ - public setMeasurements(measurements: Measurements): void { - this._measurements = { ...this._measurements, ...measurements }; - } - /** * Set metadata for this transaction. * @hidden From afcdaeeb3876c5030fea635814e13a13c7451a94 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 12 Apr 2022 15:51:47 +0000 Subject: [PATCH 6/7] Add units to web vitals measurements --- packages/tracing/src/browser/metrics.ts | 42 +++++++++++++++---------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 9f691b8e340e..55f53675aa19 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -1,6 +1,6 @@ /* eslint-disable max-lines */ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { SpanContext } from '@sentry/types'; +import { Measurements, SpanContext } from '@sentry/types'; import { browserPerformanceTimeOrigin, getGlobalObject, htmlTreeAsString, isNodeEnv, logger } from '@sentry/utils'; import { IS_DEBUG_BUILD } from '../flags'; @@ -17,7 +17,7 @@ const global = getGlobalObject(); /** Class tracking metrics */ export class MetricsInstrumentation { - private _measurements: Record = {}; + private _measurements: Measurements = {}; private _performanceCursor: number = 0; private _lcpEntry: LargestContentfulPaint | undefined; @@ -79,14 +79,14 @@ export class MetricsInstrumentation { if (entry.name === 'first-paint' && shouldRecord) { IS_DEBUG_BUILD && logger.log('[Measurements] Adding FP'); - this._measurements['fp'] = { value: entry.startTime }; - this._measurements['mark.fp'] = { value: startTimestamp }; + this._measurements['fp'] = { value: entry.startTime, unit: 'millisecond' }; + this._measurements['mark.fp'] = { value: startTimestamp, unit: 'second' }; } if (entry.name === 'first-contentful-paint' && shouldRecord) { IS_DEBUG_BUILD && logger.log('[Measurements] Adding FCP'); - this._measurements['fcp'] = { value: entry.startTime }; - this._measurements['mark.fcp'] = { value: startTimestamp }; + this._measurements['fcp'] = { value: entry.startTime, unit: 'millisecond' }; + this._measurements['mark.fcp'] = { value: startTimestamp, unit: 'second' }; } break; @@ -115,12 +115,18 @@ export class MetricsInstrumentation { // start of the response in milliseconds if (typeof responseStartTimestamp === 'number') { IS_DEBUG_BUILD && logger.log('[Measurements] Adding TTFB'); - this._measurements['ttfb'] = { value: (responseStartTimestamp - transaction.startTimestamp) * 1000 }; + this._measurements['ttfb'] = { + value: (responseStartTimestamp - transaction.startTimestamp) * 1000, + unit: 'millisecond', + }; if (typeof requestStartTimestamp === 'number' && requestStartTimestamp <= responseStartTimestamp) { // Capture the time spent making the request and receiving the first byte of the response. // This is the time between the start of the request and the start of the response in milliseconds. - this._measurements['ttfb.requestTime'] = { value: (responseStartTimestamp - requestStartTimestamp) * 1000 }; + this._measurements['ttfb.requestTime'] = { + value: (responseStartTimestamp - requestStartTimestamp) * 1000, + unit: 'second', + }; } } @@ -163,7 +169,11 @@ export class MetricsInstrumentation { } Object.keys(this._measurements).forEach(measurementName => { - transaction.setMeasurement(measurementName, this._measurements[measurementName].value); + transaction.setMeasurement( + measurementName, + this._measurements[measurementName].value, + this._measurements[measurementName].unit, + ); }); tagMetricInfo(transaction, this._lcpEntry, this._clsEntry); @@ -192,11 +202,11 @@ export class MetricsInstrumentation { } if (isMeasurementValue(connection.rtt)) { - this._measurements['connection.rtt'] = { value: connection.rtt }; + this._measurements['connection.rtt'] = { value: connection.rtt, unit: 'millisecond' }; } if (isMeasurementValue(connection.downlink)) { - this._measurements['connection.downlink'] = { value: connection.downlink }; + this._measurements['connection.downlink'] = { value: connection.downlink, unit: 'megabit' }; } } @@ -221,7 +231,7 @@ export class MetricsInstrumentation { } IS_DEBUG_BUILD && logger.log('[Measurements] Adding CLS'); - this._measurements['cls'] = { value: metric.value }; + this._measurements['cls'] = { value: metric.value, unit: 'millisecond' }; this._clsEntry = entry as LayoutShift; }); } @@ -237,8 +247,8 @@ export class MetricsInstrumentation { const timeOrigin = msToSec(browserPerformanceTimeOrigin as number); const startTime = msToSec(entry.startTime); IS_DEBUG_BUILD && logger.log('[Measurements] Adding LCP'); - this._measurements['lcp'] = { value: metric.value }; - this._measurements['mark.lcp'] = { value: timeOrigin + startTime }; + this._measurements['lcp'] = { value: metric.value, unit: 'millisecond' }; + this._measurements['mark.lcp'] = { value: timeOrigin + startTime, unit: 'second' }; this._lcpEntry = entry as LargestContentfulPaint; }, this._reportAllChanges); } @@ -254,8 +264,8 @@ export class MetricsInstrumentation { const timeOrigin = msToSec(browserPerformanceTimeOrigin as number); const startTime = msToSec(entry.startTime); IS_DEBUG_BUILD && logger.log('[Measurements] Adding FID'); - this._measurements['fid'] = { value: metric.value }; - this._measurements['mark.fid'] = { value: timeOrigin + startTime }; + this._measurements['fid'] = { value: metric.value, unit: 'millisecond' }; + this._measurements['mark.fid'] = { value: timeOrigin + startTime, unit: 'second' }; }); } } From 3a95f2c70686acfa026f731a0de4786ed82d082d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 13 Apr 2022 13:21:03 +0000 Subject: [PATCH 7/7] Update download speed unit --- packages/tracing/src/browser/metrics.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 55f53675aa19..dffc39abb088 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -206,7 +206,7 @@ export class MetricsInstrumentation { } if (isMeasurementValue(connection.downlink)) { - this._measurements['connection.downlink'] = { value: connection.downlink, unit: 'megabit' }; + this._measurements['connection.downlink'] = { value: connection.downlink, unit: '' }; // unit is empty string for now, while relay doesn't support download speed units } }