ref: Remove unnecessary checks for SpanJSON.data existence (#14731)

mydea · web-flow · commit f862dd6777fe · 2024-12-17T08:56:30.000+01:00
Since this was reworked in #14693 to always return something, we can safe some checks/fallbacks. This most likely does not change all the things, but focuses on the places that were easy to find/replace.
diff --git a/packages/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts
@@ -208,7 +208,7 @@ function isPerformanceResourceTiming(entry: PerformanceEntry): entry is Performa
  * @param span A span that has yet to be finished, must contain `url` on data.
  */
 function addHTTPTimings(span: Span): void {
-  const { url } = spanToJSON(span).data || {};
+  const { url } = spanToJSON(span).data;
 
   if (!url || typeof url !== 'string') {
     return;
diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts
@@ -426,7 +426,7 @@ describe('browserTracingIntegration', () => {
     const pageloadSpan = getActiveSpan();
 
     expect(spanToJSON(pageloadSpan!).description).toBe('changed');
-    expect(spanToJSON(pageloadSpan!).data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toBe('custom');
+    expect(spanToJSON(pageloadSpan!).data[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toBe('custom');
   });
 
   describe('startBrowserTracingNavigationSpan', () => {
@@ -608,7 +608,7 @@ describe('browserTracingIntegration', () => {
       const pageloadSpan = getActiveSpan();
 
       expect(spanToJSON(pageloadSpan!).description).toBe('changed');
-      expect(spanToJSON(pageloadSpan!).data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toBe('custom');
+      expect(spanToJSON(pageloadSpan!).data[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toBe('custom');
     });
 
     it('sets the navigation span name on `scope.transactionName`', () => {
diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts
@@ -94,7 +94,7 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
   // Else, we generate it from the span
   const dsc = getDynamicSamplingContextFromClient(span.spanContext().traceId, client);
   const jsonSpan = spanToJSON(rootSpan);
-  const attributes = jsonSpan.data || {};
+  const attributes = jsonSpan.data;
   const maybeSampleRate = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE];
 
   if (maybeSampleRate != null) {
diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts
@@ -1,5 +1,5 @@
 import { getClient, getCurrentScope } from '../currentScopes';
-import type { Span, SpanAttributes, StartSpanOptions } from '../types-hoist';
+import type { Span, StartSpanOptions } from '../types-hoist';
 
 import { DEBUG_BUILD } from '../debug-build';
 import { SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON } from '../semanticAttributes';
@@ -255,7 +255,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
       return;
     }
 
-    const attributes: SpanAttributes = spanJSON.data || {};
+    const attributes = spanJSON.data;
     if (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON]) {
       span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON, _finishReason);
     }
diff --git a/packages/core/test/lib/tracing/sentrySpan.test.ts b/packages/core/test/lib/tracing/sentrySpan.test.ts
@@ -30,7 +30,7 @@ describe('SentrySpan', () => {
 
       const spanJson = spanToJSON(span);
       expect(spanJson.description).toEqual('new name');
-      expect(spanJson.data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toEqual('custom');
+      expect(spanJson.data[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toEqual('custom');
     });
   });
 
diff --git a/packages/nestjs/src/sdk.ts b/packages/nestjs/src/sdk.ts
@@ -30,7 +30,7 @@ export function init(options: NodeOptions | undefined = {}): NodeClient | undefi
 }
 
 function addNestSpanAttributes(span: Span): void {
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: app_creation, request_context, handler
   const type = attributes['nestjs.type'];
diff --git a/packages/nestjs/src/setup.ts b/packages/nestjs/src/setup.ts
@@ -252,7 +252,7 @@ Module({
 export { SentryModule };
 
 function addNestSpanAttributes(span: Span): void {
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: app_creation, request_context, handler
   const type = attributes['nestjs.type'];
diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts
@@ -308,7 +308,6 @@ export function init(options: NodeOptions): NodeClient | undefined {
       event.type === 'transaction' &&
       event.contexts?.trace?.data?.['next.span_type'] === 'BaseServer.handleRequest'
     ) {
-      event.contexts.trace.data = event.contexts.trace.data || {};
       event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_OP] = 'http.server';
       event.contexts.trace.op = 'http.server';
 
diff --git a/packages/node/src/integrations/tracing/connect.ts b/packages/node/src/integrations/tracing/connect.ts
@@ -89,7 +89,7 @@ export const setupConnectErrorHandler = (app: ConnectApp): void => {
 };
 
 function addConnectSpanAttributes(span: Span): void {
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: middleware, request_handler
   const type = attributes['connect.type'];
diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts
@@ -26,7 +26,7 @@ export const instrumentExpress = generateInstrumentOnce(
       requestHook(span) {
         addOriginToSpan(span, 'auto.http.otel.express');
 
-        const attributes = spanToJSON(span).data || {};
+        const attributes = spanToJSON(span).data;
         // this is one of: middleware, request_handler, router
         const type = attributes['express.type'];
 
diff --git a/packages/node/src/integrations/tracing/fastify.ts b/packages/node/src/integrations/tracing/fastify.ts
@@ -136,7 +136,7 @@ export function setupFastifyErrorHandler(fastify: Fastify): void {
 }
 
 function addFastifySpanAttributes(span: Span): void {
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: middleware, request_handler
   const type = attributes['fastify.type'];
diff --git a/packages/node/src/integrations/tracing/graphql.ts b/packages/node/src/integrations/tracing/graphql.ts
@@ -47,7 +47,7 @@ export const instrumentGraphql = generateInstrumentOnce<GraphqlOptions>(
       responseHook(span) {
         addOriginToSpan(span, 'auto.graphql.otel.graphql');
 
-        const attributes = spanToJSON(span).data || {};
+        const attributes = spanToJSON(span).data;
 
         // If operation.name is not set, we fall back to use operation.type only
         const operationType = attributes['graphql.operation.type'];
@@ -58,7 +58,7 @@ export const instrumentGraphql = generateInstrumentOnce<GraphqlOptions>(
 
           // We guard to only do this on http.server spans
 
-          const rootSpanAttributes = spanToJSON(rootSpan).data || {};
+          const rootSpanAttributes = spanToJSON(rootSpan).data;
 
           const existingOperations = rootSpanAttributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION] || [];
 
diff --git a/packages/node/src/integrations/tracing/hapi/index.ts b/packages/node/src/integrations/tracing/hapi/index.ts
@@ -128,7 +128,7 @@ export async function setupHapiErrorHandler(server: Server): Promise<void> {
 }
 
 function addHapiSpanAttributes(span: Span): void {
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: router, plugin, server.ext
   const type = attributes['hapi.type'];
diff --git a/packages/node/src/integrations/tracing/knex.ts b/packages/node/src/integrations/tracing/knex.ts
@@ -22,7 +22,7 @@ const _knexIntegration = (() => {
         const { data } = spanToJSON(span);
         // knex.version is always set in the span data
         // https://github.com/open-telemetry/opentelemetry-js-contrib/blob/0309caeafc44ac9cb13a3345b790b01b76d0497d/plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts#L138
-        if (data && 'knex.version' in data) {
+        if ('knex.version' in data) {
           span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.knex');
         }
       });
diff --git a/packages/node/src/integrations/tracing/koa.ts b/packages/node/src/integrations/tracing/koa.ts
@@ -105,7 +105,7 @@ export const setupKoaErrorHandler = (app: { use: (arg0: (ctx: any, next: any) =>
 function addKoaSpanAttributes(span: Span): void {
   span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.http.otel.koa');
 
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: middleware, router
   const type = attributes['koa.type'];
diff --git a/packages/node/src/integrations/tracing/nest/nest.ts b/packages/node/src/integrations/tracing/nest/nest.ts
@@ -135,7 +135,7 @@ export function setupNestErrorHandler(app: MinimalNestJsApp, baseFilter: NestJsE
 }
 
 function addNestSpanAttributes(span: Span): void {
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: app_creation, request_context, handler
   const type = attributes['nestjs.type'];
diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts
@@ -49,8 +49,8 @@ const cacheResponseHook: RedisResponseCustomAttributeFunction = (span: Span, red
 
   // otel/ioredis seems to be using the old standard, as there was a change to those params: https://github.com/open-telemetry/opentelemetry-specification/issues/3199
   // We are using params based on the docs: https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/
-  const networkPeerAddress = spanToJSON(span).data?.['net.peer.name'];
-  const networkPeerPort = spanToJSON(span).data?.['net.peer.port'];
+  const networkPeerAddress = spanToJSON(span).data['net.peer.name'];
+  const networkPeerPort = spanToJSON(span).data['net.peer.port'];
   if (networkPeerPort && networkPeerAddress) {
     span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort });
   }
diff --git a/packages/node/src/integrations/tracing/tedious.ts b/packages/node/src/integrations/tracing/tedious.ts
@@ -27,7 +27,7 @@ const _tediousIntegration = (() => {
       client.on('spanStart', span => {
         const { description, data } = spanToJSON(span);
         // Tedius integration always set a span name and `db.system` attribute to `mssql`.
-        if (!description || data?.['db.system'] !== 'mssql') {
+        if (!description || data['db.system'] !== 'mssql') {
           return;
         }
 
diff --git a/packages/node/src/integrations/tracing/vercelai/index.ts b/packages/node/src/integrations/tracing/vercelai/index.ts
@@ -18,24 +18,21 @@ const _vercelAIIntegration = (() => {
         for (const span of event.spans) {
           const { data: attributes, description: name } = span;
 
-          if (!attributes || !name || span.origin !== 'auto.vercelai.otel') {
+          if (!name || span.origin !== 'auto.vercelai.otel') {
             continue;
           }
 
-          // attributes around token usage can only be set on span finish
-          span.data = span.data || {};
-
           if (attributes['ai.usage.completionTokens'] != undefined) {
-            span.data['ai.completion_tokens.used'] = attributes['ai.usage.completionTokens'];
+            attributes['ai.completion_tokens.used'] = attributes['ai.usage.completionTokens'];
           }
           if (attributes['ai.usage.promptTokens'] != undefined) {
-            span.data['ai.prompt_tokens.used'] = attributes['ai.usage.promptTokens'];
+            attributes['ai.prompt_tokens.used'] = attributes['ai.usage.promptTokens'];
           }
           if (
             typeof attributes['ai.usage.completionTokens'] == 'number' &&
             typeof attributes['ai.usage.promptTokens'] == 'number'
           ) {
-            span.data['ai.total_tokens.used'] =
+            attributes['ai.total_tokens.used'] =
               attributes['ai.usage.completionTokens'] + attributes['ai.usage.promptTokens'];
           }
         }
@@ -51,7 +48,7 @@ const _vercelAIIntegration = (() => {
 
         const { data: attributes, description: name } = spanToJSON(span);
 
-        if (!attributes || !name) {
+        if (!name) {
           return;
         }
 
diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts
@@ -315,7 +315,7 @@ function getCurrentURL(span: Span): string | undefined {
   const spanData = spanToJSON(span).data;
   // `ATTR_URL_FULL` is the new attribute, but we still support the old one, `SEMATTRS_HTTP_URL`, for now.
   // eslint-disable-next-line deprecation/deprecation
-  const urlAttribute = spanData?.[SEMATTRS_HTTP_URL] || spanData?.[ATTR_URL_FULL];
+  const urlAttribute = spanData[SEMATTRS_HTTP_URL] || spanData[ATTR_URL_FULL];
   if (typeof urlAttribute === 'string') {
     return urlAttribute;
   }
diff --git a/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts b/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts
@@ -21,7 +21,7 @@ export function enhanceDscWithOpenTelemetryRootSpanName(client: Client): void {
     // This mutates the passed-in DSC
 
     const jsonSpan = spanToJSON(rootSpan);
-    const attributes = jsonSpan.data || {};
+    const attributes = jsonSpan.data;
     const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
 
     const { description } = spanHasName(rootSpan) ? parseSpanDescription(rootSpan) : { description: undefined };
diff --git a/packages/remix/src/utils/integrations/opentelemetry.ts b/packages/remix/src/utils/integrations/opentelemetry.ts
@@ -34,7 +34,7 @@ const _remixIntegration = (() => {
 }) satisfies IntegrationFn;
 
 const addRemixSpanAttributes = (span: Span): void => {
-  const attributes = spanToJSON(span).data || {};
+  const attributes = spanToJSON(span).data;
 
   // this is one of: loader, action, requestHandler
   const type = attributes['code.function'];
diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts
@@ -105,7 +105,7 @@ export function instrumentVueRouter(
     if (options.instrumentPageLoad && isPageLoadNavigation) {
       const activeRootSpan = getActiveRootSpan();
       if (activeRootSpan) {
-        const existingAttributes = spanToJSON(activeRootSpan).data || {};
+        const existingAttributes = spanToJSON(activeRootSpan).data;
         if (existingAttributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] !== 'custom') {
           activeRootSpan.updateName(spanName);
           activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, transactionSource);
diff --git a/packages/vue/test/router.test.ts b/packages/vue/test/router.test.ts
@@ -8,6 +8,10 @@ import type { Span, SpanAttributes } from '@sentry/core';
 import type { Route } from '../src/router';
 import { instrumentVueRouter } from '../src/router';
 
+const MOCK_SPAN = {
+  spanContext: () => ({ traceId: '1234', spanId: '5678' }),
+};
+
 const captureExceptionSpy = vi.spyOn(SentryBrowser, 'captureException');
 vi.mock('@sentry/core', async () => {
   const actual = await vi.importActual('@sentry/core');
@@ -76,7 +80,7 @@ describe('instrumentVueRouter()', () => {
   });
 
   it('should return instrumentation that instruments VueRouter.onError', () => {
-    const mockStartSpan = vi.fn();
+    const mockStartSpan = vi.fn().mockReturnValue(MOCK_SPAN);
     instrumentVueRouter(
       mockVueRouter,
       { routeLabel: 'name', instrumentPageLoad: true, instrumentNavigation: true },
@@ -103,7 +107,7 @@ describe('instrumentVueRouter()', () => {
   ])(
     'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for navigations',
     (fromKey, toKey, transactionName, transactionSource) => {
-      const mockStartSpan = vi.fn();
+      const mockStartSpan = vi.fn().mockReturnValue(MOCK_SPAN);
       instrumentVueRouter(
         mockVueRouter,
         { routeLabel: 'name', instrumentPageLoad: true, instrumentNavigation: true },
@@ -143,7 +147,8 @@ describe('instrumentVueRouter()', () => {
     'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for pageloads',
     (fromKey, toKey, transactionName, transactionSource) => {
       const mockRootSpan = {
-        getSpanJSON: vi.fn().mockReturnValue({ op: 'pageload' }),
+        ...MOCK_SPAN,
+        getSpanJSON: vi.fn().mockReturnValue({ op: 'pageload', data: {} }),
         updateName: vi.fn(),
         setAttribute: vi.fn(),
         setAttributes: vi.fn(),
@@ -183,7 +188,7 @@ describe('instrumentVueRouter()', () => {
   );
 
   it('allows to configure routeLabel=path', () => {
-    const mockStartSpan = vi.fn();
+    const mockStartSpan = vi.fn().mockReturnValue(MOCK_SPAN);
     instrumentVueRouter(
       mockVueRouter,
       { routeLabel: 'path', instrumentPageLoad: true, instrumentNavigation: true },
@@ -211,7 +216,7 @@ describe('instrumentVueRouter()', () => {
   });
 
   it('allows to configure routeLabel=name', () => {
-    const mockStartSpan = vi.fn();
+    const mockStartSpan = vi.fn().mockReturnValue(MOCK_SPAN);
     instrumentVueRouter(
       mockVueRouter,
       { routeLabel: 'name', instrumentPageLoad: true, instrumentNavigation: true },
@@ -240,6 +245,7 @@ describe('instrumentVueRouter()', () => {
 
   it("doesn't overwrite a pageload transaction name it was set to custom before the router resolved the route", () => {
     const mockRootSpan = {
+      ...MOCK_SPAN,
       updateName: vi.fn(),
       setAttribute: vi.fn(),
       setAttributes: vi.fn(),
@@ -294,9 +300,7 @@ describe('instrumentVueRouter()', () => {
   });
 
   it("updates the scope's `transactionName` when a route is resolved", () => {
-    const mockStartSpan = vi.fn().mockImplementation(_ => {
-      return {};
-    });
+    const mockStartSpan = vi.fn().mockReturnValue(MOCK_SPAN);
 
     const scopeSetTransactionNameSpy = vi.fn();
 
@@ -329,6 +333,7 @@ describe('instrumentVueRouter()', () => {
     'should return instrumentation that considers the instrumentPageLoad = %p',
     (instrumentPageLoad, expectedCallsAmount) => {
       const mockRootSpan = {
+        ...MOCK_SPAN,
         updateName: vi.fn(),
         setData: vi.fn(),
         setAttribute: vi.fn(),
@@ -367,7 +372,7 @@ describe('instrumentVueRouter()', () => {
   ])(
     'should return instrumentation that considers the instrumentNavigation = %p',
     (instrumentNavigation, expectedCallsAmount) => {
-      const mockStartSpan = vi.fn();
+      const mockStartSpan = vi.fn().mockReturnValue(MOCK_SPAN);
       instrumentVueRouter(
         mockVueRouter,
         { routeLabel: 'name', instrumentPageLoad: true, instrumentNavigation },
@@ -386,7 +391,7 @@ describe('instrumentVueRouter()', () => {
   );
 
   it("doesn't throw when `next` is not available in the beforeEach callback (Vue Router 4)", () => {
-    const mockStartSpan = vi.fn();
+    const mockStartSpan = vi.fn().mockReturnValue(MOCK_SPAN);
     instrumentVueRouter(
       mockVueRouter,
       { routeLabel: 'path', instrumentPageLoad: true, instrumentNavigation: true },

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`import { getClient, getCurrentScope } from '../currentScopes';`
`2`		`-import type { Span, SpanAttributes, StartSpanOptions } from '../types-hoist';`
	`2`	`+import type { Span, StartSpanOptions } from '../types-hoist';`
`3`	`3`
`4`	`4`	`import { DEBUG_BUILD } from '../debug-build';`
`5`	`5`	`import { SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON } from '../semanticAttributes';`
`@@ -255,7 +255,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti`
`255`	`255`	`return;`
`256`	`256`	`}`
`257`	`257`
`258`		`- const attributes: SpanAttributes = spanJSON.data \|\| {};`
	`258`	`+ const attributes = spanJSON.data;`
`259`	`259`	`if (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON]) {`
`260`	`260`	`span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON, _finishReason);`
`261`	`261`	`}`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ export function init(options: NodeOptions \| undefined = {}): NodeClient \| undefi`
`30`	`30`	`}`
`31`	`31`
`32`	`32`	`function addNestSpanAttributes(span: Span): void {`
`33`		`- const attributes = spanToJSON(span).data \|\| {};`
	`33`	`+ const attributes = spanToJSON(span).data;`
`34`	`34`
`35`	`35`	`// this is one of: app_creation, request_context, handler`
`36`	`36`	`const type = attributes['nestjs.type'];`