Uh oh!
There was an error while loading. Please reload this page.
ref(browser-tests): Add waitForMetricRequest helper - #20002
Conversation
…ent timing tests Replace custom createMetricCollector/waitForIdentifiers polling pattern with a shared waitForMetricRequest helper that matches existing waitFor patterns (waitForErrorRequest, waitForTransactionRequest, etc.). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Deps
Other
Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧Core
Deps
Deps Dev
Other
🤖 This preview updates automatically when you update the PR. |
There was a problem hiding this comment.
Pull request overview
Adds a reusable Playwright helper to await and collect metrics from Sentry envelope requests, and refactors the element timing browser integration tests to use this helper instead of custom request listeners + polling.
Changes:
- Added
waitForMetricRequest(page, callback?)to accumulateSerializedMetric[]acrosstrace_metricenvelope requests and resolve once the callback condition is met. - Refactored element timing metrics tests to use
waitForMetricRequest(removing the bespoke collector/polling loop).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dev-packages/browser-integration-tests/utils/helpers.ts | Introduces waitForMetricRequest to parse metric containers from envelopes and accumulate metrics until a condition is satisfied. |
| dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts | Replaces custom metric collection + polling with waitForMetricRequest in pageload and navigation scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await page.goto(url); | ||
| // Wait for pageload element timing metrics to arrive before navigating | ||
| await collector.waitForIdentifiers(['image-fast', 'text1']); | ||
| // Reset so we only capture post-navigation metrics | ||
| collector.reset(); | ||
| await waitForMetricRequest(page, metrics => | ||
| metrics.some(m => m.name === 'ui.element.render_time' && getIdentifier(m) === 'image-fast'), | ||
| ); |
There was a problem hiding this comment.
waitForMetricRequest only observes future requests. In this test it’s awaited afterpage.goto(url), so the pageload metric envelope could already have been sent during navigation, leading to a hang/timeout. Create the waitForMetricRequest(...) promise before calling page.goto, and await them together (similar to the first test).
| // Trigger navigation | ||
| await page.locator('#button1').click(); | ||
| // Wait for navigation element timing metrics | ||
| await collector.waitForIdentifiers(['navigation-image', 'navigation-text']); | ||
| const navigationMetrics = await waitForMetricRequest(page, metrics => { | ||
| const seen = new Set(metrics.filter(m => m.name === 'ui.element.render_time').map(getIdentifier)); | ||
| return seen.has('navigation-image') && seen.has('navigation-text'); | ||
| }); |
There was a problem hiding this comment.
waitForMetricRequest is started after the click. If the click-triggered navigation flushes metrics quickly (or if Playwright waits for navigation during click()), the relevant envelope request can happen before the waiter is registered, making this flaky. Start waitForMetricRequest(...) before click() and await both (e.g. via Promise.all).
size-limit report 📦
|
| * and resolves when the callback returns true for the full set of collected metrics. | ||
| * If no callback is provided, resolves on the first request containing metrics. | ||
| */ | ||
| export function waitForMetricRequest( |
There was a problem hiding this comment.
super-l: given we return Promise<SerializedMetric[]>, wdyt about just calling this waitForMetrics? I went with this pattern for streamed spans (example, though your collection mechanism is much more sophisticated :D).
Besides, I think the waitForErrorRequest et al. helpers actually return a Promise<Request<Event>> which was a bit cumbersome to work with in most cases. So I personally prefer this pattern of directly returning the metrics much more.
No strong feelings, just a suggestion!
There was a problem hiding this comment.
I think that is actually nicer, most of the time we want the actual thing we are waiting for/filtering on. We should do the same for others as well if the pattern holds there.
Good point, thanks!
| // Reset so we only capture post-navigation metrics | ||
| collector.reset(); | ||
| await waitForMetricRequest(page, metrics => |
There was a problem hiding this comment.
m: This is a valid catch from copilot: we should start listening for metrics before going to the URL (or clicking below) and after the respective action await the promise.
Rename waitForMetricRequest to waitForMetrics since it returns SerializedMetric[] directly. Fix race conditions in navigation test by creating metric listener promises before triggering page.goto() and click() actions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Summary
waitForMetricRequesthelper to browser integration test utils, following the samepage.waitForRequestpattern aswaitForErrorRequest,waitForTransactionRequest, etc.waitForMetricRequestinstead of a customcreateMetricCollectorwith polling-basedwaitForIdentifiersSerializedMetric[]across envelope requests and resolves when the callback returnstruefor the full collected setCloses#20005 (added automatically)