Fix: Continuity high TTI values reported (mostly when c.tti gets leaked to other type of beacon)

sukratkashyap commented 4 years ago

Hi Team,

What we want

We want c.tti to be calculated accurately on the page load beacon (spa_hard or undefined initiators).

Cases it fails:

Unpredictable cases:

Page is hidden
User has too many apps open (browser itself doesn't get much CPU) etc.

Predictable cases: According to our code, the assumption of the value of c.tti is:

VisuallyReady < c.tti < (page_ready + waitAfterOnLoad)

We try to calculate c.tti after page_ready is fired and for waitAfterOnLoad ms. After which we send page load beacon without c.tti. (Since our assumption equation didn't work for some users)

Now, If afterOnLoad=true

c.tti is possible to be attached to any type of beacon, especially when it fails at page_load. But then the c.tti values reported are very high and very unreliable. I know it's anyways not a good idea to trust them. But this PR tries to somewhat rectify that issue.

Scenario

User goes to a page
c.tti failed to be calculated
Page load beacon is sent without c.tti
** Data in the Timeline class in Continuity flushes its data.
User scrolls up and down on the page. A number of beacon starts getting sent (error, xhr, interactions, clicks etc.)
c.tti is calculated before every beacon being sent (but never has good enough data (for seeing 500ms CPU idle) because the real data gets flushed whenever a beacon is sent)
User then starts doing something else (anything apart from interacting with the page like playing COD mobile)
User comes back to the page after he wins a battle royale.

Another beacon is sent (this time we have quite a lot of data to find the 500 ms CPU).

if (data.fps && (!data.fps[j] || data.fps[j] < TIME_TO_INTERACTIVE_MIN_FPS_PER_INTERVAL)) {
// No FPS or less than 20 FPS during this interval
idleIntervals = 0;
continue;
}

Because we flushed data every time. This condition !data.fps[j] makes the function feel that the page was busy until the time we last started capturing the data. (which is when the User went to play COD mobile)
Hence c.tti will be reported as the last time we started capturing the data. (and it will be a very large value, because of the time he spent scrolling up and down on the page)

Fix

Moving idleIntervals outside the loop. Because we want to remember the number of intervals it was free before flushing out the data.
Adding bucketsVisited to track buckets we have already visited. So that we don't go through the data again for which we didn't see any improvements
Making onBeacon to remove only the data that we have seen. But if c.tti was calculated then flush as we use to before.

nicjansma commented 3 years ago

@sukratkashyap this is a really great explanation and idea. Thank you for putting this together.

So if I'm understanding correctly, one of the major changes here is it makes it so idleIntervals isn't needed to be re-calculated every time as we keep track of the bucketsVisited to kind-of restart the calculation where we left off.

Have you thought about what a test might look like for this? It would be a little complex (and might have a few phases), but I think we could do it with a combination of some of the existing continuity tests plus waiting a bit after onload for the TTI to actually happen.

I don't see any other major concerns with the code. If you have time to build tests for this, that would be great, otherwise we can try to tackle them soon.

nicjansma commented 3 years ago

@sukratkashyap Thank you again for this PR. I've merged these changes and a few others suggested in https://github.com/akamai/boomerang/issues/315, into a new PR https://github.com/akamai/boomerang/pull/326, and have refactored things a bit so it can be unit-test covered.

I'm going to close out this PR, but I've noted your contributions in the other PR.

Thank you!

akamai / boomerang