Skip to content

Commit e4bf207

Browse files
authored
fix(browser): Set user_agent.original on all spans for consistent filtering (#24216)
This patch adjusts the browser `httpContextIntegration` to: - send `user_agent.original` instead of `http.request.headers.user_agent`: The former should be used to get the browser's user agent, while the latter shall only be used for outgoing requests. - ensure `user_agent.original` is set as a span attribute on every span, and not just the segment span as previously. The other attributes are still only set on the segment span. Why? Sentry's "Filter events from legacy browsers" inbound filter stopped working for streamed spans because the wrong attribute was used and it was only applied to the segment span. This PR fixes both of these issues.
1 parent 81a3b26 commit e4bf207

11 files changed

Lines changed: 114 additions & 34 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Sentry.startSpan({ name: 'parent-span', op: 'test' }, () => {
2+
Sentry.startSpan({ name: 'child-span', op: 'test-child' }, () => {
3+
// noop
4+
});
5+
});

dev-packages/browser-integration-tests/suites/integrations/httpContext-streamed/test.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { expect } from '@playwright/test';
22
import { sentryTest } from '../../../utils/fixtures';
33
import { shouldSkipTracingTest } from '../../../utils/helpers';
4+
import { URL_FULL, USER_AGENT_ORIGINAL } from '@sentry/conventions/attributes';
45
import { getSpanOp, waitForStreamedSpans } from '../../../utils/spanUtils';
56

67
sentryTest('httpContextIntegration captures url, user-agent, and referer', async ({ getLocalTestUrl, page }) => {
@@ -15,8 +16,8 @@ sentryTest('httpContextIntegration captures url, user-agent, and referer', async
1516

1617
const pageloadSpan = spans.find(s => getSpanOp(s) === 'pageload');
1718

18-
expect(pageloadSpan!.attributes['url.full']).toEqual({ type: 'string', value: expect.any(String) });
19-
expect(pageloadSpan!.attributes['http.request.header.user_agent']).toEqual({
19+
expect(pageloadSpan!.attributes[URL_FULL]).toEqual({ type: 'string', value: expect.any(String) });
20+
expect(pageloadSpan!.attributes[USER_AGENT_ORIGINAL]).toEqual({
2021
type: 'string',
2122
value: expect.any(String),
2223
});
@@ -25,3 +26,25 @@ sentryTest('httpContextIntegration captures url, user-agent, and referer', async
2526
value: 'https://sentry.io/',
2627
});
2728
});
29+
30+
sentryTest(
31+
'httpContextIntegration only attaches the user agent to non-segment spans',
32+
async ({ getLocalTestUrl, page }) => {
33+
sentryTest.skip(shouldSkipTracingTest());
34+
const url = await getLocalTestUrl({ testDir: __dirname });
35+
36+
const spansPromise = waitForStreamedSpans(page, spans => spans.some(s => s.name === 'child-span'));
37+
38+
await page.goto(url, { referer: 'https://sentry.io/' });
39+
40+
const spans = await spansPromise;
41+
42+
const childSpan = spans.find(s => s.name === 'child-span');
43+
44+
expect(childSpan!.is_segment).toBe(false);
45+
expect(childSpan!.attributes[USER_AGENT_ORIGINAL]).toEqual({ type: 'string', value: expect.any(String) });
46+
// The document URL and referer only belong on the segment span.
47+
expect(childSpan!.attributes[URL_FULL]).toBeUndefined();
48+
expect(childSpan!.attributes['http.request.header.referer']).toBeUndefined();
49+
},
50+
);

dev-packages/browser-integration-tests/suites/public-api/startSpan/streamed/test.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
SENTRY_SDK_NAME,
1919
SENTRY_SDK_VERSION,
2020
SENTRY_TRACE_LIFECYCLE,
21+
USER_AGENT_ORIGINAL,
2122
} from '@sentry/conventions/attributes';
2223

2324
sentryTest(
@@ -107,6 +108,10 @@ sentryTest(
107108
type: 'string',
108109
value: 'stream',
109110
},
111+
[USER_AGENT_ORIGINAL]: {
112+
type: 'string',
113+
value: expect.any(String),
114+
},
110115
},
111116
end_timestamp: expect.any(Number),
112117
is_segment: false,
@@ -147,6 +152,10 @@ sentryTest(
147152
type: 'string',
148153
value: 'stream',
149154
},
155+
[USER_AGENT_ORIGINAL]: {
156+
type: 'string',
157+
value: expect.any(String),
158+
},
150159
},
151160
end_timestamp: expect.any(Number),
152161
is_segment: false,
@@ -191,6 +200,10 @@ sentryTest(
191200
type: 'string',
192201
value: 'stream',
193202
},
203+
[USER_AGENT_ORIGINAL]: {
204+
type: 'string',
205+
value: expect.any(String),
206+
},
194207
},
195208
end_timestamp: expect.any(Number),
196209
is_segment: false,
@@ -215,7 +228,7 @@ sentryTest(
215228
type: 'string',
216229
value: expect.any(String),
217230
},
218-
'http.request.header.user_agent': {
231+
[USER_AGENT_ORIGINAL]: {
219232
type: 'string',
220233
value: expect.any(String),
221234
},

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/navigation-streamed/test.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,13 @@ import {
77
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
88
SEMANTIC_ATTRIBUTE_SENTRY_SDK_INTEGRATIONS,
99
} from '@sentry/core';
10-
import { SENTRY_SEGMENT_NAME_SOURCE, SENTRY_TRACE_LIFECYCLE, URL_FULL, URL_PATH } from '@sentry/conventions/attributes';
10+
import {
11+
SENTRY_SEGMENT_NAME_SOURCE,
12+
SENTRY_TRACE_LIFECYCLE,
13+
URL_FULL,
14+
URL_PATH,
15+
USER_AGENT_ORIGINAL,
16+
} from '@sentry/conventions/attributes';
1117
import { sentryTest } from '../../../../utils/fixtures';
1218
import { shouldSkipTracingTest } from '../../../../utils/helpers';
1319
import {
@@ -87,7 +93,7 @@ sentryTest('starts a streamed navigation span on page navigation', async ({ brow
8793
type: 'string',
8894
value: expect.any(String),
8995
},
90-
'http.request.header.user_agent': {
96+
[USER_AGENT_ORIGINAL]: {
9197
type: 'string',
9298
value: expect.any(String),
9399
},

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-streamed/test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
SENTRY_TRACE_LIFECYCLE,
1717
URL_FULL,
1818
URL_PATH,
19+
USER_AGENT_ORIGINAL,
1920
} from '@sentry/conventions/attributes';
2021
import { sentryTest } from '../../../../utils/fixtures';
2122
import { shouldSkipTracingTest } from '../../../../utils/helpers';
@@ -81,7 +82,7 @@ sentryTest(
8182
type: 'string',
8283
value: expect.any(String),
8384
},
84-
'http.request.header.user_agent': {
85+
[USER_AGENT_ORIGINAL]: {
8586
type: 'string',
8687
value: expect.any(String),
8788
},

dev-packages/browser-integration-tests/suites/tracing/interactions/spans/test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
SENTRY_SDK_NAME,
1515
SENTRY_SDK_VERSION,
1616
SENTRY_TRACE_LIFECYCLE,
17+
USER_AGENT_ORIGINAL,
1718
} from '@sentry/conventions/attributes';
1819
import { sentryTest } from '../../../../utils/fixtures';
1920
import { shouldSkipTracingTest } from '../../../../utils/helpers';
@@ -61,7 +62,7 @@ sentryTest('captures streamed interaction span tree. @firefox', async ({ browser
6162
type: 'string',
6263
value: expect.any(String),
6364
},
64-
'http.request.header.user_agent': {
65+
[USER_AGENT_ORIGINAL]: {
6566
type: 'string',
6667
value: expect.any(String),
6768
},
@@ -134,6 +135,10 @@ sentryTest('captures streamed interaction span tree. @firefox', async ({ browser
134135
type: 'string',
135136
value: 'ui.interaction.click',
136137
},
138+
[USER_AGENT_ORIGINAL]: {
139+
type: 'string',
140+
value: expect.any(String),
141+
},
137142
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: {
138143
type: 'string',
139144
value: 'auto.browser.interactions',

dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/transactions.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ test('Sends a pageload span', async ({ page }) => {
4040
'url.path': { value: '/', type: 'string' },
4141
'url.template': { value: '/', type: 'string' },
4242
'react.version': { value: expect.any(String), type: 'string' },
43-
'http.request.header.user_agent': { value: expect.any(String), type: 'string' },
43+
'user_agent.original': { value: expect.any(String), type: 'string' },
4444
});
4545
expect(String(span.attributes['url.full']?.value)).toMatch(/^https?:\/\/localhost:\d+\/$/);
4646
});

dev-packages/e2e-tests/test-applications/nextjs-pages-dir/tests/transactions.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ test('Sends a pageload span', async ({ page }) => {
3434
'url.path': { value: '/', type: 'string' },
3535
'url.template': { value: '/', type: 'string' },
3636
'react.version': { value: expect.any(String), type: 'string' },
37-
'http.request.header.user_agent': { value: expect.any(String), type: 'string' },
37+
'user_agent.original': { value: expect.any(String), type: 'string' },
3838
});
3939
expect(String(span.attributes['url.full']?.value)).toMatch(/^https?:\/\/localhost:\d+\/$/);
4040
});
@@ -65,7 +65,7 @@ test('Sends a navigation span', async ({ page }) => {
6565
'url.path': { value: '/user/5', type: 'string' },
6666
'url.template': { value: '/user/[id]', type: 'string' },
6767
'react.version': { value: expect.any(String), type: 'string' },
68-
'http.request.header.user_agent': { value: expect.any(String), type: 'string' },
68+
'user_agent.original': { value: expect.any(String), type: 'string' },
6969
});
7070
expect(String(span.attributes['url.full']?.value)).toMatch(/^https?:\/\/localhost:\d+\/user\/5$/);
7171

dev-packages/e2e-tests/test-applications/nitro-3/tests/errors.test.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,16 @@ import { expect, test } from '@playwright/test';
22
import { waitForError } from '@sentry-internal/test-utils';
33

44
test('Sends an error event to Sentry', async ({ request }) => {
5+
// The thrown error is reported twice: once via the h3 tracing channel and once via Nitro's `error`
6+
// hook (which sees it wrapped in an `HTTPError`). Match on the mechanism so we deterministically
7+
// await the event under test instead of whichever arrives first.
58
const errorEventPromise = waitForError('nitro-3', event => {
6-
return !event.type && !!event.exception?.values?.some(v => v.value === 'This is a test error');
9+
return (
10+
!event.type &&
11+
!!event.exception?.values?.some(
12+
v => v.value === 'This is a test error' && v.mechanism?.type === 'auto.http.nitro.onTraceError',
13+
)
14+
);
715
});
816

917
await request.get('/api/test-error').catch(() => {

packages/browser/src/integrations/httpcontext.ts

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,7 @@
1-
import {
2-
_INTERNAL_filterKeyValueData,
3-
defineIntegration,
4-
safeSetSpanJSONAttributes,
5-
SEMANTIC_ATTRIBUTE_SENTRY_OP,
6-
} from '@sentry/core';
1+
import { _INTERNAL_filterKeyValueData, defineIntegration, safeSetSpanJSONAttributes } from '@sentry/core';
72
import { getHttpRequestData, WINDOW } from '../helpers';
83
import { filterCollectedUrl } from '@sentry/core';
9-
import { URL_FULL } from '@sentry/conventions/attributes';
4+
import { HTTP_REQUEST_HEADER_KEY_BASE, SENTRY_OP, URL_FULL, USER_AGENT_ORIGINAL } from '@sentry/conventions/attributes';
105

116
/**
127
* Collects information about HTTP request headers and
@@ -40,9 +35,8 @@ export const httpContextIntegration = defineIntegration(() => {
4035
...(Object.keys(headers).length > 0 ? { headers } : { headers: undefined }),
4136
};
4237
},
43-
processSegmentSpan(span, client) {
44-
const spanOp = span.attributes?.[SEMANTIC_ATTRIBUTE_SENTRY_OP];
4538

39+
processSpan(span, client) {
4640
// if none of the information we want exists, don't bother
4741
if (!WINDOW.navigator && !WINDOW.location && !WINDOW.document) {
4842
return;
@@ -58,11 +52,17 @@ export const httpContextIntegration = defineIntegration(() => {
5852
);
5953

6054
safeSetSpanJSONAttributes(span, {
61-
// Coerce empty string to undefined so the helper's nullish check drops it,
62-
// rather than writing an empty `url.full` attribute onto the span.
63-
[URL_FULL]: spanOp !== 'http.client' ? filterCollectedUrl(reqData.url) : undefined,
64-
'http.request.header.user_agent': headers['User-Agent'],
65-
'http.request.header.referer': headers['Referer'],
55+
// This attribute is used by the "Filter out events from legacy browsers and crawlers" features on the Sentry backend.
56+
// Therefore, it's set on every span.
57+
[USER_AGENT_ORIGINAL]: headers['User-Agent'],
58+
59+
// These attributes, we only need on the segment span (analogous to the `request` context for events)
60+
...(span.is_segment && {
61+
// Coerce empty string to undefined so the helper's nullish check drops it,
62+
// rather than writing an empty `url.full` attribute onto the span.
63+
[URL_FULL]: span.attributes?.[SENTRY_OP] !== 'http.client' ? filterCollectedUrl(reqData.url) : undefined,
64+
[`${HTTP_REQUEST_HEADER_KEY_BASE}.referer`]: headers['Referer'],
65+
}),
6666
});
6767
},
6868
};

0 commit comments

Comments
 (0)