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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Apr 13, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
605fe42
ref(measurements): Update `setMeasurements` to only set a single meas…
lforst Apr 12, 2022
dd9f8c8
Remove restrictions on `unit`
lforst Apr 12, 2022
e617f7f
Add function to include multiple measurements at once
lforst Apr 12, 2022
fad13a1
Add integration tests for transaction measurements
lforst Apr 12, 2022
14c62ff
Remove `setMeasurements` method
lforst Apr 12, 2022
afcdaee
Add units to web vitals measurements
lforst Apr 12, 2022
3a95f2c
Update download speed unit
lforst Apr 13, 2022
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
15 changes: 9 additions & 6 deletions packages/tracing/src/browser/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -17,7 +17,7 @@ const global = getGlobalObject<Window>();

/** Class tracking metrics */
export class MetricsInstrumentation {
lforst marked this conversation as resolved.
Show resolved Hide resolved
private _measurements: Measurements = {};
private _measurements: Record<string, { value: number }> = {};
Copy link
Member

Choose a reason for hiding this comment

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

do we need to add units to any of these? Most vitals like LCP are in seconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added units here: afcdaee


private _performanceCursor: number = 0;
private _lcpEntry: LargestContentfulPaint | undefined;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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 };
}
}

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

Expand Down
10 changes: 7 additions & 3 deletions packages/tracing/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { getCurrentHub, Hub } from '@sentry/hub';
import {
Event,
Measurements,
MeasurementUnit,
Transaction as TransactionInterface,
TransactionContext,
TransactionMetadata,
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

The default should be an empty string, ms is pretty unexpected behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I know units are basically free form text, I agree. Changed in dd9f8c8.

this._measurements[name] = { value, unit };
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/types/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export { Stacktrace } from './stacktrace';
export {
CustomSamplingContext,
Measurements,
MeasurementUnit,
SamplingContext,
TraceparentData,
Transaction,
Expand Down
3 changes: 2 additions & 1 deletion packages/types/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ export interface SamplingContext extends CustomSamplingContext {
request?: ExtractedNodeRequestData;
}

export type Measurements = Record<string, { value: number }>;
export type MeasurementUnit = 'ns' | 'ms' | 's';
lforst marked this conversation as resolved.
Show resolved Hide resolved
export type Measurements = Record<string, { value: number; unit: MeasurementUnit }>;

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

Expand Down