From 16b21ce2abbf94cd45b8b3c1981da4f24a79fbfc Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 23 Oct 2024 09:52:04 +0200 Subject: [PATCH 1/5] feat(browser): Add `http.response_delivery_type` attribute to resource spans --- .../src/metrics/browserMetrics.ts | 5 ++++ .../test/browser/browserMetrics.test.ts | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index 3b98558f2728..17300ee45484 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -513,6 +513,7 @@ export interface ResourceEntry extends Record { encodedBodySize?: number; decodedBodySize?: number; renderBlockingStatus?: string; + deliveryType?: string; } /** Create resource-related spans */ @@ -539,6 +540,10 @@ export function _addResourceSpans( setResourceEntrySizeData(attributes, entry, 'encodedBodySize', 'http.response_content_length'); setResourceEntrySizeData(attributes, entry, 'decodedBodySize', 'http.decoded_response_content_length'); + if (entry.deliveryType != null) { + attributes['http.response_delivery_type'] = entry.deliveryType || 'default'; + } + if ('renderBlockingStatus' in entry) { attributes['resource.render_blocking_status'] = entry.renderBlockingStatus; } diff --git a/packages/browser-utils/test/browser/browserMetrics.test.ts b/packages/browser-utils/test/browser/browserMetrics.test.ts index 8e67df1645d0..b57ce61fe063 100644 --- a/packages/browser-utils/test/browser/browserMetrics.test.ts +++ b/packages/browser-utils/test/browser/browserMetrics.test.ts @@ -340,6 +340,32 @@ describe('_addResourceSpans', () => { }); }); +// resource delivery types: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/deliveryType +// i.e. better but not yet widely supported way to check for browser cache hit +it.each(['', 'cache', 'navigational-prefetch'])( + 'attaches delivery type to resource spans if available', + deliveryType => { + const spans: Span[] = []; + + getClient()?.on('spanEnd', span => { + spans.push(span); + }); + + const entry: ResourceEntry = { + initiatorType: 'css', + transferSize: 0, + encodedBodySize: 0, + decodedBodySize: 0, + deliveryType, + }; + + _addResourceSpans(span, entry, resourceEntryName, 100, 23, 345); + + expect(spans).toHaveLength(1); + expect(spanToJSON(spans[0]!)).toEqual(expect.objectContaining({ 'http.response_delivery_type': deliveryType })); + }, +); + const setGlobalLocation = (location: Location) => { // @ts-expect-error need to delete this in order to set to new value delete WINDOW.location; From b176f1d4d879ccb5665173e2b71f774face3e4f1 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 23 Oct 2024 10:49:04 +0200 Subject: [PATCH 2/5] add integration test for empty string attribute --- .../startSpan/attributes/subject.js | 11 ++++++++++ .../public-api/startSpan/attributes/test.ts | 21 +++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/test.ts diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/subject.js b/dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/subject.js new file mode 100644 index 000000000000..d91cfe341de9 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/subject.js @@ -0,0 +1,11 @@ +async function run() { + Sentry.startSpan({ name: 'parent_span' }, () => { + Sentry.startSpan({ name: 'child_span', attributes: { someAttribute: '' } }, () => { + // whatever a user would do here + }); + }); +} + +(async () => { + await run(); +})(); diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/test.ts b/dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/test.ts new file mode 100644 index 000000000000..a5e3e651467a --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/startSpan/attributes/test.ts @@ -0,0 +1,21 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { + envelopeRequestParser, + shouldSkipTracingTest, + waitForTransactionRequestOnUrl, +} from '../../../../utils/helpers'; + +sentryTest('sends an empty string attribute', async ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + const req = await waitForTransactionRequestOnUrl(page, url); + const transaction = envelopeRequestParser(req); + + const childSpan = transaction.spans?.[0]; + expect(childSpan?.data?.someAttribute).toBe(''); +}); From 962090888c417148020c508fe369aff2552d28b1 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 23 Oct 2024 11:11:02 +0200 Subject: [PATCH 3/5] fix tests --- .../test/browser/browserMetrics.test.ts | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/packages/browser-utils/test/browser/browserMetrics.test.ts b/packages/browser-utils/test/browser/browserMetrics.test.ts index b57ce61fe063..dd0656cc92ea 100644 --- a/packages/browser-utils/test/browser/browserMetrics.test.ts +++ b/packages/browser-utils/test/browser/browserMetrics.test.ts @@ -338,13 +338,34 @@ describe('_addResourceSpans', () => { }), ); }); -}); -// resource delivery types: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/deliveryType -// i.e. better but not yet widely supported way to check for browser cache hit -it.each(['', 'cache', 'navigational-prefetch'])( - 'attaches delivery type to resource spans if available', - deliveryType => { + // resource delivery types: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/deliveryType + // i.e. better but not yet widely supported way to check for browser cache hit + it.each(['cache', 'navigational-prefetch'])( + 'attaches delivery type ("%s") to resource spans if available', + deliveryType => { + const spans: Span[] = []; + + getClient()?.on('spanEnd', span => { + spans.push(span); + }); + + const entry: ResourceEntry = { + initiatorType: 'css', + transferSize: 0, + encodedBodySize: 0, + decodedBodySize: 0, + deliveryType, + }; + + _addResourceSpans(span, entry, resourceEntryName, 100, 23, 345); + + expect(spans).toHaveLength(1); + expect(spanToJSON(spans[0]!).data).toMatchObject({ 'http.response_delivery_type': deliveryType }); + }, + ); + + it('attaches "default" as delivery if the empty string ("") default value is set', () => { const spans: Span[] = []; getClient()?.on('spanEnd', span => { @@ -356,15 +377,15 @@ it.each(['', 'cache', 'navigational-prefetch'])( transferSize: 0, encodedBodySize: 0, decodedBodySize: 0, - deliveryType, + deliveryType: '', }; _addResourceSpans(span, entry, resourceEntryName, 100, 23, 345); expect(spans).toHaveLength(1); - expect(spanToJSON(spans[0]!)).toEqual(expect.objectContaining({ 'http.response_delivery_type': deliveryType })); - }, -); + expect(spanToJSON(spans[0]!).data).toMatchObject({ 'http.response_delivery_type': 'default' }); + }); +}); const setGlobalLocation = (location: Location) => { // @ts-expect-error need to delete this in order to set to new value From d8feca028dcb0269ea938e3aa0ba5a69223cd108 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 24 Oct 2024 09:20:16 +0200 Subject: [PATCH 4/5] add comment --- packages/browser-utils/src/metrics/browserMetrics.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index 17300ee45484..caf5cefceebe 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -541,6 +541,9 @@ export function _addResourceSpans( setResourceEntrySizeData(attributes, entry, 'decodedBodySize', 'http.decoded_response_content_length'); if (entry.deliveryType != null) { + // Falling back to 'default' as attributes with empty string are dropped by our backend. + // The empty string is the default value for the deliveryType attribute, indicating that + // the browser actually sent a request (as opposed to retrieved the response from its cache). attributes['http.response_delivery_type'] = entry.deliveryType || 'default'; } From 595f2d3598a73dc7c677e8b7f00c3ae6d4ea1273 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 31 Oct 2024 12:31:11 +0100 Subject: [PATCH 5/5] switch to empty string as default value --- .../src/metrics/browserMetrics.ts | 5 +--- .../test/browser/browserMetrics.test.ts | 23 +------------------ 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts index caf5cefceebe..5789bbe94513 100644 --- a/packages/browser-utils/src/metrics/browserMetrics.ts +++ b/packages/browser-utils/src/metrics/browserMetrics.ts @@ -541,10 +541,7 @@ export function _addResourceSpans( setResourceEntrySizeData(attributes, entry, 'decodedBodySize', 'http.decoded_response_content_length'); if (entry.deliveryType != null) { - // Falling back to 'default' as attributes with empty string are dropped by our backend. - // The empty string is the default value for the deliveryType attribute, indicating that - // the browser actually sent a request (as opposed to retrieved the response from its cache). - attributes['http.response_delivery_type'] = entry.deliveryType || 'default'; + attributes['http.response_delivery_type'] = entry.deliveryType; } if ('renderBlockingStatus' in entry) { diff --git a/packages/browser-utils/test/browser/browserMetrics.test.ts b/packages/browser-utils/test/browser/browserMetrics.test.ts index dd0656cc92ea..be78df6920fc 100644 --- a/packages/browser-utils/test/browser/browserMetrics.test.ts +++ b/packages/browser-utils/test/browser/browserMetrics.test.ts @@ -341,7 +341,7 @@ describe('_addResourceSpans', () => { // resource delivery types: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/deliveryType // i.e. better but not yet widely supported way to check for browser cache hit - it.each(['cache', 'navigational-prefetch'])( + it.each(['cache', 'navigational-prefetch', ''])( 'attaches delivery type ("%s") to resource spans if available', deliveryType => { const spans: Span[] = []; @@ -364,27 +364,6 @@ describe('_addResourceSpans', () => { expect(spanToJSON(spans[0]!).data).toMatchObject({ 'http.response_delivery_type': deliveryType }); }, ); - - it('attaches "default" as delivery if the empty string ("") default value is set', () => { - const spans: Span[] = []; - - getClient()?.on('spanEnd', span => { - spans.push(span); - }); - - const entry: ResourceEntry = { - initiatorType: 'css', - transferSize: 0, - encodedBodySize: 0, - decodedBodySize: 0, - deliveryType: '', - }; - - _addResourceSpans(span, entry, resourceEntryName, 100, 23, 345); - - expect(spans).toHaveLength(1); - expect(spanToJSON(spans[0]!).data).toMatchObject({ 'http.response_delivery_type': 'default' }); - }); }); const setGlobalLocation = (location: Location) => {