feat(browser)!: Extract performance.{mark,measure} spans into new userTimingSpansIntegration - #22554
Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
06b5536 to
f28a18c
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
Minor stuff, proposing a rename and maybe an opportunity to align the origin with similar origins in the SDK.
| const spanEndTimestamp = originalStartTimestamp + duration; | ||
|
|
||
| const attributes: SpanAttributes = { | ||
| [SENTRY_ORIGIN]: 'auto.resource.browser.metrics', |
There was a problem hiding this comment.
l: Maybe we should have a better descriptive origin here, I know this is just an extraction PR but could be a good opportunity to do that? Unless product relies on this specific value. cc @Lms24
Since origins are supposed to inform the user which part of the SDK created this telemetry we could use: auto.browser.performance_timing or similar. What do you think?
There was a problem hiding this comment.
yup, agreed. Whatever name we settle on, we should reflect it in the origin. See: https://develop.sentry.dev/sdk/telemetry/traces/trace-origin/
There was a problem hiding this comment.
Then auto.browser.user_timing.{mark/measure}? or just auto.browser.user_timing?
There was a problem hiding this comment.
went for the more fine-grained auto.browser.user_timing.{mark|measure}
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
One more suggestion: WDYT about removing setting the sentry-tracing-init mark? Feels like we're not better than some other libraries/scripts that emit their own marks into users' timelines.
The purpose of this mark was to know when the SDK initializes in comparison to the relative timestamps. With this integration only being optional, we loose the generated span in 99% of cases. I'd vote we remove it and - if it turns out this information would be good to have - we add back a 0-duration span instead.
There was a problem hiding this comment.
suggestion: wdyt about pulling all the usertiming tests out of the /metrics sub directory? Given this is now an integration, it could just live under tracing.
There was a problem hiding this comment.
+1 I think the browser metrics is a bloated term right now
| addPerformanceInstrumentationHandler('mark', handleEntries); | ||
| addPerformanceInstrumentationHandler('measure', handleEntries); |
There was a problem hiding this comment.
we talked about the approach change compared to the browserTracing implementation offline.
We might miss certain emissions with the listener approach here as opposed to peformance.getEntries() that we previously caught (two scenarios: entries happening before Sentry.init, or right before the idle span ends).
Let's check if we can use the getEntries approach here easily (without cursor conflicts in browserTracing). If we can, I'd rather prefer this. We likely need another client hook (something like segmentSpanEnd that fires before afterSegmentSpanEnd, but feel free to come up with whatever makes sense here).
If it turns out there are issues with the getEntries approach, we can also stick with this approach.
There was a problem hiding this comment.
i went with a beforeIdleSpanEnd hook in 88d45ad that runs before the idle span's ending logic, since (for now at least) there seem to be no other use cases for a beforeSegmentSpanEnd hook and it avoids deduplication logic.
we could now also remove—either here or in a follow up—the local idle span beforeSpanEnd callback in favor of this hook (it's only other usage is in browserTracingIntegration)
…userTimingSpansIntegration`
f28a18c to
df7f7ce
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 df7f7ce. Configure here.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
One more request: Let's exclude this integration from the SDK CDN bundles. If we remove it from browserTracing and make it opt-in for SDKs in NPM, we might as well spare bundle users the bundle size.
Instead, should make it a pluggable integration (see CDN tab) that also ships its own CDN bundle, which users can register in addition to the main SDK bundle.
If you prefer, we can also tackle adding the CDN bundle in a follow-up. Happy to leave that up to you! But we should remove the integration from the SDK bundle in this one.
|
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Nice, thanks for reworking the approach and handling the CDN integration bundle!
LGTM now! Last optional change: I think we can move the two remaining integration tests out of the /metrics sub dir and potentially group all user timing tests under /tracing/user-timing/*.
Good to go from my end, feel free to merge!
3 participants
What?
browserTracingIntegrationno longer capturesperformance.mark()/performance.measure()spans by default. This behavior moves into a new opt-inuserTimingSpansIntegration, and theignorePerformanceApiSpansoption is replaced by the integration'signoreoption.Why?
User Timing spans are useful for some apps and pure noise for others (browser extensions and third-party libraries emit their own mark/measure entries). Bundling them into
browserTracingIntegrationmeant everyone paid the bundle-size and span-volume cost whether they wanted the data or not, and the only escape hatch was an ignore-list. Making it a separate, explicitly-added integration means you opt in when you want it, and it drops out of the tree-shaken bundle when you don't.Closes #22352