node tests and jsdoc

Lms24 · Lms24 · commit c7b6718d841e · 2024-11-14T15:58:19.000+01:00
diff --git a/dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js b/dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js
@@ -0,0 +1,50 @@
+const { loggingTransport } = require('@sentry-internal/node-integration-tests');
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  // disable attaching headers to /test/* endpoints
+  tracePropagationTargets: [/^(?!.*test).*$/],
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+});
+
+// express must be required after Sentry is initialized
+const express = require('express');
+const cors = require('cors');
+const bodyParser = require('body-parser');
+const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests');
+
+const app = express();
+
+app.use(cors());
+app.use(bodyParser.json());
+app.use(bodyParser.text());
+app.use(bodyParser.raw());
+
+app.get('/test/:id/span-updateName', (_req, res) => {
+  const span = Sentry.getActiveSpan();
+  const rootSpan = Sentry.getRootSpan(span);
+  rootSpan.updateName('new-name');
+  res.send({ response: 'response 1' });
+});
+
+app.get('/test/:id/span-updateName-source', (_req, res) => {
+  const span = Sentry.getActiveSpan();
+  const rootSpan = Sentry.getRootSpan(span);
+  rootSpan.updateName('new-name');
+  rootSpan.setAttribute(Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');
+  res.send({ response: 'response 2' });
+});
+
+app.get('/test/:id/updateSpanName', (_req, res) => {
+  const span = Sentry.getActiveSpan();
+  const rootSpan = Sentry.getRootSpan(span);
+  Sentry.updateSpanName(rootSpan, 'new-name');
+  res.send({ response: 'response 3' });
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);
diff --git a/dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts
@@ -0,0 +1,67 @@
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+describe('express tracing', () => {
+  afterAll(() => {
+    cleanupChildProcesses();
+  });
+
+  describe('CJS', () => {
+    // This test documents the unfortunate behaviour of using `span.updateName` on the server-side.
+    // For http.server root spans (which is the root span on the server 99% of the time), Otel's http instrumentation
+    // calls `span.updateName` and overwrites whatever the name was set to before (by us or by users).
+    test("calling just `span.updateName` doesn't update the final name in express (missing source)", done => {
+      createRunner(__dirname, 'server.js')
+        .expect({
+          transaction: {
+            transaction: 'GET /test/:id/span-updateName',
+            transaction_info: {
+              source: 'route',
+            },
+          },
+        })
+        .start(done)
+        .makeRequest('get', '/test/123/span-updateName');
+    });
+
+    // Also calling `updateName` AND setting a source doesn't change anything - Otel has no concept of source, this is sentry-internal.
+    // Therefore, only the source is updated but the name is still overwritten by Otel.
+    test("calling `span.updateName` and setting attribute source doesn't update the final name in express but it updates the source", done => {
+      createRunner(__dirname, 'server.js')
+        .expect({
+          transaction: {
+            transaction: 'GET /test/:id/span-updateName-source',
+            transaction_info: {
+              source: 'custom',
+            },
+          },
+        })
+        .start(done)
+        .makeRequest('get', '/test/123/span-updateName-source');
+    });
+
+    // This test documents the correct way to update the span name (and implicitly the source) in Node:
+    test('calling `Sentry.updateSpanName` updates the final name and source in express', done => {
+      createRunner(__dirname, 'server.js')
+        .expect({
+          transaction: txnEvent => {
+            expect(txnEvent).toMatchObject({
+              transaction: 'new-name',
+              transaction_info: {
+                source: 'custom',
+              },
+              contexts: {
+                trace: {
+                  data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' },
+                },
+              },
+            });
+            // ensure we delete the internal attribute once we're done with it
+            expect(txnEvent.contexts?.trace?.data?.['_sentry_span_name_set_by_user']).toBeUndefined();
+          },
+        })
+        .start(done)
+        .makeRequest('get', '/test/123/updateSpanName');
+    });
+  });
+});
diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/basic-usage/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/basic-usage/test.ts
@@ -1,11 +1,24 @@
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node';
 import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
 
 afterAll(() => {
   cleanupChildProcesses();
 });
 
-test('should send a manually started root span', done => {
+test('sends a manually started root span with source custom', done => {
   createRunner(__dirname, 'scenario.ts')
-    .expect({ transaction: { transaction: 'test_span' } })
+    .expect({
+      transaction: {
+        transaction: 'test_span',
+        transaction_info: { source: 'custom' },
+        contexts: {
+          trace: {
+            span_id: expect.any(String),
+            trace_id: expect.any(String),
+            data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' },
+          },
+        },
+      },
+    })
     .start(done);
 });
diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/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.0,
+  transport: loggingTransport,
+});
+
+Sentry.startSpan(
+  { name: 'test_span', attributes: { [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' } },
+  (span: Sentry.Span) => {
+    span.updateName('new name');
+  },
+);
diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/test.ts
@@ -0,0 +1,24 @@
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('updates the span name when calling `span.updateName`', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .expect({
+      transaction: {
+        transaction: 'new name',
+        transaction_info: { source: 'url' },
+        contexts: {
+          trace: {
+            span_id: expect.any(String),
+            trace_id: expect.any(String),
+            data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' },
+          },
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/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.0,
+  transport: loggingTransport,
+});
+
+Sentry.startSpan(
+  { name: 'test_span', attributes: { [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' } },
+  (span: Sentry.Span) => {
+    Sentry.updateSpanName(span, 'new name');
+  },
+);
diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/test.ts
@@ -0,0 +1,24 @@
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('updates the span name and source when calling `updateSpanName`', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .expect({
+      transaction: {
+        transaction: 'new name',
+        transaction_info: { source: 'custom' },
+        contexts: {
+          trace: {
+            span_id: expect.any(String),
+            trace_id: expect.any(String),
+            data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' },
+          },
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts
@@ -169,4 +169,10 @@ describe('httpIntegration', () => {
       runner.makeRequest('get', '/testRequest');
     });
   });
+
+  describe('does not override the span name set by the user', () => {
+    test('via `span.updateName`', done => {
+      createRunner(__dirname, 'server-updateName.js').start(done);
+    });
+  });
 });
diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts
@@ -91,6 +91,14 @@ export function createEventEnvelope(
   // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid
   // of this `delete`, lest we miss putting it back in the next time the property is in use.)
   delete event.sdkProcessingMetadata;
+  try {
+    // @ts-expect-error - for bundle size we try/catch the access to this property
+    delete event.contexts.trace.data._sentry_span_name_set_by_user;
+    // @ts-expect-error - for bundle size we try/catch the access to this property
+    event.spans.forEach(span => delete span.data._sentry_span_name_set_by_user);
+  } catch {
+    // Do nothing
+  }
 
   const eventItem: EventItem = [{ type: eventType }, event];
   return createEnvelope<EventEnvelope>(envelopeHeaders, [eventItem]);
diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts
@@ -307,4 +307,5 @@ export function updateMetricSummaryOnActiveSpan(
 export function updateSpanName(span: Span, name: string): void {
   span.updateName(name);
   span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');
+  span.setAttribute('_sentry_span_name_set_by_user', name);
 }
diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts
@@ -36,12 +36,12 @@ interface SpanDescription {
 /**
  * Infer the op & description for a set of name, attributes and kind of a span.
  */
-export function inferSpanData(originalName: string, attributes: SpanAttributes, kind: SpanKind): SpanDescription {
+export function inferSpanData(spanName: string, attributes: SpanAttributes, kind: SpanKind): SpanDescription {
   // if http.method exists, this is an http request span
   // eslint-disable-next-line deprecation/deprecation
   const httpMethod = attributes[ATTR_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD];
   if (httpMethod) {
-    return descriptionForHttpMethod({ attributes, name: originalName, kind }, httpMethod);
+    return descriptionForHttpMethod({ attributes, name: getOriginalName(spanName, attributes), kind }, httpMethod);
   }
 
   // eslint-disable-next-line deprecation/deprecation
@@ -53,7 +53,7 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes,
   // If db.type exists then this is a database call span
   // If the Redis DB is used as a cache, the span description should not be changed
   if (dbSystem && !opIsCache) {
-    return descriptionForDbSystem({ attributes, name: originalName });
+    return descriptionForDbSystem({ attributes, name: spanName });
   }
 
   const customSourceOrRoute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom' ? 'custom' : 'route';
@@ -64,7 +64,7 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes,
   if (rpcService) {
     return {
       op: 'rpc',
-      description: originalName,
+      description: getOriginalName(spanName, attributes),
       source: customSourceOrRoute,
     };
   }
@@ -75,7 +75,7 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes,
   if (messagingSystem) {
     return {
       op: 'message',
-      description: originalName,
+      description: getOriginalName(spanName, attributes),
       source: customSourceOrRoute,
     };
   }
@@ -84,10 +84,14 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes,
   // eslint-disable-next-line deprecation/deprecation
   const faasTrigger = attributes[SEMATTRS_FAAS_TRIGGER];
   if (faasTrigger) {
-    return { op: faasTrigger.toString(), description: originalName, source: customSourceOrRoute };
+    return {
+      op: faasTrigger.toString(),
+      description: getOriginalName(spanName, attributes),
+      source: customSourceOrRoute,
+    };
   }
 
-  return { op: undefined, description: originalName, source: 'custom' };
+  return { op: undefined, description: spanName, source: 'custom' };
 }
 
 /**
@@ -110,7 +114,7 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription {
 function descriptionForDbSystem({ attributes, name }: { attributes: Attributes; name: string }): SpanDescription {
   // if we already set the source to custom, we don't overwrite the span description but just adjust the op
   if (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom') {
-    return { op: 'db', description: name, source: 'custom' };
+    return { op: 'db', description: getOriginalName(name, attributes), source: 'custom' };
   }
 
   // Use DB statement (Ex "SELECT * FROM table") if possible as description.
@@ -146,7 +150,7 @@ export function descriptionForHttpMethod(
   const { urlPath, url, query, fragment, hasRoute } = getSanitizedUrl(attributes, kind);
 
   if (!urlPath) {
-    return { op: opParts.join('.'), description: name, source: 'custom' };
+    return { op: opParts.join('.'), description: getOriginalName(name, attributes), source: 'custom' };
   }
 
   const graphqlOperationsAttribute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION];
@@ -192,7 +196,7 @@ export function descriptionForHttpMethod(
 
   return {
     op: opParts.join('.'),
-    description: useInferredDescription ? description : name,
+    description: useInferredDescription ? description : getOriginalName(name, attributes),
     source: useInferredDescription ? source : 'custom',
     data,
   };
@@ -258,3 +262,11 @@ export function getSanitizedUrl(
 
   return { urlPath: undefined, url, query, fragment, hasRoute: false };
 }
+
+function getOriginalName(name: string, attributes: Attributes): string {
+  return attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom' &&
+    attributes['_sentry_span_name_set_by_user'] &&
+    typeof attributes['_sentry_span_name_set_by_user'] === 'string'
+    ? attributes['_sentry_span_name_set_by_user']
+    : name;
+}
diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts
@@ -234,6 +234,22 @@ export interface Span {
 
   /**
    * Update the name of the span.
+   *
+   * **Important:** You most likely want to use `Sentry.updateSpanName(span, name)` instead!
+   *
+   * This method will update the current span name but cannot guarantee that the new name will be
+   * the final name of the span. Instrumentation might still overwrite the name with an automatically
+   * computed name, for example in `http.server` or `db` spans.
+   *
+   * You can ensure that your name is kept and not overwritten by
+   * - either calling `Sentry.updateSpanName(span, name)`
+   * - or by calling `span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom')`
+   *   in addition to `span.updateName`.
+   *
+   * If you want to update a span name in a browser-only app, `span.updateName` and `Sentry.updateSpanName`
+   * are identical: In both cases, the name will not be overwritten by the Sentry SDK.
+   *
+   * @param name the new name of the span
    */
   updateName(name: string): this;
 

Original file line number	Diff line number	Diff line change
`@@ -307,4 +307,5 @@ export function updateMetricSummaryOnActiveSpan(`
`307`	`307`	`export function updateSpanName(span: Span, name: string): void {`
`308`	`308`	`span.updateName(name);`
`309`	`309`	`span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');`
	`310`	`+ span.setAttribute('_sentry_span_name_set_by_user', name);`
`310`	`311`	`}`