feat(types): Deprecate op on Span interface (#10217)

Lms24 · web-flow · commit b3f05893f18b · 2024-01-18T14:05:54.000+01:00
As discovered during #10208, the `op` property of the `Span` interface was still missing a deprecation because `Span` inherited it from `SpanContext` which I missed.
diff --git a/dev-packages/browser-integration-tests/suites/public-api/startTransaction/basic_usage/test.ts b/dev-packages/browser-integration-tests/suites/public-api/startTransaction/basic_usage/test.ts
@@ -29,16 +29,20 @@ sentryTest('should report finished spans as children of the root transaction', a
   expect(transaction.spans).toHaveLength(3);
 
   const span_1 = transaction.spans?.[0];
+
+  // eslint-disable-next-line deprecation/deprecation
   expect(span_1?.op).toBe('span_1');
   expect(span_1?.parentSpanId).toEqual(rootSpanId);
   // eslint-disable-next-line deprecation/deprecation
   expect(span_1?.data).toMatchObject({ foo: 'bar', baz: [1, 2, 3] });
 
   const span_3 = transaction.spans?.[1];
+  // eslint-disable-next-line deprecation/deprecation
   expect(span_3?.op).toBe('span_3');
   expect(span_3?.parentSpanId).toEqual(rootSpanId);
 
   const span_5 = transaction.spans?.[2];
+  // eslint-disable-next-line deprecation/deprecation
   expect(span_5?.op).toBe('span_5');
   // eslint-disable-next-line deprecation/deprecation
   expect(span_5?.parentSpanId).toEqual(span_3?.spanId);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/http-timings/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browsertracing/http-timings/test.ts
@@ -26,6 +26,7 @@ sentryTest('should create fetch spans with http timing @firefox', async ({ brows
   const envelopes = await getMultipleSentryEnvelopeRequests<Event>(page, 2, { url, timeout: 10000 });
   const tracingEvent = envelopes[envelopes.length - 1]; // last envelope contains tracing data on all browsers
 
+  // eslint-disable-next-line deprecation/deprecation
   const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client');
 
   expect(requestSpans).toHaveLength(3);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/long-tasks-disabled/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browsertracing/long-tasks-disabled/test.ts
@@ -16,6 +16,7 @@ sentryTest('should not capture long task when flag is disabled.', async ({ brows
   const url = await getLocalTestPath({ testDir: __dirname });
 
   const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+  // eslint-disable-next-line deprecation/deprecation
   const uiSpans = eventData.spans?.filter(({ op }) => op?.startsWith('ui'));
 
   expect(uiSpans?.length).toBe(0);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/long-tasks-enabled/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browsertracing/long-tasks-enabled/test.ts
@@ -16,6 +16,7 @@ sentryTest('should capture long task.', async ({ browserName, getLocalTestPath,
   const url = await getLocalTestPath({ testDir: __dirname });
 
   const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+  // eslint-disable-next-line deprecation/deprecation
   const uiSpans = eventData.spans?.filter(({ op }) => op?.startsWith('ui'));
 
   expect(uiSpans?.length).toBeGreaterThan(0);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-browser-spans/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-browser-spans/test.ts
@@ -12,6 +12,7 @@ sentryTest('should add browser-related spans to pageload transaction', async ({
   const url = await getLocalTestPath({ testDir: __dirname });
 
   const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+  // eslint-disable-next-line deprecation/deprecation
   const browserSpans = eventData.spans?.filter(({ op }) => op === 'browser');
 
   // Spans `connect`, `cache` and `DNS` are not always inside `pageload` transaction.
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-resource-spans/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-resource-spans/test.ts
@@ -18,6 +18,7 @@ sentryTest('should add resource spans to pageload transaction', async ({ getLoca
   const url = await getLocalTestPath({ testDir: __dirname });
 
   const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+  // eslint-disable-next-line deprecation/deprecation
   const resourceSpans = eventData.spans?.filter(({ op }) => op?.startsWith('resource'));
 
   // Webkit 16.0 (which is linked to Playwright 1.27.1) consistently creates 2 consectutive spans for `css`,
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-fid/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-fid/test.ts
@@ -25,6 +25,7 @@ sentryTest('should capture a FID vital.', async ({ browserName, getLocalTestPath
   const fidSpan = eventData.spans?.filter(({ description }) => description === 'first input delay')[0];
 
   expect(fidSpan).toBeDefined();
+  // eslint-disable-next-line deprecation/deprecation
   expect(fidSpan?.op).toBe('ui.action');
   expect(fidSpan?.parentSpanId).toBe(eventData.contexts?.trace_span_id);
 });
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-fp-fcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-fp-fcp/test.ts
@@ -20,6 +20,7 @@ sentryTest('should capture FP vital.', async ({ browserName, getLocalTestPath, p
   const fpSpan = eventData.spans?.filter(({ description }) => description === 'first-paint')[0];
 
   expect(fpSpan).toBeDefined();
+  // eslint-disable-next-line deprecation/deprecation
   expect(fpSpan?.op).toBe('paint');
   expect(fpSpan?.parentSpanId).toBe(eventData.contexts?.trace_span_id);
 });
@@ -39,6 +40,7 @@ sentryTest('should capture FCP vital.', async ({ getLocalTestPath, page }) => {
   const fcpSpan = eventData.spans?.filter(({ description }) => description === 'first-contentful-paint')[0];
 
   expect(fcpSpan).toBeDefined();
+  // eslint-disable-next-line deprecation/deprecation
   expect(fcpSpan?.op).toBe('paint');
   expect(fcpSpan?.parentSpanId).toBe(eventData.contexts?.trace_span_id);
 });
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/fetch/test.ts b/dev-packages/browser-integration-tests/suites/tracing/request/fetch/test.ts
@@ -25,6 +25,7 @@ sentryTest('should create spans for multiple fetch requests', async ({ getLocalT
   const envelopes = await getMultipleSentryEnvelopeRequests<Event>(page, 2, { url, timeout: 10000 });
   const tracingEvent = envelopes[envelopes.length - 1]; // last envelope contains tracing data on all browsers
 
+  // eslint-disable-next-line deprecation/deprecation
   const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client');
 
   expect(requestSpans).toHaveLength(3);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/xhr/test.ts b/dev-packages/browser-integration-tests/suites/tracing/request/xhr/test.ts
@@ -13,6 +13,7 @@ sentryTest('should create spans for multiple XHR requests', async ({ getLocalTes
   const url = await getLocalTestPath({ testDir: __dirname });
 
   const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+  // eslint-disable-next-line deprecation/deprecation
   const requestSpans = eventData.spans?.filter(({ op }) => op === 'http.client');
 
   expect(requestSpans).toHaveLength(3);
diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts
@@ -1,4 +1,11 @@
-import { Hub, addTracingExtensions, getCurrentScope, makeMain, spanToJSON } from '../../../src';
+import {
+  Hub,
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  addTracingExtensions,
+  getCurrentScope,
+  makeMain,
+  spanToJSON,
+} from '../../../src';
 import { Scope } from '../../../src/scope';
 import {
   Span,
@@ -116,14 +123,16 @@ describe('startSpan', () => {
       expect(ref.parentSpanId).toEqual('1234567890123456');
     });
 
-    it('allows for transaction to be mutated', async () => {
+    // TODO (v8): Remove this test in favour of the one below
+    it('(deprecated op) allows for transaction to be mutated', async () => {
       let ref: any = undefined;
       client.on('finishTransaction', transaction => {
         ref = transaction;
       });
       try {
         await startSpan({ name: 'GET users/[id]' }, span => {
           if (span) {
+            // eslint-disable-next-line deprecation/deprecation
             span.op = 'http.server';
           }
           return callback();
@@ -132,6 +141,25 @@ describe('startSpan', () => {
         //
       }
 
+      expect(spanToJSON(ref).op).toEqual('http.server');
+    });
+
+    it('allows for transaction to be mutated', async () => {
+      let ref: any = undefined;
+      client.on('finishTransaction', transaction => {
+        ref = transaction;
+      });
+      try {
+        await startSpan({ name: 'GET users/[id]' }, span => {
+          if (span) {
+            span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'http.server');
+          }
+          return callback();
+        });
+      } catch (e) {
+        //
+      }
+
       expect(ref.op).toEqual('http.server');
     });
 
@@ -156,7 +184,8 @@ describe('startSpan', () => {
       expect(ref.spanRecorder.spans[1].status).toEqual(isError ? 'internal_error' : undefined);
     });
 
-    it('allows for span to be mutated', async () => {
+    // TODO (v8): Remove this test in favour of the one below
+    it('(deprecated op) allows for span to be mutated', async () => {
       let ref: any = undefined;
       client.on('finishTransaction', transaction => {
         ref = transaction;
@@ -165,6 +194,7 @@ describe('startSpan', () => {
         await startSpan({ name: 'GET users/[id]', parentSampled: true }, () => {
           return startSpan({ name: 'SELECT * from users' }, childSpan => {
             if (childSpan) {
+              // eslint-disable-next-line deprecation/deprecation
               childSpan.op = 'db.query';
             }
             return callback();
@@ -177,6 +207,28 @@ describe('startSpan', () => {
       expect(ref.spanRecorder.spans).toHaveLength(2);
       expect(ref.spanRecorder.spans[1].op).toEqual('db.query');
     });
+
+    it('allows for span to be mutated', async () => {
+      let ref: any = undefined;
+      client.on('finishTransaction', transaction => {
+        ref = transaction;
+      });
+      try {
+        await startSpan({ name: 'GET users/[id]', parentSampled: true }, () => {
+          return startSpan({ name: 'SELECT * from users' }, childSpan => {
+            if (childSpan) {
+              childSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'db.query');
+            }
+            return callback();
+          });
+        });
+      } catch (e) {
+        //
+      }
+
+      expect(ref.spanRecorder.spans).toHaveLength(2);
+      expect(spanToJSON(ref.spanRecorder.spans[1]).op).toEqual('db.query');
+    });
   });
 
   it('creates & finishes span', async () => {
diff --git a/packages/ember/tests/helpers/utils.ts b/packages/ember/tests/helpers/utils.ts
@@ -66,12 +66,15 @@ export function assertSentryTransactions(
   // instead of checking the specific order of runloop spans (which is brittle),
   // we check (below) that _any_ runloop spans are added
   const filteredSpans = spans
+    // eslint-disable-next-line deprecation/deprecation
     .filter(span => !span.op?.startsWith('ui.ember.runloop.'))
     .map(s => {
+      // eslint-disable-next-line deprecation/deprecation
       return `${s.op} | ${spanToJSON(s).description}`;
     });
 
   assert.true(
+    // eslint-disable-next-line deprecation/deprecation
     spans.some(span => span.op?.startsWith('ui.ember.runloop.')),
     'it captures runloop spans',
   );
diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts
@@ -204,7 +204,7 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi
   };
   sentrySpan.setAttributes(allData);
 
-  sentrySpan.op = op;
+  sentrySpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, op);
   sentrySpan.updateName(description);
 }
 
diff --git a/packages/opentelemetry-node/test/spanprocessor.test.ts b/packages/opentelemetry-node/test/spanprocessor.test.ts
@@ -469,12 +469,16 @@ describe('SentrySpanProcessor', () => {
 
           child.updateName('new name');
 
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe(undefined);
+          expect(sentrySpan && spanToJSON(sentrySpan).op).toBe(undefined);
           expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('SELECT * FROM users;');
 
           child.end();
 
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe(undefined);
+          expect(sentrySpan && spanToJSON(sentrySpan).op).toBe(undefined);
           expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('new name');
 
           parentOtelSpan.end();
@@ -493,7 +497,9 @@ describe('SentrySpanProcessor', () => {
 
           child.end();
 
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe('http.client');
+          expect(spanToJSON(sentrySpan!).op).toBe('http.client');
 
           parentOtelSpan.end();
         });
@@ -511,7 +517,9 @@ describe('SentrySpanProcessor', () => {
 
           child.end();
 
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe('http.server');
+          expect(spanToJSON(sentrySpan!).op).toBe('http.server');
 
           parentOtelSpan.end();
         });
@@ -692,8 +700,12 @@ describe('SentrySpanProcessor', () => {
 
           child.end();
 
+          const { description, op } = spanToJSON(sentrySpan!);
+
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe('db');
-          expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('SELECT * FROM users');
+          expect(op).toBe('db');
+          expect(description).toBe('SELECT * FROM users');
 
           parentOtelSpan.end();
         });
@@ -711,8 +723,12 @@ describe('SentrySpanProcessor', () => {
 
           child.end();
 
+          const { description, op } = spanToJSON(sentrySpan!);
+
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe('db');
-          expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('fetch users from DB');
+          expect(op).toBe('db');
+          expect(description).toBe('fetch users from DB');
 
           parentOtelSpan.end();
         });
@@ -730,8 +746,11 @@ describe('SentrySpanProcessor', () => {
 
           child.end();
 
+          const { op, description } = spanToJSON(sentrySpan!);
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe('rpc');
-          expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('test operation');
+          expect(op).toBe('rpc');
+          expect(description).toBe('test operation');
 
           parentOtelSpan.end();
         });
@@ -749,8 +768,12 @@ describe('SentrySpanProcessor', () => {
 
           child.end();
 
+          const { op, description } = spanToJSON(sentrySpan!);
+
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe('message');
-          expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('test operation');
+          expect(op).toBe('message');
+          expect(description).toBe('test operation');
 
           parentOtelSpan.end();
         });
@@ -768,8 +791,12 @@ describe('SentrySpanProcessor', () => {
 
           child.end();
 
+          const { op, description } = spanToJSON(sentrySpan!);
+
+          // eslint-disable-next-line deprecation/deprecation
           expect(sentrySpan?.op).toBe('test faas trigger');
-          expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('test operation');
+          expect(op).toBe('test faas trigger');
+          expect(description).toBe('test operation');
 
           parentOtelSpan.end();
         });
diff --git a/packages/tracing-internal/test/browser/metrics/utils.test.ts b/packages/tracing-internal/test/browser/metrics/utils.test.ts
@@ -13,7 +13,9 @@ describe('_startChild()', () => {
 
     expect(span).toBeInstanceOf(Span);
     expect(spanToJSON(span).description).toBe('evaluation');
+    // eslint-disable-next-line deprecation/deprecation
     expect(span.op).toBe('script');
+    expect(spanToJSON(span).op).toBe('script');
   });
 
   it('adjusts the start timestamp if child span starts before transaction', () => {
diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts
@@ -166,13 +166,21 @@ export interface SpanContext {
 }
 
 /** Span holding trace_id, span_id */
-export interface Span extends SpanContext {
+export interface Span extends Omit<SpanContext, 'op' | 'status'> {
   /**
    * Human-readable identifier for the span. Identical to span.description.
    * @deprecated Use `spanToJSON(span).description` instead.
    */
   name: string;
 
+  /**
+   * Operation of the Span.
+   *
+   * @deprecated Use `startSpan()` functions to set, `span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'op')
+   * to update and `spanToJSON().op` to read the op instead
+   */
+  op?: string;
+
   /**
    * The ID of the span.
    * @deprecated Use `spanContext().spanId` instead.
