feat(core): Add server.address to browser http.client spans (#11634)

mydea · web-flow · commit daf2edf9bab3 · 2024-04-17T09:57:28.000+02:00
Closes #11632
diff --git a/.size-limit.js b/.size-limit.js
@@ -208,7 +208,7 @@ module.exports = [
       'tls',
     ],
     gzip: true,
-    limit: '150 KB',
+    limit: '160 KB',
   },
 ];
 
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,79 @@
+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);
+
+  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
@@ -36,6 +36,13 @@ sentryTest('should create spans for fetch requests', async ({ getLocalTestPath,
       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,79 @@
+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);
+
+  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
@@ -24,6 +24,13 @@ sentryTest('should create spans for XHR requests', async ({ getLocalTestPath, pa
       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
@@ -76,6 +76,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/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts
@@ -4,6 +4,7 @@ import {
   addXhrInstrumentationHandler,
 } from '@sentry-internal/browser-utils';
 import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SentryNonRecordingSpan,
   getActiveSpan,
@@ -26,6 +27,7 @@ import {
   browserPerformanceTimeOrigin,
   dynamicSamplingContextToSentryBaggageHeader,
   generateSentryTraceHeader,
+  parseUrl,
   stringMatchesSomePattern,
 } from '@sentry/utils';
 import { WINDOW } from '../helpers';
@@ -115,6 +117,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);
       }
@@ -310,17 +324,22 @@ export function xhrCallback(
 
   const hasParent = !!getActiveSpan();
 
+  const fullUrl = getFullURL(sentryXhrData.url);
+  const host = fullUrl ? parseUrl(fullUrl).host : undefined;
+
   const span =
     shouldCreateSpanResult && hasParent
       ? startInactiveSpan({
           name: `${sentryXhrData.method} ${sentryXhrData.url}`,
           attributes: {
             type: 'xhr',
             'http.method': sentryXhrData.method,
+            'http.url': fullUrl,
             url: sentryXhrData.url,
+            'server.address': host,
             [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser',
+            [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client',
           },
-          op: 'http.client',
         })
       : new SentryNonRecordingSpan();
 
@@ -381,3 +400,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/core/src/fetch.ts b/packages/core/src/fetch.ts
diff --git a/packages/nextjs/test/integration/test/client/tracingFetch.test.ts b/packages/nextjs/test/integration/test/client/tracingFetch.test.ts

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`	`+);`