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

mydea · AbhiPrasad · commit 670d5e95745b · 2024-04-17T16:37:20.000-04:00
Closes #11632
diff --git a/.size-limit.js b/.size-limit.js
@@ -170,6 +170,6 @@ module.exports = [
     path: 'packages/feedback/build/npm/esm/index.js',
     import: '{ Feedback }',
     gzip: true,
-    limit: '25 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
@@ -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,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
@@ -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
@@ -1,5 +1,6 @@
 /* eslint-disable max-lines */
 import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   getClient,
   getCurrentScope,
@@ -21,6 +22,7 @@ import {
   browserPerformanceTimeOrigin,
   dynamicSamplingContextToSentryBaggageHeader,
   generateSentryTraceHeader,
+  parseUrl,
   stringMatchesSomePattern,
 } from '@sentry/utils';
 
@@ -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

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