Repository navigation
Conversation
Contributor
size-limit report 📦
|
msonnb
changed the base branch from
ms/browser-tests-trace-semantics
to
ms/browser-tests-browser-tracing
October 5, 2026 07:28
msonnb
added this pull request to stack #24891
October 5, 2026 07:28
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 5, 2026 07:41
a8d6d9c to
83fab7a
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
2 times, most recently
from
October 5, 2026 13:23
5a08efb to
dc00794
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
2 times, most recently
from
October 7, 2026 11:27
b188ce7 to
a064fc9
Compare
msonnb
marked this pull request as ready for review
October 7, 2026 12:33
msonnb
requested review from
Lms24 and
logaretm
and removed request for
a team
October 7, 2026 12:33
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 12:50
a064fc9 to
3d541db
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:02
3d541db to
0fa7920
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:12
0fa7920 to
136bc99
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:25
136bc99 to
4c8dc91
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:37
4c8dc91 to
52e9455
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
logaretm
approved these changes
Oct 7, 2026
Lms24
reviewed
Oct 8, 2026
Lms24
left a comment
Lms24
left a comment
Member
There was a problem hiding this comment.
m: I'd prefer keeping one transaction test that asserts on the respective web vital being added as a measurement to the pageload span.
Comment on lines
-18
to
-23
| // A plain (non-streamed) `beforeSendSpan` operates on the v1 `SpanJSON`. INP is sent as a v2 span, | ||
| // so this verifies the static callback still runs and its changes are carried into the v2 span. | ||
| beforeSendSpan: Sentry.withStaticSpan(span => { | ||
| if (span.op === 'ui.interaction.click') { | ||
| span.description = 'scrubbed'; | ||
| span.data['custom.attribute'] = 'from-before-send-span'; |
Member
There was a problem hiding this comment.
m: I think we should keep this test as "static", since the main objective here is asserting that the INP span goes through beforeSendSpan in the v1 format when span streaming is disabled.
So once we remove transactions, this test can be dropped
Comment on lines
+12
to
+22
| 'records connection RTT on pageload and navigation spans', | ||
| async ({ getLocalTestUrl, page, browserName }) => { | ||
| sentryTest.skip(shouldSkipTracingTest() || browserName !== 'chromium'); | ||
| const url = await getLocalTestUrl({ testDir: __dirname }); | ||
| await page.goto(url); | ||
|
|
||
| const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise) as Event; | ||
|
|
||
| const navigationRequestPromise = waitForTransactionRequest( | ||
| page, | ||
| event => event.contexts?.trace?.op === 'navigation', | ||
| ); | ||
| await page.goto(`${url}#foo`); | ||
|
|
||
| const navigationRequest = envelopeRequestParser(await navigationRequestPromise) as Event; | ||
|
|
||
| expect(pageloadRequest.contexts?.trace?.op).toBe('pageload'); | ||
| expect(navigationRequest.contexts?.trace?.op).toBe('navigation'); | ||
| const [pageload] = await waitForStreamedSpanAndTraceHeaderOnUrl(page, url); | ||
| const [navigation] = await waitForStreamedSpanAndTraceHeaderOnUrl(page, `${url}#foo`); | ||
|
|
||
| expect(pageloadRequest.measurements?.['connection.rtt']?.value).toBeDefined(); | ||
| expect(navigationRequest.measurements?.['connection.rtt']).toBeUndefined(); | ||
| expect(pageload.attributes[NETWORK_CONNECTION_RTT]).toEqual({ type: 'integer', value: 0 }); | ||
| expect(navigation.attributes[NETWORK_CONNECTION_RTT]).toEqual(pageload.attributes[NETWORK_CONNECTION_RTT]); | ||
| expect(navigation.attributes[BROWSER_WEB_VITAL_FCP_VALUE]).toBeUndefined(); | ||
| expect(navigation.attributes[BROWSER_WEB_VITAL_TTFB_VALUE]).toBeUndefined(); |
Member
There was a problem hiding this comment.
hmm actually not sure if sending connction.rtt on navigation spans is an SDK bug or expected behaviour now? probably worth looking into. if it's a bug, I'm also fine with merging the test as-is and fixing it in a follow-up.
Member
Author
There was a problem hiding this comment.
sentry-javascript/packages/browser-utils/src/performance/entries.ts
Lines 526 to 534 in d489a08
so from this it looks like this is expected now
| expect(streamSpan.end_timestamp).toBeGreaterThan(streamSpan.start_timestamp); | ||
| expect(streamSpan.parent_span_id).toBe(requestSpan.parent_span_id); | ||
| expect(streamSpan.trace_id).toBe(requestSpan.trace_id); | ||
| expect(streamSpan.end_timestamp).toBeGreaterThanOrEqual(streamSpan.start_timestamp); |
Member
There was a problem hiding this comment.
l: any reason this assertion got weaker?
Suggested change
| expect(streamSpan.end_timestamp).toBeGreaterThanOrEqual(streamSpan.start_timestamp); | |
| expect(streamSpan.end_timestamp).toBeGreaterThan(streamSpan.start_timestamp); |
| }); | ||
|
|
||
| expect(requestSpan?.data).not.toHaveProperty('url.fragment'); | ||
| expect(requestSpan?.attributes).not.toHaveProperty('url.fragment'); |
Member
There was a problem hiding this comment.
m: TIL that this check actually asserts that attributes.url.fragment doesn't exist, instead of attributes['url.fragment']. I had no idea 😬
Turns out the correct syntax here is
Suggested change
| expect(requestSpan?.attributes).not.toHaveProperty('url.fragment'); | |
| expect(requestSpan?.attributes).not.toHaveProperty(['url.fragment']); |
Would be amazing if you could go over our tests and check for this pattern. I'm pretty sure we use this more often. But of course as a follow up!
EDIT: Looks like this is "just" a playwright thing. Vitest asserts on the whole key.
Member
Author
There was a problem hiding this comment.
oof, good catch. only one more nuxt test was affected, fixed it here: 811c410#diff-6ef19271481ee7f31290d28e413d8ff6213b9251009cc4f01c140bfddf3eb80d
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 9, 2026 08:49
0949ba8 to
811c410
Compare
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: OpenAI ChatGPT <codex@openai.com>
Co-Authored-By: OpenAI ChatGPT <codex@openai.com>
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 9, 2026 09:42
2beb4b0 to
a063aeb
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a063aeb. Configure here.
Lms24
approved these changes
Oct 9, 2026
Co-Authored-By: OpenAI ChatGPT <codex@openai.com>
Comment on lines
+605
to
+609
| if (!interactionSelector) { | ||
| // Older interactions may no longer be buffered. A requested click must be observed, | ||
| // otherwise hiding would silently lose the INP value the caller is testing. | ||
| fallback = setTimeout(done, 1000); | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: The hidePage function can hang indefinitely when an interactionSelector is provided, as there's no fallback timeout if the expected performance entry isn't found.
Severity: MEDIUM
Suggested Fix
Add a fallback timeout within the page.evaluate call, for example by racing the existing promise with a setTimeout. This will ensure the test fails with a clear error message instead of hanging when the performance entry is not found.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/browser-integration-tests/utils/helpers.ts#L605-L609
Potential issue: When `hidePage` is called with an `interactionSelector`, it waits for a
corresponding 'click' `PerformanceEventTiming` entry. However, if this entry is not
found (e.g., due to buffer eviction on a loaded CI machine), the Promise within
`page.evaluate()` never resolves. Because `page.evaluate()` lacks a built-in timeout,
the test will hang until the global test timeout is reached (e.g., 30 seconds),
resulting in a generic and unhelpful "Test timeout exceeded" error instead of a clear
failure message. This makes debugging flaky tests difficult.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Exercise request instrumentation, web vitals, interactions, and user timing with default span streaming. Retain the existing transaction counterparts for fetch, XHR, resource timing, and TTFB as explicitly pinned
*-staticsuites so the migration preserves coverage of both lifecycles. Check RTT as a span attribute on pageload and navigation.Fixes #24143