feat(core): Add attributes to Span (#10008)

mydea · anonrig · commit 7e9b3644e57b · 2024-01-03T17:41:26.000-05:00
Together with `setAttribute()` and `setAttributes()` APIs, mirroring the
OpenTelemetry API for their spans.

For now, these are stored as `data` on spans/transactions, until we
"directly" support them.
diff --git a/packages/core/src/tracing/span.ts b/packages/core/src/tracing/span.ts
@@ -3,6 +3,8 @@ import type {
   Instrumenter,
   Primitive,
   Span as SpanInterface,
+  SpanAttributeValue,
+  SpanAttributes,
   SpanContext,
   SpanOrigin,
   TraceContext,
@@ -104,6 +106,11 @@ export class Span implements SpanInterface {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   public data: { [key: string]: any };
 
+  /**
+   * @inheritDoc
+   */
+  public attributes: SpanAttributes;
+
   /**
    * List of spans that were finalized
    */
@@ -137,6 +144,7 @@ export class Span implements SpanInterface {
     this.startTimestamp = spanContext.startTimestamp || timestampInSeconds();
     this.tags = spanContext.tags || {};
     this.data = spanContext.data || {};
+    this.attributes = spanContext.attributes || {};
     this.instrumenter = spanContext.instrumenter || 'sentry';
     this.origin = spanContext.origin || 'manual';
 
@@ -217,12 +225,27 @@ export class Span implements SpanInterface {
   /**
    * @inheritDoc
    */
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
   public setData(key: string, value: any): this {
     this.data = { ...this.data, [key]: value };
     return this;
   }
 
+  /** @inheritdoc */
+  public setAttribute(key: string, value: SpanAttributeValue | undefined): void {
+    if (value === undefined) {
+      // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
+      delete this.attributes[key];
+    } else {
+      this.attributes[key] = value;
+    }
+  }
+
+  /** @inheritdoc */
+  public setAttributes(attributes: SpanAttributes): void {
+    Object.keys(attributes).forEach(key => this.setAttribute(key, attributes[key]));
+  }
+
   /**
    * @inheritDoc
    */
@@ -297,7 +320,7 @@ export class Span implements SpanInterface {
    */
   public toContext(): SpanContext {
     return dropUndefinedKeys({
-      data: this.data,
+      data: this._getData(),
       description: this.description,
       endTimestamp: this.endTimestamp,
       op: this.op,
@@ -335,7 +358,7 @@ export class Span implements SpanInterface {
    */
   public getTraceContext(): TraceContext {
     return dropUndefinedKeys({
-      data: Object.keys(this.data).length > 0 ? this.data : undefined,
+      data: this._getData(),
       description: this.description,
       op: this.op,
       parent_span_id: this.parentSpanId,
@@ -365,7 +388,7 @@ export class Span implements SpanInterface {
     origin?: SpanOrigin;
   } {
     return dropUndefinedKeys({
-      data: Object.keys(this.data).length > 0 ? this.data : undefined,
+      data: this._getData(),
       description: this.description,
       op: this.op,
       parent_span_id: this.parentSpanId,
@@ -378,6 +401,36 @@ export class Span implements SpanInterface {
       origin: this.origin,
     });
   }
+
+  /**
+   * Get the merged data for this span.
+   * For now, this combines `data` and `attributes` together,
+   * until eventually we can ingest `attributes` directly.
+   */
+  private _getData():
+    | {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        [key: string]: any;
+      }
+    | undefined {
+    const { data, attributes } = this;
+
+    const hasData = Object.keys(data).length > 0;
+    const hasAttributes = Object.keys(attributes).length > 0;
+
+    if (!hasData && !hasAttributes) {
+      return undefined;
+    }
+
+    if (hasData && hasAttributes) {
+      return {
+        ...data,
+        ...attributes,
+      };
+    }
+
+    return hasData ? data : attributes;
+  }
 }
 
 export type SpanStatusType =
diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts
@@ -1,12 +1,11 @@
-import type { TransactionContext } from '@sentry/types';
+import type { Span, TransactionContext } from '@sentry/types';
 import { dropUndefinedKeys, isThenable, logger, tracingContextFromHeaders } from '@sentry/utils';
 
 import { DEBUG_BUILD } from '../debug-build';
 import { getCurrentScope, withScope } from '../exports';
 import type { Hub } from '../hub';
 import { getCurrentHub } from '../hub';
 import { hasTracingEnabled } from '../utils/hasTracingEnabled';
-import type { Span } from './span';
 
 /**
  * Wraps a function with a transaction/span and finishes the span after the function is done.
diff --git a/packages/core/test/lib/tracing/span.test.ts b/packages/core/test/lib/tracing/span.test.ts
@@ -29,4 +29,162 @@ describe('span', () => {
     expect(span.name).toEqual('new name');
     expect(span.description).toEqual('new name');
   });
+
+  describe('setAttribute', () => {
+    it('allows to set attributes', () => {
+      const span = new Span();
+
+      span.setAttribute('str', 'bar');
+      span.setAttribute('num', 1);
+      span.setAttribute('zero', 0);
+      span.setAttribute('bool', true);
+      span.setAttribute('false', false);
+      span.setAttribute('undefined', undefined);
+      span.setAttribute('numArray', [1, 2]);
+      span.setAttribute('strArray', ['aa', 'bb']);
+      span.setAttribute('boolArray', [true, false]);
+      span.setAttribute('arrayWithUndefined', [1, undefined, 2]);
+
+      expect(span.attributes).toEqual({
+        str: 'bar',
+        num: 1,
+        zero: 0,
+        bool: true,
+        false: false,
+        numArray: [1, 2],
+        strArray: ['aa', 'bb'],
+        boolArray: [true, false],
+        arrayWithUndefined: [1, undefined, 2],
+      });
+    });
+
+    it('deletes attributes when setting to `undefined`', () => {
+      const span = new Span();
+
+      span.setAttribute('str', 'bar');
+
+      expect(Object.keys(span.attributes).length).toEqual(1);
+
+      span.setAttribute('str', undefined);
+
+      expect(Object.keys(span.attributes).length).toEqual(0);
+    });
+
+    it('disallows invalid attribute types', () => {
+      const span = new Span();
+
+      /** @ts-expect-error this is invalid */
+      span.setAttribute('str', {});
+
+      /** @ts-expect-error this is invalid */
+      span.setAttribute('str', null);
+
+      /** @ts-expect-error this is invalid */
+      span.setAttribute('str', [1, 'a']);
+    });
+  });
+
+  describe('setAttributes', () => {
+    it('allows to set attributes', () => {
+      const span = new Span();
+
+      const initialAttributes = span.attributes;
+
+      expect(initialAttributes).toEqual({});
+
+      const newAttributes = {
+        str: 'bar',
+        num: 1,
+        zero: 0,
+        bool: true,
+        false: false,
+        undefined: undefined,
+        numArray: [1, 2],
+        strArray: ['aa', 'bb'],
+        boolArray: [true, false],
+        arrayWithUndefined: [1, undefined, 2],
+      };
+      span.setAttributes(newAttributes);
+
+      expect(span.attributes).toEqual({
+        str: 'bar',
+        num: 1,
+        zero: 0,
+        bool: true,
+        false: false,
+        numArray: [1, 2],
+        strArray: ['aa', 'bb'],
+        boolArray: [true, false],
+        arrayWithUndefined: [1, undefined, 2],
+      });
+
+      expect(span.attributes).not.toBe(newAttributes);
+
+      span.setAttributes({
+        num: 2,
+        numArray: [3, 4],
+      });
+
+      expect(span.attributes).toEqual({
+        str: 'bar',
+        num: 2,
+        zero: 0,
+        bool: true,
+        false: false,
+        numArray: [3, 4],
+        strArray: ['aa', 'bb'],
+        boolArray: [true, false],
+        arrayWithUndefined: [1, undefined, 2],
+      });
+    });
+
+    it('deletes attributes when setting to `undefined`', () => {
+      const span = new Span();
+
+      span.setAttribute('str', 'bar');
+
+      expect(Object.keys(span.attributes).length).toEqual(1);
+
+      span.setAttributes({ str: undefined });
+
+      expect(Object.keys(span.attributes).length).toEqual(0);
+    });
+  });
+
+  // Ensure that attributes & data are merged together
+  describe('_getData', () => {
+    it('works without data & attributes', () => {
+      const span = new Span();
+
+      expect(span['_getData']()).toEqual(undefined);
+    });
+
+    it('works with data only', () => {
+      const span = new Span();
+      span.setData('foo', 'bar');
+
+      expect(span['_getData']()).toEqual({ foo: 'bar' });
+      expect(span['_getData']()).toBe(span.data);
+    });
+
+    it('works with attributes only', () => {
+      const span = new Span();
+      span.setAttribute('foo', 'bar');
+
+      expect(span['_getData']()).toEqual({ foo: 'bar' });
+      expect(span['_getData']()).toBe(span.attributes);
+    });
+
+    it('merges data & attributes', () => {
+      const span = new Span();
+      span.setAttribute('foo', 'foo');
+      span.setAttribute('bar', 'bar');
+      span.setData('foo', 'foo2');
+      span.setData('baz', 'baz');
+
+      expect(span['_getData']()).toEqual({ foo: 'foo', bar: 'bar', baz: 'baz' });
+      expect(span['_getData']()).not.toBe(span.attributes);
+      expect(span['_getData']()).not.toBe(span.data);
+    });
+  });
 });
diff --git a/packages/node/src/integrations/undici/types.ts b/packages/node/src/integrations/undici/types.ts
@@ -1,7 +1,7 @@
 // Vendored from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/5a94716c6788f654aea7999a5fc28f4f1e7c48ad/types/node/diagnostics_channel.d.ts
 
 import type { URL } from 'url';
-import type { Span } from '@sentry/core';
+import type { Span } from '@sentry/types';
 
 // License:
 // This project is licensed under the MIT license.
diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts
@@ -1,8 +1,8 @@
-/* eslint-disable @sentry-internal/sdk/no-optional-chaining */
-import type { Span } from '@sentry/core';
 import { getCurrentScope } from '@sentry/core';
 import { getActiveTransaction, runWithAsyncContext, startSpan } from '@sentry/core';
 import { captureException } from '@sentry/node';
+/* eslint-disable @sentry-internal/sdk/no-optional-chaining */
+import type { Span } from '@sentry/types';
 import { dynamicSamplingContextToSentryBaggageHeader, objectify } from '@sentry/utils';
 import type { Handle, ResolveOptions } from '@sveltejs/kit';
 
diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts
@@ -480,7 +480,6 @@ describe('Span', () => {
 
       expect(newContext).toStrictEqual({
         ...originalContext,
-        data: {},
         spanId: expect.any(String),
         startTimestamp: expect.any(Number),
         tags: {},
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
@@ -89,7 +89,7 @@ export type {
 
 // eslint-disable-next-line deprecation/deprecation
 export type { Severity, SeverityLevel } from './severity';
-export type { Span, SpanContext, SpanOrigin } from './span';
+export type { Span, SpanContext, SpanOrigin, SpanAttributeValue, SpanAttributes } from './span';
 export type { StackFrame } from './stackframe';
 export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace';
 export type { TextEncoderInternal } from './textencoder';
diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts
@@ -12,6 +12,17 @@ export type SpanOrigin =
   | `${SpanOriginType}.${SpanOriginCategory}.${SpanOriginIntegrationName}`
   | `${SpanOriginType}.${SpanOriginCategory}.${SpanOriginIntegrationName}.${SpanOriginIntegrationPart}`;
 
+// These types are aligned with OpenTelemetry Span Attributes
+export type SpanAttributeValue =
+  | string
+  | number
+  | boolean
+  | Array<null | undefined | string>
+  | Array<null | undefined | number>
+  | Array<null | undefined | boolean>;
+
+export type SpanAttributes = Record<string, SpanAttributeValue | undefined>;
+
 /** Interface holding all properties that can be set on a Span on creation. */
 export interface SpanContext {
   /**
@@ -65,6 +76,11 @@ export interface SpanContext {
    */
   data?: { [key: string]: any };
 
+  /**
+   * Attributes of the Span.
+   */
+  attributes?: SpanAttributes;
+
   /**
    * Timestamp in seconds (epoch time) indicating when the span started.
    */
@@ -118,6 +134,11 @@ export interface Span extends SpanContext {
    */
   data: { [key: string]: any };
 
+  /**
+   * @inheritDoc
+   */
+  attributes: SpanAttributes;
+
   /**
    * The transaction containing this span
    */
@@ -156,6 +177,18 @@ export interface Span extends SpanContext {
    */
   setData(key: string, value: any): this;
 
+  /**
+   * Set a single attribute on the span.
+   * Set it to `undefined` to remove the attribute.
+   */
+  setAttribute(key: string, value: SpanAttributeValue | undefined): void;
+
+  /**
+   * Set multiple attributes on the span.
+   * Any attribute set to `undefined` will be removed.
+   */
+  setAttributes(attributes: SpanAttributes): void;
+
   /**
    * Sets the status attribute on the current span
    * See: {@sentry/tracing SpanStatus} for possible values
diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts
