feat: Implement timed events & remove transaction.measurements

mydea · mydea · commit cda28234ca14 · 2024-04-04T09:17:03.000+02:00
Now, we interpret timed events with special attributes as timed events.
diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/setMeasurement/subject.js b/dev-packages/browser-integration-tests/suites/public-api/startSpan/setMeasurement/subject.js
@@ -1,11 +1,6 @@
-const transaction = Sentry.startInactiveSpan({
-  name: 'some_transaction',
-  forceTransaction: true,
+Sentry.startSpan({ name: 'some_transaction' }, () => {
+  Sentry.setMeasurement('metric.foo', 42, 'ms');
+  Sentry.setMeasurement('metric.bar', 1337, 'nanoseconds');
+  Sentry.setMeasurement('metric.baz', 99, 's');
+  Sentry.setMeasurement('metric.baz', 1, '');
 });
-
-transaction.setMeasurement('metric.foo', 42, 'ms');
-transaction.setMeasurement('metric.bar', 1337, 'nanoseconds');
-transaction.setMeasurement('metric.baz', 99, 's');
-transaction.setMeasurement('metric.baz', 1);
-
-transaction.end();
diff --git a/dev-packages/node-integration-tests/suites/public-api/setMeasurement/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/setMeasurement/scenario.ts
@@ -0,0 +1,16 @@
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  tracesSampleRate: 1,
+  transport: loggingTransport,
+});
+
+Sentry.startSpan({ name: 'some_transaction' }, () => {
+  Sentry.setMeasurement('metric.foo', 42, 'ms');
+  Sentry.setMeasurement('metric.bar', 1337, 'nanoseconds');
+  Sentry.setMeasurement('metric.baz', 99, 's');
+  Sentry.setMeasurement('metric.baz', 1, '');
+});
diff --git a/dev-packages/node-integration-tests/suites/public-api/setMeasurement/test.ts b/dev-packages/node-integration-tests/suites/public-api/setMeasurement/test.ts
@@ -0,0 +1,20 @@
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('should attach measurement to transaction', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .expect({
+      transaction: {
+        transaction: 'some_transaction',
+        measurements: {
+          'metric.foo': { value: 42, unit: 'ms' },
+          'metric.bar': { value: 1337, unit: 'nanoseconds' },
+          'metric.baz': { value: 1, unit: '' },
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts
@@ -22,3 +22,9 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN = 'sentry.origin';
 
 /** The reason why an idle span finished. */
 export const SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON = 'sentry.idle_span_finish_reason';
+
+/** The unit of a measurement, which may be stored as a TimedEvent. */
+export const SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT = 'sentry.measurement_unit';
+
+/** The value of a measurement, which may be stored as a TimedEvent. */
+export const SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE = 'sentry.measurement_value';
diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts
@@ -16,6 +16,6 @@ export {
   withActiveSpan,
 } from './trace';
 export { getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
-export { setMeasurement } from './measurement';
+export { setMeasurement, timedEventsToMeasurements } from './measurement';
 export { sampleSpan } from './sampling';
 export { logSpanEnd, logSpanStart } from './logSpans';
diff --git a/packages/core/src/tracing/measurement.ts b/packages/core/src/tracing/measurement.ts
@@ -1,4 +1,8 @@
-import type { MeasurementUnit, Span, Transaction } from '@sentry/types';
+import type { MeasurementUnit, Measurements, TimedEvent } from '@sentry/types';
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT,
+  SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE,
+} from '../semanticAttributes';
 import { getActiveSpan, getRootSpan } from '../utils/spanUtils';
 
 /**
@@ -8,13 +12,28 @@ export function setMeasurement(name: string, value: number, unit: MeasurementUni
   const activeSpan = getActiveSpan();
   const rootSpan = activeSpan && getRootSpan(activeSpan);
 
-  if (rootSpan && rootSpanIsTransaction(rootSpan)) {
-    // eslint-disable-next-line deprecation/deprecation
-    rootSpan.setMeasurement(name, value, unit);
+  if (rootSpan) {
+    rootSpan.addEvent(name, {
+      [SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE]: value,
+      [SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT]: unit as string,
+    });
   }
 }
 
-function rootSpanIsTransaction(rootSpan: Span): rootSpan is Transaction {
-  // eslint-disable-next-line deprecation/deprecation
-  return typeof (rootSpan as Transaction).setMeasurement === 'function';
+/**
+ * Convert timed events to measurements.
+ */
+export function timedEventsToMeasurements(events: TimedEvent[]): Measurements {
+  const measurements: Measurements = {};
+  events.forEach(event => {
+    const attributes = event.attributes || {};
+    const unit = attributes[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT] as MeasurementUnit | undefined;
+    const value = attributes[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE] as number | undefined;
+
+    if (typeof unit === 'string' && typeof value === 'number') {
+      measurements[event.name] = { value, unit };
+    }
+  });
+
+  return measurements;
 }
diff --git a/packages/core/src/tracing/sentryNonRecordingSpan.ts b/packages/core/src/tracing/sentryNonRecordingSpan.ts
@@ -59,4 +59,13 @@ export class SentryNonRecordingSpan implements Span {
   public isRecording(): boolean {
     return false;
   }
+
+  /** @inheritdoc */
+  public addEvent(
+    _name: string,
+    _attributesOrStartTime?: SpanAttributes | SpanTimeInput,
+    _startTime?: SpanTimeInput,
+  ): this {
+    return this;
+  }
 }
diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts
@@ -8,6 +8,7 @@ import type {
   SpanOrigin,
   SpanStatus,
   SpanTimeInput,
+  TimedEvent,
 } from '@sentry/types';
 import { dropUndefinedKeys, timestampInSeconds, uuid4 } from '@sentry/utils';
 import { getClient } from '../currentScopes';
@@ -33,6 +34,8 @@ export class SentrySpan implements Span {
   protected _endTime?: number | undefined;
   /** Internal keeper of the status */
   protected _status?: SpanStatus;
+  /** The timed events added to this span. */
+  protected _events: TimedEvent[];
 
   /**
    * You should never call the constructor manually, always use `Sentry.startSpan()`
@@ -65,6 +68,8 @@ export class SentrySpan implements Span {
     if (spanContext.endTimestamp) {
       this._endTime = spanContext.endTimestamp;
     }
+
+    this._events = [];
   }
 
   /** @inheritdoc */
@@ -162,6 +167,32 @@ export class SentrySpan implements Span {
     return !this._endTime && !!this._sampled;
   }
 
+  /**
+   * @inheritdoc
+   */
+  public addEvent(
+    name: string,
+    attributesOrStartTime?: SpanAttributes | SpanTimeInput,
+    startTime?: SpanTimeInput,
+  ): this {
+    if (this._endTime) {
+      return this;
+    }
+
+    const time = isSpanTimeInput(attributesOrStartTime) ? attributesOrStartTime : startTime || timestampInSeconds();
+    const attributes = isSpanTimeInput(attributesOrStartTime) ? {} : attributesOrStartTime || {};
+
+    const event: TimedEvent = {
+      name,
+      time: spanTimeInputToSeconds(time),
+      attributes,
+    };
+
+    this._events.push(event);
+
+    return this;
+  }
+
   /** Emit `spanEnd` when the span is ended. */
   private _onSpanEnded(): void {
     const client = getClient();
@@ -170,3 +201,7 @@ export class SentrySpan implements Span {
     }
   }
 }
+
+function isSpanTimeInput(value: undefined | SpanAttributes | SpanTimeInput): value is SpanTimeInput {
+  return (value && typeof value === 'number') || value instanceof Date || Array.isArray(value);
+}
diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts
@@ -1,8 +1,6 @@
 import type {
   Contexts,
   Hub,
-  MeasurementUnit,
-  Measurements,
   SpanJSON,
   SpanTimeInput,
   Transaction as TransactionInterface,
@@ -18,6 +16,7 @@ import { getMetricSummaryJsonForSpan } from '../metrics/metric-summary';
 import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes';
 import { getSpanDescendants, spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils';
 import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
+import { timedEventsToMeasurements } from './measurement';
 import { SentrySpan } from './sentrySpan';
 import { getCapturedScopesOnSpan } from './utils';
 
@@ -30,8 +29,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
 
   protected _name: string;
 
-  private _measurements: Measurements;
-
   private _contexts: Contexts;
 
   private _trimEnd?: boolean | undefined;
@@ -46,7 +43,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
    */
   public constructor(transactionContext: TransactionArguments, hub?: Hub) {
     super(transactionContext);
-    this._measurements = {};
     this._contexts = {};
 
     // eslint-disable-next-line deprecation/deprecation
@@ -69,15 +65,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
     return this;
   }
 
-  /**
-   * @inheritDoc
-   *
-   * @deprecated Use top-level `setMeasurement()` instead.
-   */
-  public setMeasurement(name: string, value: number, unit: MeasurementUnit = ''): void {
-    this._measurements[name] = { value, unit };
-  }
-
   /**
    * @inheritDoc
    */
@@ -169,15 +156,13 @@ export class Transaction extends SentrySpan implements TransactionInterface {
       }),
     };
 
-    const hasMeasurements = Object.keys(this._measurements).length > 0;
+    const measurements = timedEventsToMeasurements(this._events);
+    const hasMeasurements = Object.keys(measurements).length;
 
     if (hasMeasurements) {
       DEBUG_BUILD &&
-        logger.log(
-          '[Measurements] Adding measurements to transaction',
-          JSON.stringify(this._measurements, undefined, 2),
-        );
-      transaction.measurements = this._measurements;
+        logger.log('[Measurements] Adding measurements to transaction', JSON.stringify(measurements, undefined, 2));
+      transaction.measurements = measurements;
     }
 
     return transaction;
diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts
@@ -2,7 +2,7 @@ import type { Span } from '@opentelemetry/api';
 import { SpanKind } from '@opentelemetry/api';
 import type { ReadableSpan } from '@opentelemetry/sdk-trace-base';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
-import { captureEvent, getMetricSummaryJsonForSpan } from '@sentry/core';
+import { captureEvent, getMetricSummaryJsonForSpan, timedEventsToMeasurements } from '@sentry/core';
 import {
   SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
@@ -138,7 +138,10 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] {
 
     transactionEvent.spans = spans;
 
-    // TODO Measurements are not yet implemented in OTEL
+    const measurements = timedEventsToMeasurements(span.events);
+    if (Object.keys(measurements).length) {
+      transactionEvent.measurements = measurements;
+    }
 
     captureEvent(transactionEvent);
   });
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
@@ -101,6 +101,7 @@ export type {
   MetricSummary,
 } from './span';
 export type { SpanStatus } from './spanStatus';
+export type { TimedEvent } from './timedEvent';
 export type { StackFrame } from './stackframe';
 export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace';
 export type { PropagationContext, TracePropagationTargets } from './tracing';
diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts
@@ -227,4 +227,9 @@ export interface Span {
    * This will return false if tracing is disabled, this span was not sampled or if the span is already finished.
    */
   isRecording(): boolean;
+
+  /**
+   * Adds an event to the Span.
+   */
+  addEvent(name: string, attributesOrStartTime?: SpanAttributes | SpanTimeInput, startTime?: SpanTimeInput): this;
 }
diff --git a/packages/types/src/timedEvent.ts b/packages/types/src/timedEvent.ts
@@ -0,0 +1,7 @@
+import type { SpanAttributes, SpanTimeInput } from './span';
+
+export interface TimedEvent {
+  name: string;
+  time: SpanTimeInput;
+  attributes?: SpanAttributes;
+}
diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts
@@ -1,4 +1,3 @@
-import type { MeasurementUnit } from './measurement';
 import type { ExtractedNodeRequestData, WorkerLocation } from './misc';
 import type { SentrySpanArguments, Span } from './span';
 
@@ -48,19 +47,7 @@ export interface TraceparentData {
 /**
  * Transaction "Class", inherits Span only has `setName`
  */
-export interface Transaction extends Omit<TransactionArguments, 'name' | 'op' | 'spanId' | 'traceId'>, Span {
-  /**
-
-   * 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)
-   *
-   * @deprecated Use top-level `setMeasurement()` instead.
-   */
-  setMeasurement(name: string, value: number, unit: MeasurementUnit): void;
-}
+export interface Transaction extends Omit<TransactionArguments, 'name' | 'op' | 'spanId' | 'traceId'>, Span {}
 
 /**
  * Context data passed by the user when starting a transaction, to be used by the tracesSampler method.

Original file line number	Diff line number	Diff line change
`@@ -59,4 +59,13 @@ export class SentryNonRecordingSpan implements Span {`
`59`	`59`	`public isRecording(): boolean {`
`60`	`60`	`return false;`
`61`	`61`	`}`
	`62`	`+`
	`63`	`+ /** @inheritdoc */`
	`64`	`+ public addEvent(`
	`65`	`+ _name: string,`
	`66`	`+ _attributesOrStartTime?: SpanAttributes \| SpanTimeInput,`
	`67`	`+ _startTime?: SpanTimeInput,`
	`68`	`+ ): this {`
	`69`	`+ return this;`
	`70`	`+ }`
`62`	`71`	`}`