Skip to content

ref(browser-tests): Add waitForMetricRequest helper #20002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
logaretm merged 2 commits into from
Mar 30, 2026
Merged

ref(browser-tests): Add waitForMetricRequest helper #20002

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e34504
ref(browser-tests): Add waitForMetricRequest helper and refactor elem…
logaretm Mar 26, 2026
9a2942e
ref(browser-tests): Address PR review feedback for waitForMetrics helper
logaretm Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 32 additions & 97 deletions dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,84 +1,15 @@
import type { Page, Request, Route } from '@playwright/test';
import type { Page, Route } from '@playwright/test';
import { expect } from '@playwright/test';
import type { Envelope } from '@sentry/core';
import type { SerializedMetric } from '@sentry/core';
import { sentryTest } from '../../../../utils/fixtures';
import {
properFullEnvelopeRequestParser,
shouldSkipMetricsTest,
shouldSkipTracingTest,
} from '../../../../utils/helpers';

type MetricItem = Record<string, unknown> & {
name: string;
type: string;
value: number;
unit?: string;
attributes: Record<string, { value: string | number; type: string }>;
};

function extractMetricsFromRequest(req: Request): MetricItem[] {
try {
const envelope = properFullEnvelopeRequestParser<Envelope>(req);
const items = envelope[1];
const metrics: MetricItem[] = [];
for (const item of items) {
const [header] = item;
if (header.type === 'trace_metric') {
const payload = item[1] as { items?: MetricItem[] };
if (payload.items) {
metrics.push(...payload.items);
}
}
}
return metrics;
} catch {
return [];
}
}

/**
* Collects element timing metrics from envelope requests on the page.
* Returns a function to get all collected metrics so far and a function
* that waits until all expected identifiers have been seen in render_time metrics.
*/
function createMetricCollector(page: Page) {
const collectedRequests: Request[] = [];

page.on('request', req => {
if (!req.url().includes('/api/1337/envelope/')) return;
const metrics = extractMetricsFromRequest(req);
if (metrics.some(m => m.name.startsWith('ui.element.'))) {
collectedRequests.push(req);
}
});

function getAll(): MetricItem[] {
return collectedRequests.flatMap(req => extractMetricsFromRequest(req));
}
import { shouldSkipMetricsTest, shouldSkipTracingTest, waitForMetricRequest } from '../../../../utils/helpers';

async function waitForIdentifiers(identifiers: string[], timeout = 30_000): Promise<void> {
const deadline = Date.now() + timeout;
while (Date.now() < deadline) {
const all = getAll().filter(m => m.name === 'ui.element.render_time');
const seen = new Set(all.map(m => m.attributes['ui.element.identifier']?.value));
if (identifiers.every(id => seen.has(id))) {
return;
}
await page.waitForTimeout(500);
}
// Final check with assertion for clear error message
const all = getAll().filter(m => m.name === 'ui.element.render_time');
const seen = all.map(m => m.attributes['ui.element.identifier']?.value);
for (const id of identifiers) {
expect(seen).toContain(id);
}
}

function reset(): void {
collectedRequests.length = 0;
}
function getIdentifier(m: SerializedMetric): unknown {
return m.attributes?.['ui.element.identifier']?.value;
}

return { getAll, waitForIdentifiers, reset };
function getPaintType(m: SerializedMetric): unknown {
return m.attributes?.['ui.element.paint_type']?.value;
}

sentryTest(
Expand All @@ -91,19 +22,23 @@ sentryTest(
serveAssets(page);

const url = await getLocalTestUrl({ testDir: __dirname });
const collector = createMetricCollector(page);

await page.goto(url);
const expectedIdentifiers = ['image-fast', 'text1', 'button1', 'image-slow', 'lazy-image', 'lazy-text'];

// Wait until all expected element identifiers have been flushed as metrics
await collector.waitForIdentifiers(['image-fast', 'text1', 'button1', 'image-slow', 'lazy-image', 'lazy-text']);
// Wait for all expected element identifiers to arrive as metrics
const [allMetrics] = await Promise.all([
waitForMetricRequest(page, metrics => {
const seen = new Set(metrics.filter(m => m.name === 'ui.element.render_time').map(getIdentifier));
return expectedIdentifiers.every(id => seen.has(id));
}),
page.goto(url),
]);

const allMetrics = collector.getAll().filter(m => m.name.startsWith('ui.element.'));
const renderTimeMetrics = allMetrics.filter(m => m.name === 'ui.element.render_time');
const loadTimeMetrics = allMetrics.filter(m => m.name === 'ui.element.load_time');

const renderIdentifiers = renderTimeMetrics.map(m => m.attributes['ui.element.identifier']?.value);
const loadIdentifiers = loadTimeMetrics.map(m => m.attributes['ui.element.identifier']?.value);
const renderIdentifiers = renderTimeMetrics.map(getIdentifier);
const loadIdentifiers = loadTimeMetrics.map(getIdentifier);

// All text and image elements should have render_time
expect(renderIdentifiers).toContain('image-fast');
Expand All @@ -124,18 +59,18 @@ sentryTest(
expect(loadIdentifiers).not.toContain('lazy-text');

// Validate metric structure for image-fast
const imageFastRender = renderTimeMetrics.find(m => m.attributes['ui.element.identifier']?.value === 'image-fast');
const imageFastRender = renderTimeMetrics.find(m => getIdentifier(m) === 'image-fast');
expect(imageFastRender).toMatchObject({
name: 'ui.element.render_time',
type: 'distribution',
unit: 'millisecond',
value: expect.any(Number),
});
expect(imageFastRender!.attributes['ui.element.paint_type']?.value).toBe('image-paint');
expect(getPaintType(imageFastRender!)).toBe('image-paint');

// Validate text-paint metric
const text1Render = renderTimeMetrics.find(m => m.attributes['ui.element.identifier']?.value === 'text1');
expect(text1Render!.attributes['ui.element.paint_type']?.value).toBe('text-paint');
const text1Render = renderTimeMetrics.find(m => getIdentifier(m) === 'text1');
expect(getPaintType(text1Render!)).toBe('text-paint');
},
);

Expand All @@ -147,25 +82,25 @@ sentryTest('emits element timing metrics after navigation', async ({ getLocalTes
serveAssets(page);

const url = await getLocalTestUrl({ testDir: __dirname });
const collector = createMetricCollector(page);

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 =>
Copy link
Copy Markdown
Member

@Lms24 Lms24 Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@logaretm logaretm Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rare copilot W

metrics.some(m => m.name === 'ui.element.render_time' && getIdentifier(m) === 'image-fast'),
);
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitForMetricRequest only observes future requests. In this test it’s awaited after page.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).

Copilot uses AI. Check for mistakes.

// 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');
});
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.

const allMetrics = collector.getAll();
const renderTimeMetrics = allMetrics.filter(m => m.name === 'ui.element.render_time');
const renderIdentifiers = renderTimeMetrics.map(m => m.attributes['ui.element.identifier']?.value);
const renderTimeMetrics = navigationMetrics.filter(m => m.name === 'ui.element.render_time');
const renderIdentifiers = renderTimeMetrics.map(getIdentifier);

expect(renderIdentifiers).toContain('navigation-image');
expect(renderIdentifiers).toContain('navigation-text');
Expand Down
52 changes: 52 additions & 0 deletions dev-packages/browser-integration-tests/utils/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import type {
Event as SentryEvent,
EventEnvelope,
EventEnvelopeHeaders,
SerializedMetric,
SerializedMetricContainer,
SerializedSession,
TransactionEvent,
} from '@sentry/core';
Expand Down Expand Up @@ -283,6 +285,56 @@ export function waitForClientReportRequest(page: Page, callback?: (report: Clien
});
}

/**
* Wait for metric requests. Accumulates metrics across all matching requests
* 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(
Copy link
Copy Markdown
Member

@Lms24 Lms24 Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Member Author

@logaretm logaretm Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

page: Page,
callback?: (metrics: SerializedMetric[]) => boolean,
): Promise<SerializedMetric[]> {
const collected: SerializedMetric[] = [];

return page
.waitForRequest(req => {
const postData = req.postData();
if (!postData) {
return false;
}

try {
const envelope = properFullEnvelopeRequestParser<Envelope>(req);
const items = envelope[1];
const metrics: SerializedMetric[] = [];
for (const item of items) {
const [header] = item;
if (header.type === 'trace_metric') {
const payload = item[1] as SerializedMetricContainer;
if (payload.items) {
metrics.push(...payload.items);
}
}
}

if (metrics.length === 0) {
return false;
}

collected.push(...metrics);

if (callback) {
return callback(collected);
}

return true;
} catch {
return false;
}
})
.then(() => collected);
}

export async function waitForSession(
page: Page,
callback?: (session: SerializedSession) => boolean,
Expand Down
Loading