fix(metrics): use web-vitals ttfb calculation #11185
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AbhiPrasad
requested review from
stephanie-anderson and
lforst
and removed request for
a team
|
Opened #11186 as a follow up to this |
AbhiPrasad
force-pushed
the
abhi-ttfb-tracing-fix
branch
from
2a8b25c
to
b121a3d
Compare
size-limit report 📦
|
Closed
Lms24
approved these changes
Mar 19, 2024
There was a problem hiding this comment.
Thanks for the detailed writeup! The fix makes sense to me and iiuc should even increase accuracy (?).
RE vendoring vs using the library: Given that this library seems to be well maintained I agree, it's probably less overhead to rely on it directly. One thing we should check is if we removed unnecessary code or anything similar to reduce bundle size. In that case, I think, keeping the vendored version would make more sense.
AbhiPrasad
force-pushed
the
abhi-ttfb-tracing-fix
branch
from
8339bc5
to
ead53ec
Compare
AbhiPrasad
added a commit
that referenced
this pull request
Mar 21, 2024
c298lee
pushed a commit
that referenced
this pull request
Mar 22, 2024
Recommend reading through https://web.dev/articles/ttfb before review. In https://www.notion.so/sentry/TTFB-vital-is-0-for-navigation-events-2337114dd75542569eb70255a467aba6 we identified that ttfb was being incorrectly calculated in certain scenarios. This was because we were calculating `ttfb` relative to transaction start time, **before** it has been adjusted by `browser` related spans about request/response ( remember browser tracing adjusts the start timestamp of a pageload transaction after adding certain request/response related spans). This meant that `Math.max(responseStartTimestamp - transactionStartTime, 0)` would just end up being `0` most of the time because using `transactionStartTime` was not correct. To fix this, we avoid trying to rely on our `transactionStartTime` timestamp at all, but instead using the web vitals version helper for this. When this gets merged in, I'll backport it to v7. I'm doing this in v8 first because I don't want to deal with the merge conflict that comes when we eventually migrate this code from tracing internal into the browser package. As a next step, we should seriously think about getting rid of all of our vendored code and just rely on the web vitals library - it's a huge pain to maintain this, and I'm sure there are some insidious bugs sneaking about.
cadesalaberry
pushed a commit
to cadesalaberry/sentry-javascript
that referenced
this pull request
Apr 19, 2024
Recommend reading through https://web.dev/articles/ttfb before review. In https://www.notion.so/sentry/TTFB-vital-is-0-for-navigation-events-2337114dd75542569eb70255a467aba6 we identified that ttfb was being incorrectly calculated in certain scenarios. This was because we were calculating `ttfb` relative to transaction start time, **before** it has been adjusted by `browser` related spans about request/response ( remember browser tracing adjusts the start timestamp of a pageload transaction after adding certain request/response related spans). This meant that `Math.max(responseStartTimestamp - transactionStartTime, 0)` would just end up being `0` most of the time because using `transactionStartTime` was not correct. To fix this, we avoid trying to rely on our `transactionStartTime` timestamp at all, but instead using the web vitals version helper for this. When this gets merged in, I'll backport it to v7. I'm doing this in v8 first because I don't want to deal with the merge conflict that comes when we eventually migrate this code from tracing internal into the browser package. As a next step, we should seriously think about getting rid of all of our vendored code and just rely on the web vitals library - it's a huge pain to maintain this, and I'm sure there are some insidious bugs sneaking about.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Kevan first took a stab at this with #10970.
Recommend reading through https://web.dev/articles/ttfb before review.
In https://www.notion.so/sentry/TTFB-vital-is-0-for-navigation-events-2337114dd75542569eb70255a467aba6 we identified that ttfb was being incorrectly calculated in certain scenarios. This was because we were calculating
ttfbrelative to transaction start time, before it has been adjusted bybrowserrelated spans about request/response ( remember browser tracing adjusts the start timestamp of a pageload transaction after adding certain request/response related spans).This meant that
Math.max(responseStartTimestamp - transactionStartTime, 0)would just end up being0most of the time because usingtransactionStartTimewas not correct.To fix this, we avoid trying to rely on our
transactionStartTimetimestamp at all, but instead using the web vitals version helper for this.When this gets merged in, I'll backport it to v7. I'm doing this in v8 first because I don't want to deal with the merge conflict that comes when we eventually migrate this code from tracing internal into the browser package.
As a next step, we should seriously think about getting rid of all of our vendored code and just rely on the web vitals library - it's a huge pain to maintain this, and I'm sure there are some insidious bugs sneaking about.