feat(browser): Replace element timing spans with metrics#19869
feat(browser): Replace element timing spans with metrics#19869
Conversation
…ne integration Remove element timing span creation from browserTracingIntegration and introduce a new elementTimingIntegration that emits Element Timing API data as Sentry distribution metrics instead of spans. Element timing values (renderTime, loadTime) are point-in-time timestamps, not durations, making metrics a better fit than spans. The new integration emits `element_timing.render_time` and `element_timing.load_time` metrics with `element.identifier` and `element.paint_type` attributes. refs JS-1678 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Deps
Other
Bug Fixes 🐛Core
Deps
Other
Internal Changes 🔧Deps Dev
Other
🤖 This preview updates automatically when you update the PR. |
size-limit report 📦
|
Rewrite Playwright integration tests to expect Sentry distribution metrics instead of spans, matching the new elementTimingIntegration. 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.
|
Metrics are buffered and flushed after 5 seconds. The previous test used a 3 second timeout which wasn't enough. Now properly waits for metric envelopes with adequate timeouts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The integration was only exported from the npm package entry point but not from the CDN bundle entry points, causing Sentry.elementTimingIntegration to be undefined in bundle_tracing_* test configurations. Verified locally: - PW_BUNDLE=bundle_tracing_logs_metrics: 2 passed - PW_BUNDLE=bundle_tracing_replay_feedback_logs_metrics_min: 2 passed - browser-utils unit tests: 132 passed - browser unit tests: 545 passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates Element Timing collection in the browser SDK from (awkward) span modeling to Sentry distribution metrics, by removing Element Timing span creation from browserTracingIntegration and introducing a standalone elementTimingIntegration export.
Changes:
- Remove Element Timing span tracking from
browserTracingIntegrationand deprecate theenableElementTimingoption. - Add
elementTimingIntegration(re-exported from@sentry-internal/browser-utils) and emitelement_timing.render_time/element_timing.load_timeas distribution metrics withelement.identifierandelement.paint_typeattributes. - Update unit + Playwright integration tests to validate metric emission instead of spans.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/browser/src/tracing/browserTracingIntegration.ts | Stops starting Element Timing tracking; deprecates enableElementTiming. |
| packages/browser/src/index.ts | Re-exports elementTimingIntegration for SDK consumers. |
| packages/browser/src/index.bundle.tracing.ts | Re-exports elementTimingIntegration in tracing bundle. |
| packages/browser/src/index.bundle.tracing.replay.ts | Re-exports elementTimingIntegration in tracing+replay bundle. |
| packages/browser/src/index.bundle.tracing.replay.logs.metrics.ts | Re-exports elementTimingIntegration in tracing+replay+logs+metrics bundle. |
| packages/browser/src/index.bundle.tracing.replay.feedback.ts | Re-exports elementTimingIntegration in tracing+replay+feedback bundle. |
| packages/browser/src/index.bundle.tracing.replay.feedback.logs.metrics.ts | Re-exports elementTimingIntegration in tracing+replay+feedback+logs+metrics bundle. |
| packages/browser/src/index.bundle.tracing.logs.metrics.ts | Re-exports elementTimingIntegration in tracing+logs+metrics bundle. |
| packages/browser-utils/src/metrics/elementTiming.ts | Implements elementTimingIntegration emitting distribution metrics; makes startTrackingElementTiming a deprecated no-op. |
| packages/browser-utils/src/index.ts | Exports elementTimingIntegration from browser-utils package surface. |
| packages/browser-utils/test/metrics/elementTiming.test.ts | Reworks unit tests to assert metrics.distribution calls instead of spans. |
| dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts | Updates Playwright tests to assert metric envelopes and identifiers. |
| dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/init.js | Enables Sentry.elementTimingIntegration() in the test app init. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch { | ||
| return []; |
dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts
Outdated
Show resolved
Hide resolved
dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts
Outdated
Show resolved
Hide resolved
…ing tests Instead of sleeping 8 seconds and hoping all metrics have arrived, poll until the expected element identifiers appear in the collected metrics. This makes the tests faster (6-13s vs 16-20s) and more reliable — they complete as soon as data arrives and fail with clear assertions when it doesn't. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dead default value for removed
enableElementTimingusage- Removed the dead
enableElementTiming: truedefault value from DEFAULT_BROWSER_TRACING_OPTIONS since the option is deprecated and no longer used.
- Removed the dead
Or push these changes by commenting:
@cursor push 8056bbebb4
Preview (8056bbebb4)
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts
--- a/dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts
+++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts
@@ -96,14 +96,7 @@
await page.goto(url);
// Wait until all expected element identifiers have been flushed as metrics
- await collector.waitForIdentifiers([
- 'image-fast',
- 'text1',
- 'button1',
- 'image-slow',
- 'lazy-image',
- 'lazy-text',
- ]);
+ await collector.waitForIdentifiers(['image-fast', 'text1', 'button1', 'image-slow', 'lazy-image', 'lazy-text']);
const allMetrics = collector.getAll().filter(m => m.name.startsWith('element_timing.'));
const renderTimeMetrics = allMetrics.filter(m => m.name === 'element_timing.render_time');
diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts
--- a/packages/browser/src/tracing/browserTracingIntegration.ts
+++ b/packages/browser/src/tracing/browserTracingIntegration.ts
@@ -334,7 +334,6 @@
enableLongTask: true,
enableLongAnimationFrame: true,
enableInp: true,
- enableElementTiming: true,
ignoreResourceSpans: [],
ignorePerformanceApiSpans: [],
detectRedirects: true,This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
|
|
||
| const { | ||
| enableInp, | ||
| enableElementTiming, |
There was a problem hiding this comment.
Deprecated option silently ignored without runtime warning
Medium Severity
The enableElementTiming option previously defaulted to true, meaning all browserTracingIntegration users automatically received element timing data. Now the option is deprecated and silently ignored — no runtime warning is emitted in DEBUG_BUILD. Users who relied on this default behavior will silently lose element timing data with no indication that they need to add elementTimingIntegration() to their integrations array. This is effectively a breaking change to a public API option without a runtime deprecation notice to guide migration.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| }); | ||
| }, | ||
| }; | ||
| }) satisfies IntegrationFn; |
There was a problem hiding this comment.
Missing integration/E2E test for new feat PR
Low Severity
The PR updates an existing integration test from span-based assertions to metric-based assertions but significantly weakens the test coverage. The old tests thoroughly validated metric values (render time ranges, load time ranges, relative ordering), element metadata (tag type, size, URL, id), and span structure. The new tests only check that identifiers appear in the correct metric names and validate paint type for two elements. Metric values, element metadata, and boundary checks are not asserted, reducing confidence that the integration works correctly end-to-end.
Triggered by project rule: PR Review Guidelines for Cursor Bot
The property was removed from defaults but the type still required it, causing a build error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes element timing span creation from
browserTracingIntegration(deprecatesenableElementTimingoption, introduces a new standaloneelementTimingIntegrationthat emits Element Timing API data as Sentry distribution metrics instead of spans.Emits
element_timing.render_timeandelement_timing.load_timemetrics withelement.identifierandelement.paint_typeattributes. I believe users can query by the element identifier if they are interested in metrics for a specific element.Me and Lukas think this is a safe change because it was never documented, even then I made sure to export NO-OP replacement functions to stub them out.
Reasoning for the change
Element Timing values (
renderTime,loadTime) are point-in-time timestamps, not durations. Modeling them as spans required awkward workarounds (zero-duration spans, arbitrary start times) that didn't produce meaningful trace data. Metrics are the correct abstraction here.See discussion in #19261 for full context.
Usage
closes #19260