fix: remove recovery logic for fmp

[patrickhulce]

removes recovery logic for using the last FMPCandidate when FMP can't be found in the trace

this gives a misleading result and since our network idle wait time is far more generous now, the only cases this should happen is when it legitimately could not be computed or a bug that needs more investigation we shouldn't mask silently

[paulirish]

I've been considering a slightly diff approach for this:

We continue to use our fallback, however we issue a debugString or something that we're not 100% confident this FMP was correctly selected.

Regardless, I think it'd be valuable to continue tracking the value of this fallback so we could compare it against the explicitly tagged one. E.g. a plots run where were see these two separately could be great.

wdyt

[patrickhulce]

+1 to tracking when this fails for sure, but I was under the impression that we consider these failure cases now that the 2-quiet impl is in over the 0-quiet from before?

[brendankenny]

Making errors more obvious in plots/ would be good in general so we more easily see if something is wrong/has gone wrong.

If we want to get more data on ditching FMP candidates before removing them, we could still throw an error on the only FMP candidates being before navStart. That would get rid of some of the negative perf metric number errors we've seen.

[brendankenny]

but I was under the impression that we consider these failure cases now that the 2-quiet impl is in over the 0-quiet from before?

Because we're more aggressively ending the trace so the candidate is more likely to be wrong? Or something else?

[patrickhulce]

If we want to get more data on ditching FMP candidates before removing them, we could still throw an error on the only FMP candidates being before navStart. That would get rid of some of the negative perf metric number errors we've seen.

+1 let's at least filter out ones before nav start for sure

Because we're more aggressively ending the trace so the candidate is more likely to be wrong? Or something else?

I was actually referring to Chrome's impl that switched from 0-quiet to 2-quiet which is what I assumed was the reason for originally including a fallback like this in lighthouse.

[paulirish]

which is what I assumed was the reason for originally including a fallback like this in lighthouse.

I don't recall exactly when the quiet change landed in Chrome, so I'm not sure. But we certainly weren't getting an FMP event in the trace rather often.

[brendankenny]

maybe when we land #2473 we can come back to this and measure failure rates? I'm personally inclined to 👍 so we do the right thing, but it would be good to know how much more we can expect to fail

[brendankenny]

@wwwillchen this PR could be a good test candidate for measuring before/after FMP values and failure rate at getting an FMP event at all.

[paulirish]

Conclusion from Patrick: Once Sentry is merged, we can evaluate the fallback rate. If it's already quite low, then we can merge this PR and let the FMP identification fail. If the fallback rate is high, then we need to investigate more.

So we're blocking this PR on #2420