review suggestions, more tests

Author: Lms24

packages/core/src/tracing/sentrySpan.ts

@@ -264,7 +264,8 @@ export class SentrySpan implements Span {
       end_timestamp: this._endTime ?? this._startTime,
       is_segment: this._isStandaloneSpan || this === getRootSpan(this),
       status: getSimpleStatusMessage(this._status),
-      attributes: this._attributes,
+      // spread to avoid mutating the original object when later processing the span
+      attributes: { ...this._attributes },
       links: getStreamedSpanLinks(this._links),
     };
   }

packages/core/src/tracing/spans/captureSpan.ts

@@ -54,11 +54,11 @@ export function captureSpan(span: Span, client: Client): SerializedStreamedSpanW
 
   if (span === segmentSpan) {
     applyScopeToSegmentSpan(spanJSON, finalScopeData);
-    // Allow hook subscribers to add additional data to the segment span JSON
+    // Allow hook subscribers to mutate the segment span JSON
     client.emit('processSegmentSpan', spanJSON);
   }
 
-  // Allow hook subscribers to add additional data to the span JSON
+  // Allow hook subscribers to mutate the span JSON
   client.emit('processSpan', spanJSON);
 
   const { beforeSendSpan } = client.getOptions();
@@ -67,11 +67,12 @@ export function captureSpan(span: Span, client: Client): SerializedStreamedSpanW
       ? applyBeforeSendSpanCallback(spanJSON, beforeSendSpan)
       : spanJSON;
 
-  // Backfill sentry.span.source from sentry.source for the PoC
-  // TODO(v11): Stop sending `sentry.source` attribute and only send `sentry.span.source`
-  if (processedSpan.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]) {
+  // Backfill sentry.span.source from sentry.source. Only `sentry.span.source` is respected by Sentry.
+  // TODO(v11): Ensure we always only send `sentry.span.source` and remove this backfill.
+  const spanNameSource = processedSpan.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
+  if (spanNameSource) {
     safeSetSpanJSONAttributes(processedSpan, {
-      [SEMANTIC_ATTRIBUTE_SENTRY_SPAN_SOURCE]: processedSpan.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE],
+      [SEMANTIC_ATTRIBUTE_SENTRY_SPAN_SOURCE]: spanNameSource,
     });
     delete processedSpan.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
   }
@@ -142,7 +143,7 @@ export function safeSetSpanJSONAttributes(
   const originalAttributes = spanJSON.attributes ?? (spanJSON.attributes = {});
 
   Object.keys(newAttributes).forEach(key => {
-    if (!originalAttributes?.[key]) {
+    if (!(key in originalAttributes)) {
       originalAttributes[key] = newAttributes[key];
     }
   });

packages/core/src/utils/spanUtils.ts

@@ -230,7 +230,8 @@ export function spanToStreamedSpanJSON(span: Span): StreamedSpanJSON {
       end_timestamp: spanTimeInputToSeconds(endTime),
       is_segment: span === INTERNAL_getSegmentSpan(span),
       status: getSimpleStatusMessage(status),
-      attributes,
+      // spread to avoid mutating the original object when later processing the span
+      attributes: { ...attributes },
       links: getStreamedSpanLinks(links),
     };
   }

packages/core/test/lib/tracing/spans/captureSpan.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect, it, vi } from 'vitest';
+import type { StreamedSpanJSON } from '../../../../src';
 import {
   captureSpan,
   SEMANTIC_ATTRIBUTE_SENTRY_ENVIRONMENT,
@@ -20,6 +21,7 @@ import {
   withScope,
   withStreamedSpan,
 } from '../../../../src';
+import { safeSetSpanJSONAttributes } from '../../../../src/tracing/spans/captureSpan';
 import { getDefaultTestClientOptions, TestClient } from '../../../mocks/client';
 
 describe('captureSpan', () => {
@@ -414,3 +416,49 @@ describe('captureSpan', () => {
     });
   });
 });
+
+describe('safeSetSpanJSONAttributes', () => {
+  it('sets attributes that do not exist', () => {
+    const spanJSON = { attributes: { a: 1, b: 2 } };
+
+    // @ts-expect-error - only passing a partial object for this test
+    safeSetSpanJSONAttributes(spanJSON, { c: 3 });
+
+    expect(spanJSON.attributes).toEqual({ a: 1, b: 2, c: 3 });
+  });
+
+  it("doesn't set attributes that already exist", () => {
+    const spanJSON = { attributes: { a: 1, b: 2 } };
+    // @ts-expect-error - only passing a partial object for this test
+    safeSetSpanJSONAttributes(spanJSON, { a: 3 });
+
+    expect(spanJSON.attributes).toEqual({ a: 1, b: 2 });
+  });
+
+  it.each([null, undefined])("doesn't overwrite attributes previously set to %s", val => {
+    const spanJSON = { attributes: { a: val, b: 2 } };
+
+    // @ts-expect-error - only passing a partial object for this test
+    safeSetSpanJSONAttributes(spanJSON, { a: 1 });
+
+    expect(spanJSON.attributes).toEqual({ a: val, b: 2 });
+  });
+
+  it("doesn't overwrite falsy attribute values (%s)", () => {
+    const spanJSON = { attributes: { a: false, b: '', c: 0 } };
+
+    // @ts-expect-error - only passing a partial object for this test
+    safeSetSpanJSONAttributes(spanJSON, { a: 1, b: 'test', c: 1 });
+
+    expect(spanJSON.attributes).toEqual({ a: false, b: '', c: 0 });
+  });
+
+  it('handles an undefined attributes property', () => {
+    const spanJSON: Partial<StreamedSpanJSON> = {};
+
+    // @ts-expect-error - only passing a partial object for this test
+    safeSetSpanJSONAttributes(spanJSON, { a: 1 });
+
+    expect(spanJSON.attributes).toEqual({ a: 1 });
+  });
+});