feat(browser): Replace element timing spans with metrics#19869
feat(browser): Replace element timing spans with metrics#19869
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
size-limit report 📦
|
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.
|
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
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.
Lms24
left a comment
There was a problem hiding this comment.
Thanks a lot for taking this on! I had some comments around attributes and naming. Would also like to get a review from @chargome since my knowledge around auto-emitted metrics is rather limited. Let's make sure we follow all prior art here.
| const renderTime = elementEntry.renderTime; | ||
| const loadTime = elementEntry.loadTime; | ||
| if (paintType) { | ||
| metricAttributes['element.paint_type'] = paintType; |
There was a problem hiding this comment.
m: Let's make sure we use ui.element.* attributes as defined in conventions
There was a problem hiding this comment.
I had already done it but my clanker "forgot" to push it 🤦♂️
| ? [msToSec(renderTime), 'render-time'] | ||
| : [timestampInSeconds(), 'entry-emission']; | ||
| if (renderTime) { | ||
| metrics.distribution(`element_timing.render_time`, renderTime, { |
There was a problem hiding this comment.
just to sanity check: @chargome are you aware of any naming conventions around automatically collected metrics?
There was a problem hiding this comment.
should we name it similarly to the attributes? It feels like metrics here are just an aggregation of what we collected previously as attributes. So ui.element.render_time?
But not sure if a metric having itself as an attribute is a bit weird/redundant or not....
There was a problem hiding this comment.
ui.element.render_time sounds good to me. But in this case we don't need the same attribute
There was a problem hiding this comment.
No strict conventions, ui.element.render_time seems fine
dev-packages/browser-integration-tests/suites/tracing/metrics/element-timing/test.ts
Show resolved
Hide resolved
b77c217 to
4f49333
Compare
chargome
left a comment
There was a problem hiding this comment.
Generally LGTM but isn't this a behavioral breaking change? Since user need to opt-in to this intergation now?
| if (DEBUG_BUILD && 'enableElementTiming' in options) { | ||
| debug.warn( | ||
| '[Tracing] `enableElementTiming` is deprecated and no longer has any effect. Use the standalone `elementTimingIntegration` instead.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
m: Should we rather log this unconditionally without a debug flag actually?
There was a problem hiding this comment.
It was never documented as far as we know, our DX team found it while trying to PR something similar.
I think yes, we should always log that in this case.
|
I renamed the metric and dropped the attribute of the same name |
I think this is fine, given these spans never made it to sentry as Awad already said. Furthermore, the option and the spans weren't documented (which reminds me, @logaretm we still need to document the new integration once this is released). I think element timing is a good initial test candidate for browser metrics. We can do more of these in the future. |
ee505f9 to
7773964
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
…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>
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>
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>
…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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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>
Align attribute names with Sentry conventions and add missing attributes (id, type, url, width, height) from the Element Timing API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Non-metrics bundles now export a no-op shim that warns at runtime, matching the established pattern for browserTracingIntegrationShim. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndles The integration has no tracing dependency, so all .metrics bundles should export the real implementation. Also fixes misleading shim warning message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use consoleSandbox instead of DEBUG_BUILD guard so the warning shows in production bundles too. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rename element_timing.render_time → ui.element.render_time and element_timing.load_time → ui.element.load_time to align with Sentry attribute conventions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Avoids silently dropping metrics due to falsy `0` values from cross-origin images without `Timing-Allow-Origin`, making the intent clearer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
86042cb to
3dd7e22
Compare
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