-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(browser): Fix bug causing unintentional dropping of transactions #12933
Conversation
To add some context, this happens for me with the replay integration and particularly for the spans for the request to the worker blob, and only in Chrome. |
5ef066c
to
e96b4af
Compare
size-limit report 📦
|
// There is a SO post attempting to explain this, but it leaves one with open questions: https://stackoverflow.com/questions/23191918/peformance-getentries-and-negative-duration-display | ||
// The way we clamp the value is probably not accurate, since we have observed this happen for things that may take a while to load, like for example the replay worker. | ||
// TODO: Investigate why this happens and how to properly mitigate. For now, this is a workaround to prevent transactions being dropped due to negative duration spans. | ||
entry.duration < 0 ? 0 : entry.duration, |
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.
l: any reason for not using Math.max
?
entry.duration < 0 ? 0 : entry.duration, | |
Math.max(0, entry.duration), |
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 have a theory that performance is better for the inline if but can change
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.
Just a comment on the comment but it looks like a reasonable guard.
const duration = msToSec( | ||
// Inexplicibly, Chrome sometimes emits a negative duration. We need to work around this. | ||
// There is a SO post attempting to explain this, but it leaves one with open questions: https://stackoverflow.com/questions/23191918/peformance-getentries-and-negative-duration-display | ||
// The way we clamp the value is probably not accurate, since we have observed this happen for things that may take a while to load, like for example the replay worker. |
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.
does the worker take a while to load though? I'd imagine creating the blob should be fairly fast since the code ships with the already loaded JS SDK bundle by default? My guess yesterday was the opposite: that it's too fast and therefore causing weird timing problems 😅
(maybe this is also blob-specific?)
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 think it loaded in something like 4ms when we tested it (which was locally in dev mode). So not too fast. Even if it loaded "too fast" a negative duration doesn't make sense in my eyes 🤔
We saw instances of pageload transactions being dropped because they contained spans with negative durations (
timestamp
<start_timestamp
).After investigating it seems like performance entries by the browser are emitted with a negative duration. We could only observe this happening in Chrome.
The particular environment this could be (flakily) reproduced is on Chrome, for the scaffold Next.js app, with the Sentry SDK and Replay installed. The negative duration span that was recorded was for the replay worker resource.
Other people also seem to be running into this but so far I don't really understand why the duration is negative and how we would mitigate this properly. This PR is more of a hot-fix than anything else.
Attempts to fix: #12914