-
Notifications
You must be signed in to change notification settings - Fork 142
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
🐛 [RUM-2782] remove buggy redirect timing estimation based on fetchStart #2683
Conversation
d75de18
to
64ef390
Compare
/to-staging |
🚂 Branch Integration: starting soon, merge in < 10m Commit 64ef390092 will soon be integrated into staging-14. This build is going to start soon! (estimated merge in less than 10m) Use |
Bundles Sizes Evolution
|
…c into staging-14 Co-authored-by: Benoît Zugmeyer <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit 64ef390092 has been merged into staging-14 in merge commit 9a182d9551. Check out the triggered pipeline on Gitlab 🦊 |
const areRedirectionTimingsInOrder = | ||
!hasRedirection(entry) || areInOrder(entry.startTime, entry.redirectStart, entry.redirectEnd, entry.fetchStart) |
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.
🥜 nitpick: I think it would be easier to read as:
const areRedirectionTimingsInOrder = | |
!hasRedirection(entry) || areInOrder(entry.startTime, entry.redirectStart, entry.redirectEnd, entry.fetchStart) | |
const areRedirectionTimingsInOrder = hasRedirection(entry) | |
? areInOrder(entry.startTime, entry.redirectStart, entry.redirectEnd, entry.fetchStart) | |
: true |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2683 +/- ##
==========================================
- Coverage 93.38% 93.33% -0.05%
==========================================
Files 239 239
Lines 6937 6931 -6
Branches 1526 1524 -2
==========================================
- Hits 6478 6469 -9
- Misses 459 462 +3 ☔ View full report in Codecov by Sentry. |
/to-staging |
🚂 Branch Integration: starting soon, merge in < 10m Commit 76ac2e2a62 will soon be integrated into staging-14. This build is going to start soon! (estimated merge in less than 10m) Use |
…c into staging-14 Co-authored-by: Benoît Zugmeyer <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit 76ac2e2a62 has been merged into staging-14 in merge commit d84fda558c. Check out the triggered pipeline on Gitlab 🦊 |
Motivation
The assumptions made when implementing #315 are not accurate anymore:
startTime
can be different fromfetchStart
for other reasons than a redirection (ex: when a service worker is involved)Firefox now provide accurate
redirectStart
andredirectEnd
even for cross-origin resources (as long as we have a correctTiming-Allow-Origin
header, same as Chrome)This is a first step to fix #2566
Changes
Remove the redirect timing approximations in RUM resource events.
Testing
I have gone over the contributing documentation.