fix(otel): Use HTTP_URL attribute for client requests (#8539)

Turns out, `HTTP_TARGET` is always the relative path, even for outgoing
requests, which omits the host. So we handle this specifically now here.

While at it (and as I'm working a bit in this codebase), I also renamed
the files from kebap-case to camelCase, to align with the rest of the
codebase better - unless there was a specific reason to use that here
@AbhiPrasad ?

Closes #8535

packages/opentelemetry-node/src/spanprocessor.ts

@@ -7,9 +7,9 @@ import type { DynamicSamplingContext, Span as SentrySpan, TraceparentData, Trans
 import { isString, logger } from '@sentry/utils';
 
 import { SENTRY_DYNAMIC_SAMPLING_CONTEXT_KEY, SENTRY_TRACE_PARENT_CONTEXT_KEY } from './constants';
-import { isSentryRequestSpan } from './utils/is-sentry-request';
-import { mapOtelStatus } from './utils/map-otel-status';
-import { parseSpanDescription } from './utils/parse-otel-span-description';
+import { isSentryRequestSpan } from './utils/isSentryRequest';
+import { mapOtelStatus } from './utils/mapOtelStatus';
+import { parseSpanDescription } from './utils/parseOtelSpanDescription';
 
 export const SENTRY_SPAN_PROCESSOR_MAP: Map<SentrySpan['spanId'], SentrySpan> = new Map<
   SentrySpan['spanId'],
@@ -207,6 +207,8 @@ function getTraceData(otelSpan: OtelSpan, parentContext: Context): Partial<Trans
 function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): void {
   const { attributes, kind } = otelSpan;
 
+  const { op, description, data } = parseSpanDescription(otelSpan);
+
   sentrySpan.setStatus(mapOtelStatus(otelSpan));
   sentrySpan.setData('otel.kind', SpanKind[kind]);
 
@@ -215,20 +217,34 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi
     sentrySpan.setData(prop, value);
   });
 
-  const { op, description } = parseSpanDescription(otelSpan);
+  if (data) {
+    Object.keys(data).forEach(prop => {
+      const value = data[prop];
+      sentrySpan.setData(prop, value);
+    });
+  }
+
   sentrySpan.op = op;
   sentrySpan.description = description;
 }
 
 function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelSpan): void {
+  const { op, description, source, data } = parseSpanDescription(otelSpan);
+
   transaction.setContext('otel', {
     attributes: otelSpan.attributes,
     resource: otelSpan.resource.attributes,
   });
 
+  if (data) {
+    Object.keys(data).forEach(prop => {
+      const value = data[prop];
+      transaction.setData(prop, value);
+    });
+  }
+
   transaction.setStatus(mapOtelStatus(otelSpan));
 
-  const { op, description, source } = parseSpanDescription(otelSpan);
   transaction.op = op;
   transaction.setName(description, source);
 }

packages/opentelemetry-node/src/utils/parse-otel-span-description.ts → packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts

@@ -1,13 +1,15 @@
-import type { AttributeValue } from '@opentelemetry/api';
+import type { Attributes, AttributeValue } from '@opentelemetry/api';
 import { SpanKind } from '@opentelemetry/api';
 import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 import type { TransactionSource } from '@sentry/types';
+import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';
 
 interface SpanDescription {
   op: string | undefined;
   description: string;
   source: TransactionSource;
+  data?: Record<string, string>;
 }
 
 /**
@@ -87,21 +89,77 @@ function descriptionForHttpMethod(otelSpan: OtelSpan, httpMethod: AttributeValue
       break;
   }
 
-  const httpTarget = attributes[SemanticAttributes.HTTP_TARGET];
   const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE];
+  const { urlPath, url, query, fragment } = getSanitizedUrl(attributes, kind);
 
-  // Ex. /api/users
-  const httpPath = httpRoute || httpTarget;
-
-  if (!httpPath) {
+  if (!urlPath) {
     return { op: opParts.join('.'), description: name, source: 'custom' };
   }
 
   // Ex. description="GET /api/users".
-  const description = `${httpMethod} ${httpPath}`;
+  const description = `${httpMethod} ${urlPath}`;
 
   // If `httpPath` is a root path, then we can categorize the transaction source as route.
-  const source: TransactionSource = httpRoute || httpPath === '/' ? 'route' : 'url';
+  const source: TransactionSource = httpRoute || urlPath === '/' ? 'route' : 'url';
+
+  const data: Record<string, string> = {};
+
+  if (url) {
+    data.url = url;
+  }
+  if (query) {
+    data['http.query'] = query;
+  }
+  if (fragment) {
+    data['http.fragment'] = fragment;
+  }
+
+  return {
+    op: opParts.join('.'),
+    description,
+    source,
+    data,
+  };
+}
+
+/** Exported for tests only */
+export function getSanitizedUrl(
+  attributes: Attributes,
+  kind: SpanKind,
+): {
+  url: string | undefined;
+  urlPath: string | undefined;
+  query: string | undefined;
+  fragment: string | undefined;
+} {
+  // This is the relative path of the URL, e.g. /sub
+  const httpTarget = attributes[SemanticAttributes.HTTP_TARGET];
+  // This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar
+  const httpUrl = attributes[SemanticAttributes.HTTP_URL];
+  // This is the normalized route name - may not always be available!
+  const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE];
+
+  const parsedUrl = typeof httpUrl === 'string' ? parseUrl(httpUrl) : undefined;
+  const url = parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined;
+  const query = parsedUrl && parsedUrl.search ? parsedUrl.search : undefined;
+  const fragment = parsedUrl && parsedUrl.hash ? parsedUrl.hash : undefined;
+
+  if (typeof httpRoute === 'string') {
+    return { urlPath: httpRoute, url, query, fragment };
+  }
+
+  if (kind === SpanKind.SERVER && typeof httpTarget === 'string') {
+    return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment };
+  }
+
+  if (parsedUrl) {
+    return { urlPath: url, url, query, fragment };
+  }
+
+  // fall back to target even for client spans, if no URL is present
+  if (typeof httpTarget === 'string') {
+    return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment };
+  }
 
-  return { op: opParts.join('.'), description, source };
+  return { urlPath: undefined, url, query, fragment };
 }

packages/opentelemetry-node/test/spanprocessor.test.ts

@@ -434,10 +434,19 @@ describe('SentrySpanProcessor', () => {
           child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET');
           child.setAttribute(SemanticAttributes.HTTP_ROUTE, '/my/route/{id}');
           child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123');
+          child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123');
 
           child.end();
 
           expect(sentrySpan?.description).toBe('GET /my/route/{id}');
+          expect(sentrySpan?.data).toEqual({
+            'http.method': 'GET',
+            'http.route': '/my/route/{id}',
+            'http.target': '/my/route/123',
+            'http.url': 'http://example.com/my/route/123',
+            'otel.kind': 'INTERNAL',
+            url: 'http://example.com/my/route/123',
+          });
 
           parentOtelSpan.end();
         });
@@ -453,10 +462,47 @@ describe('SentrySpanProcessor', () => {
 
           child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET');
           child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123');
+          child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123');
 
           child.end();
 
-          expect(sentrySpan?.description).toBe('GET /my/route/123');
+          expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123');
+          expect(sentrySpan?.data).toEqual({
+            'http.method': 'GET',
+            'http.target': '/my/route/123',
+            'http.url': 'http://example.com/my/route/123',
+            'otel.kind': 'INTERNAL',
+            url: 'http://example.com/my/route/123',
+          });
+
+          parentOtelSpan.end();
+        });
+      });
+    });
+
+    it('Adds query & hash data based on HTTP_URL', async () => {
+      const tracer = provider.getTracer('default');
+
+      tracer.startActiveSpan('GET /users', parentOtelSpan => {
+        tracer.startActiveSpan('HTTP GET', child => {
+          const sentrySpan = getSpanForOtelSpan(child);
+
+          child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET');
+          child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123');
+          child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123?what=123#myHash');
+
+          child.end();
+
+          expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123');
+          expect(sentrySpan?.data).toEqual({
+            'http.method': 'GET',
+            'http.target': '/my/route/123',
+            'http.url': 'http://example.com/my/route/123?what=123#myHash',
+            'otel.kind': 'INTERNAL',
+            url: 'http://example.com/my/route/123',
+            'http.query': '?what=123',
+            'http.fragment': '#myHash',
+          });
 
           parentOtelSpan.end();
         });

packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts

@@ -0,0 +1,144 @@
+import { SpanKind } from '@opentelemetry/api';
+import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
+
+import { getSanitizedUrl } from '../../src/utils/parseOtelSpanDescription';
+
+describe('getSanitizedUrl', () => {
+  it.each([
+    [
+      'works without attributes',
+      {},
+      SpanKind.CLIENT,
+      {
+        urlPath: undefined,
+        url: undefined,
+        fragment: undefined,
+        query: undefined,
+      },
+    ],
+    [
+      'uses url without query for client request',
+      {
+        [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
+        [SemanticAttributes.HTTP_METHOD]: 'GET',
+        [SemanticAttributes.HTTP_TARGET]: '/?what=true',
+        [SemanticAttributes.HTTP_HOST]: 'example.com:80',
+        [SemanticAttributes.HTTP_STATUS_CODE]: 200,
+      },
+      SpanKind.CLIENT,
+      {
+        urlPath: 'http://example.com/',
+        url: 'http://example.com/',
+        fragment: undefined,
+        query: '?what=true',
+      },
+    ],
+    [
+      'uses url without hash for client request',
+      {
+        [SemanticAttributes.HTTP_URL]: 'http://example.com/sub#hash',
+        [SemanticAttributes.HTTP_METHOD]: 'GET',
+        [SemanticAttributes.HTTP_TARGET]: '/sub#hash',
+        [SemanticAttributes.HTTP_HOST]: 'example.com:80',
+        [SemanticAttributes.HTTP_STATUS_CODE]: 200,
+      },
+      SpanKind.CLIENT,
+      {
+        urlPath: 'http://example.com/sub',
+        url: 'http://example.com/sub',
+        fragment: '#hash',
+        query: undefined,
+      },
+    ],
+    [
+      'uses route if available for client request',
+      {
+        [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
+        [SemanticAttributes.HTTP_METHOD]: 'GET',
+        [SemanticAttributes.HTTP_TARGET]: '/?what=true',
+        [SemanticAttributes.HTTP_ROUTE]: '/my-route',
+        [SemanticAttributes.HTTP_HOST]: 'example.com:80',
+        [SemanticAttributes.HTTP_STATUS_CODE]: 200,
+      },
+      SpanKind.CLIENT,
+      {
+        urlPath: '/my-route',
+        url: 'http://example.com/',
+        fragment: undefined,
+        query: '?what=true',
+      },
+    ],
+    [
+      'falls back to target for client request if url not available',
+      {
+        [SemanticAttributes.HTTP_METHOD]: 'GET',
+        [SemanticAttributes.HTTP_TARGET]: '/?what=true',
+        [SemanticAttributes.HTTP_HOST]: 'example.com:80',
+        [SemanticAttributes.HTTP_STATUS_CODE]: 200,
+      },
+      SpanKind.CLIENT,
+      {
+        urlPath: '/',
+        url: undefined,
+        fragment: undefined,
+        query: undefined,
+      },
+    ],
+    [
+      'uses target without query for server request',
+      {
+        [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
+        [SemanticAttributes.HTTP_METHOD]: 'GET',
+        [SemanticAttributes.HTTP_TARGET]: '/?what=true',
+        [SemanticAttributes.HTTP_HOST]: 'example.com:80',
+        [SemanticAttributes.HTTP_STATUS_CODE]: 200,
+      },
+      SpanKind.SERVER,
+      {
+        urlPath: '/',
+        url: 'http://example.com/',
+        fragment: undefined,
+        query: '?what=true',
+      },
+    ],
+    [
+      'uses target without hash for server request',
+      {
+        [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
+        [SemanticAttributes.HTTP_METHOD]: 'GET',
+        [SemanticAttributes.HTTP_TARGET]: '/sub#hash',
+        [SemanticAttributes.HTTP_HOST]: 'example.com:80',
+        [SemanticAttributes.HTTP_STATUS_CODE]: 200,
+      },
+      SpanKind.SERVER,
+      {
+        urlPath: '/sub',
+        url: 'http://example.com/',
+        fragment: undefined,
+        query: '?what=true',
+      },
+    ],
+    [
+      'uses route for server request if available',
+      {
+        [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true',
+        [SemanticAttributes.HTTP_METHOD]: 'GET',
+        [SemanticAttributes.HTTP_TARGET]: '/?what=true',
+        [SemanticAttributes.HTTP_ROUTE]: '/my-route',
+        [SemanticAttributes.HTTP_HOST]: 'example.com:80',
+        [SemanticAttributes.HTTP_STATUS_CODE]: 200,
+      },
+      SpanKind.SERVER,
+      {
+        urlPath: '/my-route',
+        url: 'http://example.com/',
+        fragment: undefined,
+        query: '?what=true',
+      },
+    ],
+  ])('%s', (_, attributes, kind, expected) => {
+    const actual = getSanitizedUrl(attributes, kind);
+
+    expect(actual).toEqual(expected);
+  });
+});