adjust parseSpanDescription and tests

Author: Lms24

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js

@@ -1,3 +1,3 @@
 const activeSpan = Sentry.getActiveSpan();
 const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan);
-rootSpan.updateName('new name');
+rootSpan?.updateName('new name');

dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js

@@ -45,6 +45,14 @@ app.get('/test/:id/updateSpanName', (_req, res) => {
   res.send({ response: 'response 3' });
 });
 
+app.get('/test/:id/updateSpanNameAndSource', (_req, res) => {
+  const span = Sentry.getActiveSpan();
+  const rootSpan = Sentry.getRootSpan(span);
+  Sentry.updateSpanName(rootSpan, 'new-name');
+  rootSpan.setAttribute(Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'component');
+  res.send({ response: 'response 4' });
+});
+
 Sentry.setupExpressErrorHandler(app);
 
 startExpressServerAndSendPortToRunner(app);

dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts

@@ -52,6 +52,7 @@ describe('express tracing', () => {
               },
               contexts: {
                 trace: {
+                  op: 'http.server',
                   data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' },
                 },
               },
@@ -64,4 +65,29 @@ describe('express tracing', () => {
         .makeRequest('get', '/test/123/updateSpanName');
     });
   });
+
+  // This test documents the correct way to update the span name (and implicitly the source) in Node:
+  test('calling `Sentry.updateSpanName` and setting source subsequently updates the final name and sets correct source', done => {
+    createRunner(__dirname, 'server.js')
+      .expect({
+        transaction: txnEvent => {
+          expect(txnEvent).toMatchObject({
+            transaction: 'new-name',
+            transaction_info: {
+              source: 'component',
+            },
+            contexts: {
+              trace: {
+                op: 'http.server',
+                data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component' },
+              },
+            },
+          });
+          // 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/updateSpanNameAndSource');
+  });
 });

dev-packages/node-integration-tests/suites/public-api/startSpan/basic-usage/test.ts

@@ -22,3 +22,21 @@ test('sends a manually started root span with source custom', done => {
     })
     .start(done);
 });
+
+test("doesn't change the name for manually started spans even if attributes triggering inference are set", done => {
+  createRunner(__dirname, 'scenario.ts')
+    .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);
+});

packages/core/src/utils/spanUtils.ts

@@ -333,6 +333,8 @@ export function showSpanDropWarning(): void {
  */
 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);
+  span.setAttributes({
+    [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
+    ['_sentry_span_name_set_by_user']: name,
+  });
 }

packages/opentelemetry/src/utils/parseSpanDescription.ts

@@ -42,7 +42,7 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind
   // eslint-disable-next-line deprecation/deprecation
   const httpMethod = attributes[ATTR_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD];
   if (httpMethod) {
-    return descriptionForHttpMethod({ attributes, name: getOriginalName(spanName, attributes), kind }, httpMethod);
+    return descriptionForHttpMethod({ attributes, name: spanName, kind }, httpMethod);
   }
 
   // eslint-disable-next-line deprecation/deprecation
@@ -64,9 +64,8 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind
   const rpcService = attributes[SEMATTRS_RPC_SERVICE];
   if (rpcService) {
     return {
+      ...getUserUpdatedNameAndSource(spanName, attributes, 'route'),
       op: 'rpc',
-      description: getOriginalName(spanName, attributes),
-      source: customSourceOrRoute,
     };
   }
 
@@ -75,9 +74,8 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind
   const messagingSystem = attributes[SEMATTRS_MESSAGING_SYSTEM];
   if (messagingSystem) {
     return {
+      ...getUserUpdatedNameAndSource(spanName, attributes, customSourceOrRoute),
       op: 'message',
-      description: getOriginalName(spanName, attributes),
-      source: customSourceOrRoute,
     };
   }
 
@@ -86,9 +84,8 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind
   const faasTrigger = attributes[SEMATTRS_FAAS_TRIGGER];
   if (faasTrigger) {
     return {
+      ...getUserUpdatedNameAndSource(spanName, attributes, customSourceOrRoute),
       op: faasTrigger.toString(),
-      description: getOriginalName(spanName, attributes),
-      source: customSourceOrRoute,
     };
   }
 
@@ -108,14 +105,27 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription {
   const attributes = spanHasAttributes(span) ? span.attributes : {};
   const name = spanHasName(span) ? span.name : '<unknown>';
   const kind = getSpanKind(span);
+  // console.log('x parseSpanDesc', { attributes, name, kind });
 
-  return inferSpanData(name, attributes, kind);
+  const res = inferSpanData(name, attributes, kind);
+
+  // console.log('x parseSpanDesc res', res);
+  return res;
 }
 
 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 we already have a custom name, we don't overwrite it but only set the op
+  if (typeof attributes['_sentry_span_name_set_by_user'] === 'string') {
+    return {
+      op: 'db',
+      description: attributes['_sentry_span_name_set_by_user'],
+      source: (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionSource) || 'custom',
+    };
+  }
+
+  // if we already have the source set to custom, we don't overwrite the span description but only set the op
   if (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom') {
-    return { op: 'db', description: getOriginalName(name, attributes), source: 'custom' };
+    return { op: 'db', description: name, source: 'custom' };
   }
 
   // Use DB statement (Ex "SELECT * FROM table") if possible as description.
@@ -151,7 +161,7 @@ export function descriptionForHttpMethod(
   const { urlPath, url, query, fragment, hasRoute } = getSanitizedUrl(attributes, kind);
 
   if (!urlPath) {
-    return { op: opParts.join('.'), description: getOriginalName(name, attributes), source: 'custom' };
+    return { ...getUserUpdatedNameAndSource(name, attributes), op: opParts.join('.') };
   }
 
   const graphqlOperationsAttribute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION];
@@ -161,12 +171,12 @@ export function descriptionForHttpMethod(
 
   // When the http span has a graphql operation, append it to the description
   // We add these in the graphqlIntegration
-  const description = graphqlOperationsAttribute
+  const inferredDescription = graphqlOperationsAttribute
     ? `${baseDescription} (${getGraphqlOperationNamesFromAttribute(graphqlOperationsAttribute)})`
     : baseDescription;
 
   // If `httpPath` is a root path, then we can categorize the transaction source as route.
-  const source: TransactionSource = hasRoute || urlPath === '/' ? 'route' : 'url';
+  const inferredSource: TransactionSource = hasRoute || urlPath === '/' ? 'route' : 'url';
 
   const data: Record<string, string> = {};
 
@@ -190,15 +200,22 @@ export function descriptionForHttpMethod(
   const origin = attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] || 'manual';
   const isManualSpan = !`${origin}`.startsWith('auto');
 
-  // If users (or in very rare occasions we) set the source to custom, we don't overwrite it
+  // If users (or in very rare occasions we) set the source to custom, we don't overwrite the name
   const alreadyHasCustomSource = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom';
 
-  const useInferredDescription = !alreadyHasCustomSource && (isClientOrServerKind || !isManualSpan);
+  const useInferredDescription =
+    !alreadyHasCustomSource &&
+    attributes['_sentry_span_name_set_by_user'] == null &&
+    (isClientOrServerKind || !isManualSpan);
+
+  const { description, source } = useInferredDescription
+    ? { description: inferredDescription, source: inferredSource }
+    : getUserUpdatedNameAndSource(name, attributes);
 
   return {
     op: opParts.join('.'),
-    description: useInferredDescription ? description : getOriginalName(name, attributes),
-    source: useInferredDescription ? source : 'custom',
+    description,
+    source,
     data,
   };
 }
@@ -265,15 +282,32 @@ export function getSanitizedUrl(
 }
 
 /**
- * Because Otel decided to mutate span names via `span.updateName`, the only way to ensure
- * that a user-set span name is preserved is to store it as a tmp attribute on the span.
+ * Because Otel instrumentation sometimes mutates span names via `span.updateName`, the only way
+ * to ensure that a user-set span name is preserved is to store it as a tmp attribute on the span.
  * We delete this attribute once we're done with it when preparing the event envelope.
+ *
+ * This temp attribute always takes precedence over the original name.
+ *
+ * We also need to take care of setting the correct source. Users can always update the source
+ * after updating the name, so we need to respect that.
+ *
  * @internal exported only for testing
  */
-export 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;
+export function getUserUpdatedNameAndSource(
+  originalName: string,
+  attributes: Attributes,
+  fallbackSource: TransactionSource = 'custom',
+): {
+  description: string;
+  source: TransactionSource;
+} {
+  const source = (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionSource) || fallbackSource;
+
+  if (attributes['_sentry_span_name_set_by_user'] && typeof attributes['_sentry_span_name_set_by_user'] === 'string')
+    return {
+      description: attributes['_sentry_span_name_set_by_user'],
+      source,
+    };
+
+  return { description: originalName, source };
 }