Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(measurements): Update setMeasurements to only set a single measurement #4920

Merged
merged 7 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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();
Original file line number Diff line number Diff line change
@@ -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<Event>(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('');
});
43 changes: 28 additions & 15 deletions packages/tracing/src/browser/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait the units are supposed to be send in full form? We can't use ms or s?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jan-auer informed me that fully written out and singular is the way to go

@jan-auer can you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the source of truth: getsentry/relay#1217

So we should use the fully written out version.

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;
Expand Down Expand Up @@ -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',
};
}
}

Expand Down Expand Up @@ -162,7 +168,14 @@ export class MetricsInstrumentation {
delete this._measurements.cls;
}

transaction.setMeasurements(this._measurements);
Object.keys(this._measurements).forEach(measurementName => {
transaction.setMeasurement(
measurementName,
this._measurements[measurementName].value,
this._measurements[measurementName].unit,
);
});

tagMetricInfo(transaction, this._lcpEntry, this._clsEntry);
transaction.setTag('sentry_reportAllChanges', this._reportAllChanges);
}
Expand All @@ -189,11 +202,11 @@ export class MetricsInstrumentation {
}

if (isMeasurementValue(connection.rtt)) {
this._measurements['connection.rtt'] = { value: connection.rtt as number };
this._measurements['connection.rtt'] = { value: connection.rtt, unit: 'millisecond' };
}

if (isMeasurementValue(connection.downlink)) {
this._measurements['connection.downlink'] = { value: connection.downlink as number };
this._measurements['connection.downlink'] = { value: connection.downlink, unit: '' }; // unit is empty string for now, while relay doesn't support download speed units
}
}

Expand All @@ -218,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;
});
}
Expand All @@ -234,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);
}
Expand All @@ -251,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' };
});
}
}
Expand Down Expand Up @@ -392,7 +405,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);
}

Expand Down
10 changes: 7 additions & 3 deletions packages/tracing/src/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,15 @@ 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 an empty string)
* @hidden
*/
public setMeasurements(measurements: Measurements): void {
this._measurements = { ...measurements };
public setMeasurement(name: string, value: number, unit: string = ''): void {
this._measurements[name] = { value, unit };
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/types/src/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export interface SamplingContext extends CustomSamplingContext {
request?: ExtractedNodeRequestData;
}

export type Measurements = Record<string, { value: number }>;
export type Measurements = Record<string, { value: number; unit: string }>;

export type TransactionSamplingMethod = 'explicitly_set' | 'client_sampler' | 'client_rate' | 'inheritance';

Expand Down