feat(core/v7): Add server.address to browser http.client spans (#11663)

AbhiPrasad · mydea · web-flow · commit b9ef1161f984 · 2024-04-18T09:09:38.000-04:00
Backport of #11634 to v7 --------- Co-authored-by: Francesco Novy <francesco.novy@sentry.io>
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/fetch-relative-url/init.js b/dev-packages/browser-integration-tests/suites/tracing/request/fetch-relative-url/init.js
@@ -0,0 +1,9 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+});
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/fetch-relative-url/subject.js b/dev-packages/browser-integration-tests/suites/tracing/request/fetch-relative-url/subject.js
@@ -0,0 +1,3 @@
+fetch('/test-req/0').then(
+  fetch('/test-req/1', { headers: { 'X-Test-Header': 'existing-header' } }).then(fetch('/test-req/2')),
+);
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/fetch-relative-url/test.ts b/dev-packages/browser-integration-tests/suites/tracing/request/fetch-relative-url/test.ts
@@ -0,0 +1,80 @@
+import { expect } from '@playwright/test';
+
+import { TEST_HOST, sentryTest } from '../../../../utils/fixtures';
+import {
+  envelopeRequestParser,
+  shouldSkipTracingTest,
+  waitForTransactionRequestOnUrl,
+} from '../../../../utils/helpers';
+
+sentryTest('should create spans for fetch requests', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+  const req = await waitForTransactionRequestOnUrl(page, url);
+  const tracingEvent = envelopeRequestParser(req);
+
+  // eslint-disable-next-line deprecation/deprecation
+  const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client');
+
+  expect(requestSpans).toHaveLength(3);
+
+  requestSpans?.forEach((span, index) =>
+    expect(span).toMatchObject({
+      description: `GET /test-req/${index}`,
+      parent_span_id: tracingEvent.contexts?.trace?.span_id,
+      span_id: expect.any(String),
+      start_timestamp: expect.any(Number),
+      timestamp: expect.any(Number),
+      trace_id: tracingEvent.contexts?.trace?.trace_id,
+      data: {
+        'http.method': 'GET',
+        'http.url': `${TEST_HOST}/test-req/${index}`,
+        url: `/test-req/${index}`,
+        'server.address': 'sentry-test.io',
+        type: 'fetch',
+      },
+    }),
+  );
+});
+
+sentryTest('should attach `sentry-trace` header to fetch requests', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  const requests = (
+    await Promise.all([
+      page.goto(url),
+      Promise.all([0, 1, 2].map(idx => page.waitForRequest(`${TEST_HOST}/test-req/${idx}`))),
+    ])
+  )[1];
+
+  expect(requests).toHaveLength(3);
+
+  const request1 = requests[0];
+  const requestHeaders1 = request1.headers();
+  expect(requestHeaders1).toMatchObject({
+    'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/),
+    baggage: expect.any(String),
+  });
+
+  const request2 = requests[1];
+  const requestHeaders2 = request2.headers();
+  expect(requestHeaders2).toMatchObject({
+    'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/),
+    baggage: expect.any(String),
+    'x-test-header': 'existing-header',
+  });
+
+  const request3 = requests[2];
+  const requestHeaders3 = request3.headers();
+  expect(requestHeaders3).toMatchObject({
+    'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/),
+    baggage: expect.any(String),
+  });
+});
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
@@ -37,6 +37,13 @@ sentryTest('should create spans for multiple fetch requests', async ({ getLocalT
       start_timestamp: expect.any(Number),
       timestamp: expect.any(Number),
       trace_id: tracingEvent.contexts?.trace?.trace_id,
+      data: {
+        'http.method': 'GET',
+        'http.url': `http://example.com/${index}`,
+        url: `http://example.com/${index}`,
+        'server.address': 'example.com',
+        type: 'fetch',
+      },
     }),
   );
 });
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/xhr-relative-url/init.js b/dev-packages/browser-integration-tests/suites/tracing/request/xhr-relative-url/init.js
@@ -0,0 +1,9 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+});
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/xhr-relative-url/subject.js b/dev-packages/browser-integration-tests/suites/tracing/request/xhr-relative-url/subject.js
@@ -0,0 +1,12 @@
+const xhr_1 = new XMLHttpRequest();
+xhr_1.open('GET', '/test-req/0');
+xhr_1.send();
+
+const xhr_2 = new XMLHttpRequest();
+xhr_2.open('GET', '/test-req/1');
+xhr_2.setRequestHeader('X-Test-Header', 'existing-header');
+xhr_2.send();
+
+const xhr_3 = new XMLHttpRequest();
+xhr_3.open('GET', '/test-req/2');
+xhr_3.send();
diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/xhr-relative-url/test.ts b/dev-packages/browser-integration-tests/suites/tracing/request/xhr-relative-url/test.ts
@@ -0,0 +1,80 @@
+import { expect } from '@playwright/test';
+
+import { TEST_HOST, sentryTest } from '../../../../utils/fixtures';
+import {
+  envelopeRequestParser,
+  shouldSkipTracingTest,
+  waitForTransactionRequestOnUrl,
+} from '../../../../utils/helpers';
+
+sentryTest('should create spans for xhr requests', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+  const req = await waitForTransactionRequestOnUrl(page, url);
+  const tracingEvent = envelopeRequestParser(req);
+
+  // eslint-disable-next-line deprecation/deprecation
+  const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client');
+
+  expect(requestSpans).toHaveLength(3);
+
+  requestSpans?.forEach((span, index) =>
+    expect(span).toMatchObject({
+      description: `GET /test-req/${index}`,
+      parent_span_id: tracingEvent.contexts?.trace?.span_id,
+      span_id: expect.any(String),
+      start_timestamp: expect.any(Number),
+      timestamp: expect.any(Number),
+      trace_id: tracingEvent.contexts?.trace?.trace_id,
+      data: {
+        'http.method': 'GET',
+        'http.url': `${TEST_HOST}/test-req/${index}`,
+        url: `/test-req/${index}`,
+        'server.address': 'sentry-test.io',
+        type: 'xhr',
+      },
+    }),
+  );
+});
+
+sentryTest('should attach `sentry-trace` header to xhr requests', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  const requests = (
+    await Promise.all([
+      page.goto(url),
+      Promise.all([0, 1, 2].map(idx => page.waitForRequest(`${TEST_HOST}/test-req/${idx}`))),
+    ])
+  )[1];
+
+  expect(requests).toHaveLength(3);
+
+  const request1 = requests[0];
+  const requestHeaders1 = request1.headers();
+  expect(requestHeaders1).toMatchObject({
+    'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/),
+    baggage: expect.any(String),
+  });
+
+  const request2 = requests[1];
+  const requestHeaders2 = request2.headers();
+  expect(requestHeaders2).toMatchObject({
+    'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/),
+    baggage: expect.any(String),
+    'x-test-header': 'existing-header',
+  });
+
+  const request3 = requests[2];
+  const requestHeaders3 = request3.headers();
+  expect(requestHeaders3).toMatchObject({
+    'sentry-trace': expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/),
+    baggage: expect.any(String),
+  });
+});
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
@@ -25,6 +25,13 @@ sentryTest('should create spans for multiple XHR requests', async ({ getLocalTes
       start_timestamp: expect.any(Number),
       timestamp: expect.any(Number),
       trace_id: eventData.contexts?.trace?.trace_id,
+      data: {
+        'http.method': 'GET',
+        'http.url': `http://example.com/${index}`,
+        url: `http://example.com/${index}`,
+        'server.address': 'example.com',
+        type: 'xhr',
+      },
     }),
   );
 });
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/middleware.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/middleware.test.ts
@@ -68,6 +68,8 @@ test('Should trace outgoing fetch requests inside middleware and create breadcru
           'http.response.status_code': 200,
           type: 'fetch',
           url: 'http://localhost:3030/',
+          'http.url': 'http://localhost:3030/',
+          'server.address': 'localhost:3030',
           'sentry.op': 'http.client',
           'sentry.origin': 'auto.http.wintercg_fetch',
         },
diff --git a/packages/nextjs/test/integration/test/client/tracingFetch.test.ts b/packages/nextjs/test/integration/test/client/tracingFetch.test.ts
@@ -33,6 +33,8 @@ test('should correctly instrument `fetch` for performance tracing', async ({ pag
         data: {
           'http.method': 'GET',
           url: 'http://example.com',
+          'http.url': 'http://example.com/',
+          'server.address': 'example.com',
           type: 'fetch',
           'http.response_content_length': expect.any(Number),
           'http.response.status_code': 200,
diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts
@@ -21,11 +21,13 @@ import {
   browserPerformanceTimeOrigin,
   dynamicSamplingContextToSentryBaggageHeader,
   generateSentryTraceHeader,
+  parseUrl,
   stringMatchesSomePattern,
 } from '@sentry/utils';
 
 import { instrumentFetchRequest } from '../common/fetch';
 import { addPerformanceInstrumentationHandler } from './instrument';
+import { WINDOW } from './types';
 
 export const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\/(?!\/)/];
 
@@ -119,6 +121,18 @@ export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumenta
   if (traceFetch) {
     addFetchInstrumentationHandler(handlerData => {
       const createdSpan = instrumentFetchRequest(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans);
+      // We cannot use `window.location` in the generic fetch instrumentation,
+      // but we need it for reliable `server.address` attribute.
+      // so we extend this in here
+      if (createdSpan) {
+        const fullUrl = getFullURL(handlerData.fetchData.url);
+        const host = fullUrl ? parseUrl(fullUrl).host : undefined;
+        createdSpan.setAttributes({
+          'http.url': fullUrl,
+          'server.address': host,
+        });
+      }
+
       if (enableHTTPTimings && createdSpan) {
         addHTTPTimings(createdSpan);
       }
@@ -279,14 +293,19 @@ export function xhrCallback(
   const scope = getCurrentScope();
   const isolationScope = getIsolationScope();
 
+  const fullUrl = getFullURL(sentryXhrData.url);
+  const host = fullUrl ? parseUrl(fullUrl).host : undefined;
+
   const span = shouldCreateSpanResult
     ? startInactiveSpan({
         name: `${sentryXhrData.method} ${sentryXhrData.url}`,
         onlyIfParent: true,
         attributes: {
           type: 'xhr',
           'http.method': sentryXhrData.method,
+          'http.url': fullUrl,
           url: sentryXhrData.url,
+          'server.address': host,
           [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser',
         },
         op: 'http.client',
@@ -338,3 +357,14 @@ function setHeaderOnXhr(
     // Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED.
   }
 }
+
+function getFullURL(url: string): string | undefined {
+  try {
+    // By adding a base URL to new URL(), this will also work for relative urls
+    // If `url` is a full URL, the base URL is ignored anyhow
+    const parsed = new URL(url, WINDOW.location.origin);
+    return parsed.href;
+  } catch {
+    return undefined;
+  }
+}
diff --git a/packages/tracing-internal/src/common/fetch.ts b/packages/tracing-internal/src/common/fetch.ts
@@ -16,6 +16,7 @@ import {
   dynamicSamplingContextToSentryBaggageHeader,
   generateSentryTraceHeader,
   isInstanceOf,
+  parseUrl,
 } from '@sentry/utils';
 
 type PolymorphicRequestHeaders =
@@ -53,23 +54,7 @@ export function instrumentFetchRequest(
 
     const span = spans[spanId];
     if (span) {
-      if (handlerData.response) {
-        setHttpStatus(span, handlerData.response.status);
-
-        const contentLength =
-          handlerData.response && handlerData.response.headers && handlerData.response.headers.get('content-length');
-
-        if (contentLength) {
-          const contentLengthNum = parseInt(contentLength);
-          if (contentLengthNum > 0) {
-            span.setAttribute('http.response_content_length', contentLengthNum);
-          }
-        }
-      } else if (handlerData.error) {
-        span.setStatus('internal_error');
-      }
-      span.end();
-
+      endSpan(span, handlerData);
       // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
       delete spans[spanId];
     }
@@ -81,6 +66,9 @@ export function instrumentFetchRequest(
 
   const { method, url } = handlerData.fetchData;
 
+  const fullUrl = getFullURL(url);
+  const host = fullUrl ? parseUrl(fullUrl).host : undefined;
+
   const span = shouldCreateSpanResult
     ? startInactiveSpan({
         name: `${method} ${url}`,
@@ -89,6 +77,8 @@ export function instrumentFetchRequest(
           url,
           type: 'fetch',
           'http.method': method,
+          'http.url': fullUrl,
+          'server.address': host,
           [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: spanOrigin,
         },
         op: 'http.client',
@@ -198,3 +188,31 @@ export function addTracingHeadersToFetchRequest(
     };
   }
 }
+
+function getFullURL(url: string): string | undefined {
+  try {
+    const parsed = new URL(url);
+    return parsed.href;
+  } catch {
+    return undefined;
+  }
+}
+
+function endSpan(span: Span, handlerData: HandlerDataFetch): void {
+  if (handlerData.response) {
+    setHttpStatus(span, handlerData.response.status);
+
+    const contentLength =
+      handlerData.response && handlerData.response.headers && handlerData.response.headers.get('content-length');
+
+    if (contentLength) {
+      const contentLengthNum = parseInt(contentLength);
+      if (contentLengthNum > 0) {
+        span.setAttribute('http.response_content_length', contentLengthNum);
+      }
+    }
+  } else if (handlerData.error) {
+    span.setStatus('internal_error');
+  }
+  span.end();
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+fetch('/test-req/0').then(`
	`2`	`+ fetch('/test-req/1', { headers: { 'X-Test-Header': 'existing-header' } }).then(fetch('/test-req/2')),`
	`3`	`+);`