From 021c8c14d047be3dd95aba5176e2dd3869589067 Mon Sep 17 00:00:00 2001 From: Ash <0Calories@users.noreply.github.com> Date: Mon, 23 Sep 2024 13:19:30 -0400 Subject: [PATCH] ref(browser): Move navigation span descriptions into op (#13527) Makes some modifications to `browser` spans: - Moves the description of navigation related `browser` spans into the op, e.g. `browser - cache` -> `browser.cache` - Sets the description to the `performanceEntry` objects' `names`, in this context it is the URL of the page This change is being made so that these `browser` spans can be ingested and grouped. Currently, all browser spans are grouped into a singular `browser` span, despite each of the operations that these span represent doing very different things. This will improve the experience in the Spans tab of transaction summary and span summary, since we will be able to have proper groupings for `browser` spans. --- CHANGELOG.md | 2 ++ .../metrics/pageload-browser-spans/test.ts | 5 ++-- .../metrics/pageload-measure-spans/test.ts | 5 ++-- .../tests/transactions.test.ts | 24 +++++++++---------- .../src/metrics/browserMetrics.ts | 12 +++++----- 5 files changed, 26 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52187092ea55..25f4425d0eba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ ## Unreleased - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +- Moved description of `browser` spans into the operation, e.g. `browser - cache` -> `browser.cache` and set the URL as + the description ## 8.31.0 diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-browser-spans/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-browser-spans/test.ts index 2b2d5fa8bae5..90660de34ded 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-browser-spans/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-browser-spans/test.ts @@ -12,7 +12,7 @@ sentryTest('should add browser-related spans to pageload transaction', async ({ const url = await getLocalTestPath({ testDir: __dirname }); const eventData = await getFirstSentryEnvelopeRequest(page, url); - const browserSpans = eventData.spans?.filter(({ op }) => op === 'browser'); + const browserSpans = eventData.spans?.filter(({ op }) => op?.startsWith('browser')); // Spans `domContentLoadedEvent`, `connect`, `cache` and `DNS` are not // always inside `pageload` transaction. @@ -21,7 +21,8 @@ sentryTest('should add browser-related spans to pageload transaction', async ({ ['loadEvent', 'request', 'response'].forEach(eventDesc => expect(browserSpans).toContainEqual( expect.objectContaining({ - description: eventDesc, + op: `browser.${eventDesc}`, + description: page.url(), parent_span_id: eventData.contexts?.trace?.span_id, }), ), diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-measure-spans/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-measure-spans/test.ts index 9209e8ca5c32..c53993cba21d 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-measure-spans/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/pageload-measure-spans/test.ts @@ -13,14 +13,15 @@ sentryTest('should add browser-related spans to pageload transaction', async ({ const url = await getLocalTestPath({ testDir: __dirname }); const eventData = await getFirstSentryEnvelopeRequest(page, url); - const browserSpans = eventData.spans?.filter(({ op }) => op === 'browser'); + const browserSpans = eventData.spans?.filter(({ op }) => op?.startsWith('browser')); // Spans `domContentLoadedEvent`, `connect`, `cache` and `DNS` are not // always inside `pageload` transaction. expect(browserSpans?.length).toBeGreaterThanOrEqual(4); - const requestSpan = browserSpans!.find(({ description }) => description === 'request'); + const requestSpan = browserSpans!.find(({ op }) => op === 'browser.request'); expect(requestSpan).toBeDefined(); + expect(requestSpan?.description).toBe(page.url()); const measureSpan = eventData.spans?.find(({ op }) => op === 'measure'); expect(measureSpan).toBeDefined(); diff --git a/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts index fa6b40c6a49d..cc5edc1fd878 100644 --- a/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts @@ -44,10 +44,10 @@ test('Captures a pageload transaction', async ({ page }) => { expect(transactionEvent.spans).toContainEqual({ data: { 'sentry.origin': 'auto.ui.browser.metrics', - 'sentry.op': 'browser', + 'sentry.op': 'browser.domContentLoadedEvent', }, - description: 'domContentLoadedEvent', - op: 'browser', + description: page.url(), + op: 'browser.domContentLoadedEvent', parent_span_id: expect.any(String), span_id: expect.any(String), start_timestamp: expect.any(Number), @@ -58,10 +58,10 @@ test('Captures a pageload transaction', async ({ page }) => { expect(transactionEvent.spans).toContainEqual({ data: { 'sentry.origin': 'auto.ui.browser.metrics', - 'sentry.op': 'browser', + 'sentry.op': 'browser.connect', }, - description: 'connect', - op: 'browser', + description: page.url(), + op: 'browser.connect', parent_span_id: expect.any(String), span_id: expect.any(String), start_timestamp: expect.any(Number), @@ -72,10 +72,10 @@ test('Captures a pageload transaction', async ({ page }) => { expect(transactionEvent.spans).toContainEqual({ data: { 'sentry.origin': 'auto.ui.browser.metrics', - 'sentry.op': 'browser', + 'sentry.op': 'browser.request', }, - description: 'request', - op: 'browser', + description: page.url(), + op: 'browser.request', parent_span_id: expect.any(String), span_id: expect.any(String), start_timestamp: expect.any(Number), @@ -86,10 +86,10 @@ test('Captures a pageload transaction', async ({ page }) => { expect(transactionEvent.spans).toContainEqual({ data: { 'sentry.origin': 'auto.ui.browser.metrics', - 'sentry.op': 'browser', + 'sentry.op': 'browser.response', }, - description: 'response', - op: 'browser', + description: page.url(), + op: 'browser.response', parent_span_id: expect.any(String), span_id: expect.any(String), start_timestamp: expect.any(Number), diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index 92fe66a832ee..3b98558f2728 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -470,8 +470,8 @@ function _addPerformanceNavigationTiming( return; } startAndEndSpan(span, timeOrigin + msToSec(start), timeOrigin + msToSec(end), { - op: 'browser', - name: name || event, + op: `browser.${name || event}`, + name: entry.name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.browser.metrics', }, @@ -490,16 +490,16 @@ function _addRequest(span: Span, entry: Record, timeOrigin: number) // In order not to produce faulty spans, where the end timestamp is before the start timestamp, we will only collect // these spans when the responseEnd value is available. The backend (Relay) would drop the entire span if it contained faulty spans. startAndEndSpan(span, requestStartTimestamp, responseEndTimestamp, { - op: 'browser', - name: 'request', + op: 'browser.request', + name: entry.name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.browser.metrics', }, }); startAndEndSpan(span, responseStartTimestamp, responseEndTimestamp, { - op: 'browser', - name: 'response', + op: 'browser.response', + name: entry.name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.browser.metrics', },