fix: always use navStart as speedline timeOrigin #2114
Conversation
|
Is this really something that should be fixed upstream? This will make a huge difference for some sites at least, and it's a little concerning that there aren't even more test errors than there are here, since it means our well-behaved unit tests and no smoke tests aren't representing reality very well... |
They already provide you with an option to do this, speedline gets a trace and outputs the defined metric over that time period, it shouldn't be in the business of forcing this IMO. However, I am filing an issue and PR that would still include screenshots from before the time origin if it's later than the screenshot which is a bug :) (in this case it's really the white default screenshot being inserted that throws everything off) |
If that's true then we've been hugely wrong for some sites :) |
Im fine with patching here to avoid a speedline bug, but I guess maybe I don't understand how speedline is defining the interval in question. Speedline integrates a function from some start to visually complete. If that start is usually but not always navStart (with no way to predict which will be used?), isn't that a pretty fundamental speedline bug? |
Yes and the fact that we have no test that even noticed that we're changing this is exactly what's concerning. We really need plots up and running... |
|
ah, I knew we had talked about this before: paulirish/speedline#38 (to clear up my confusion, navStart isn't used as the start of the interval, the first event in the trace is, whatever that happens to be?) |
|
in speedline the start is always the beginning of the trace which is a quite reasonable default (and is normally pretty close to what we always care about, navstart), given that it's perfectly valid to compute speed index on SPA navigation, modal, etc it seems a bit like imposing LH will on them to change their default when it's already an option we can use, further discussion should probably happen in paulirish/speedline#46 though
A tad harsh on ourselves given all the tests for these sorts of never asserted their correctness but assumed the downstream thing was 100% correct at the basic usage and that's it. The speedline test does indeed catch that the metric has changed but because it's so difficult to interpret I don't think anyone ever sat down and did the math to make sure it was computing the exact correct number. |
|
Aye. timeOrigin was added upstream for this usecase. Ultimately I trust our navStart identification better than someone else, so I think this model is preferred, rather than fixing the navStart detection within speedline. |
|
cool, makes sense |
yeah, I think it's reasonable to expect correct operation from the upstream library and fix bugs there when it turns out things are incorrect (or land features like But :) In theory this change reduces variance in our audit results, however we have no system set up to alert us that "hey, PR #2114 measurably reduced variance by eliminating some small percentage of wildly incorrect PSI numbers". The corollary is what's concerning: we have no system to alert us that "Hey, PR #x measurably increased variance by introducing wildly some small percentage of wildly incorrect test numbers". I might set up a cron job to pull from master and run plots/ once a day so we can at least bisect. Unless that's already in progress? |
fixes #2095
See issue for background. I wish we had a sample trace to add a test for this, but I haven't been able to grab one. speedline already supports custom reference points so this should resolve the extreme case noted in #2095