Skip to content
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

onTTFB reported without navigation entry after BFCache restore #313

Closed
SuperOleg39 opened this issue Feb 3, 2023 · 8 comments
Closed

onTTFB reported without navigation entry after BFCache restore #313

SuperOleg39 opened this issue Feb 3, 2023 · 8 comments

Comments

@SuperOleg39
Copy link

We use this reciepe - https://github.com/GoogleChrome/web-vitals#onttfb

When page was restored from BFCache, metric.entries array is empty and metric.entries[0].requestStart will throw error.

What is the correct way to collect TTFB in this case?
Maybe any rework needed in this https://github.com/GoogleChrome/web-vitals/blob/main/src/onTTFB.ts#L95 callback?

@tunetheweb
Copy link
Member

It should report as 0 in this case:

web-vitals/src/onTTFB.ts

Lines 95 to 102 in 0a87ab0

onBFCacheRestore(() => {
metric = initMetric('TTFB', 0);
report = bindReporter(
onReport,
metric,
thresholds,
opts!.reportAllChanges
);

I confirmed it does on web.dev:

image

There is an issue (see #275) where TTFB is sometimes not reported when the original nav (that the bfcache restore is restoring too) was the result of a cross origin redirect. In that case we choose not to report a TTFB for the bfcache restore too so it could be you're hitting this issue, in which case you should account for that with optional chaining:

metric.entries[0]?.requestStart

Or if you want to report 0 (we choose not to):

metric.entries[0]?.requestStart || 0

@SuperOleg39
Copy link
Author

Thank you!

So complete example, if we don't need to report 0, can use only metric.value?

import {onTTFB} from 'web-vitals';

onTTFB((metric) => {
  const requestTime = metric.entries[0] ? metric.value - metric.entries[0].requestStart : metric.value;
  console.log('Request time:', requestTime);
});

@SuperOleg39
Copy link
Author

Sorry, understand, the main decision is to not report, or report 0

@tunetheweb
Copy link
Member

Ah yes. In the case of bfcache, as the value is dummied, there is no "entries".

But I don't understand why you are not reporting metric.value every time anyway? Why are you removing the requestStart time? That means you are only reporting the actual backend time, but not any connection set up time (DNS, TCP..etc.) nor any redirect time - all of which counts in the CrUX measured "TTFB".

@SuperOleg39
Copy link
Author

But I don't understand why you are not reporting metric.value every time anyway?

Good question)

@Zizzamia what do you think about simplify TTFB report in perfume.js? Or there is some important reason behind this decision?

@SuperOleg39
Copy link
Author

That means you are only reporting the actual backend time, but not any connection set up time (DNS, TCP..etc.) nor any redirect time

Looks like in perfume.js this behaviour is expected for TTFB, because all connection set up time also reported https://zizzamia.github.io/perfume/#/navigation-timing/

@tunetheweb
Copy link
Member

Ah there's so many definitions of TTFB 😁

But for bfcache restores won't they ALL be 0 anyway? So I would guess your decision is whether to just report 0 if it's not defined.

For web-vitals we decided NOT to do that when the original TTFB was 0 - technically this should never happen, so indicates something went wrong (e.g. it was removed for privacy reasons) so rather than report 0, we elected to just not report it (for the original nav, and for bfcache restores) to avoid skewing the numbers.

There's argument to still report it for bfcache restores (as it is 0), but we elected not to for consistency.

Either way, I think this is all due to how you're implementing it slightly differently than web-vitals.js (in not using metric.value and assuming metric.entries should always exist) so think you need to decide how to handle this, rather than change anything in web-vitals. Do you agree and, if so, you OK with me closing this?

@SuperOleg39
Copy link
Author

Thanks for the complete answers, yes looks like everything is fine on the web-vitals side and we are free to decide how to handle this cases at our side)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants