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.
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
ref(tracing): Remove mark measurements #5605
Conversation
457688a
to
a85752b
Compare
a85752b
to
940b0a0
Compare
size-limit report 📦
|
}); | ||
|
||
// Delete mark.fid as we don't want it to be part of final payload | ||
delete _measurements['mark.fid']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to delete it at the end here? Can't we just remove the line that sets mark.fid
on _measurements
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, this is kind of a nit. It's being used to calculate the duration of the FID span but the way we're doing it now with this pattern seems a bit weird to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it's not the cleanest, but it is the most bundle size efficient to keep it in the same object instead of constructing a new variable.
tbh it's even worth considering if the FID span is that valuable - i don't really think it is, but that's a convo for another time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, keeping the bundle size down is important!
Agreed on the lack of value of the FID span, I think it is redundant since we're already saving it as a measurement, but yes we can discuss more on this later
Are the lines still there? @smeubank wants to know |
@lforst Yep, we refactored the frontend so we will still have the lines even without the |
Ref getsentry/sentry#38023
fixes https://getsentry.atlassian.net/browse/PERF-1700
https://www.notion.so/sentry/Stop-sending-mark-X-measurements-from-JavaScript-SDK-66d26d45e860419280ec853377195305
Currently the JS SDK sends extra measurements though, specifically
mark.lcp
,mark.fid
,mark.fp
,mark.fcp
. This is done because these measurements are being used in the Sentry front-end UI to draw these lines:These measurements should be removed, as they count toward a user’s custom measurement quota - hence we would be reducing the amount of custom measurements they can send and use because the SDK is setting them automatically. In addition, there is no way these measurements are ever going to be indexed, so we can’t move them to be built-in.
To remove these
mark.X
measurements, we’ll need to first adjust the Sentry front-end UI to stop using them, and then delete them from the SDK.@0Calories