akamai / boomerang

End user oriented web performance testing and beaconing
http://akamai.github.io/boomerang/
Other
1.86k stars 292 forks source link

Continuity: Make TTI calculation more reliable and performant #326

Open nicjansma opened 3 years ago

nicjansma commented 3 years ago

Addresses a few issues seen in calculating Time To Interactive (TTI) in the Continuity plugin.

This PR refactors the TTI calculation code into a new function called determineTti() (so it's easier to test), and contains fixes proposed by both @sukratkashyap and @scottgifford.

  1. @scottgifford pointed out that when we're running the TTI calculator, if Boomerang initializes after the page is Visually Ready, startBucket could be negative:
var startBucket = Math.floor((visuallyReady - startTime) / COLLECTION_INTERVAL);

This generally doesn't cause a problem with the TTI calculation, as "negative" (missing) buckets will always fail the FPS-is-atleast-20-check, but it's still a loop over negative indicies that could be skipped.

This PR ensures startBucket is not negative before looping:

var startBucket = Math.max(Math.floor((visuallyReady - startTime) / COLLECTION_INTERVAL), 0);
  1. Another issue @scottgifford pointed out is that the whole TTI calculation may be off-by-1 (100ms). I agree with his assessment, and have adjusted the TTI calculation to be at the correct bucket.

  2. @sukratkashyap opened a PR for some improvements to the TTI calculation as well:

Essentially, this allows the TTI calculator to be "restarted" if TTI wasn't found (e.g. it happens post-page-load, so Boomerang re-runs TTI calculation until it happens). These changes improve the performance of the TTI calculation.

@sukratkashyap's PR is great, and just needed some tests.

  1. This also fixes an issue in https://github.com/akamai/boomerang/issues/307, where if PageBusy metrics may have a race condition with TTI calculation. We now break on the final bucket where Page Busy data is missing (if it's being used).

In an effort to make this code more reliable, all of the changes above were moved into a new function determineTti(), which allowed for unit test coverage.

mc-chaos commented 2 years ago

Hi @nicjansma , Could this PR merged? Regards, Sascha

mc-chaos commented 2 years ago

Hi @nicjansma ,

would this PR be merged? Regards, Sascha

SaschaBrechmannVHV commented 1 year ago

Hi @bluesmoon , @ceckoslab , @andreas-marschke , @ashenoy2014 , could this PR be Merged ?

Regards, Sascha