core(network-analyzer): coarse rtt estimate on per-origin basis

[connorjclark]

Currently, network-analyzer skips doing a coarser RTT estimate if any origin has connection timing data. This results in us having no estimates for some origins.

This can occur if the page preconnects to an origin. The first request, if Chrome eagerly connected, will use an already established connection (which means we miss out on that timing info).

Or: consider a timespan where a request is sent to an origin with an already established connection from before the timespan. If there was a new origin connected to during this timespan, I believe we'd then lose any rtt estimate for the already established connection (as the coarse estimate path would not run).

Instead, we should split the records by origin first and then estimate each origin's records in isolation, only falling back to coarser when no connection timing is present for a specific origin.

Some snapshot changes in metrics-tests. Example: amp-m86.devtoolslog.json has a preconnect for fonts.gstatic.com, so the previous rtt estimate gave this origin a 0 (missing) estimate. Now it's {min: 0.024499999999999997, max: 4.13791712771959, avg: 1.6874723759065302, median: 0.8999999999999999}. The other one changed for the same reason.

[connorjclark]

Without these changes, result.get('https://example2.com/') is undefined.

[brendankenny]

is that better or worse than DEFAULT_SERVER_RESPONSE_TIME = 30? It's not clear from this test if it's a good estimate or not

[connorjclark]

Whether it's a better estimate or not seems besides the point for the purposes of this specific unit test - the input data is all made up (and we'd have more than just sendStart - this is using just one estimator). But the larger point of "is this actually better" is clear. Maybe the coarse estimations are just totally bonkers and we were better off when we skipped it.

15 is the result of sendStart / 3 roundtrips = 50; 50 * 0.3 fudge factor = 15. There are no notes on how the 0.3 fudge factor is derived, but that's the one knob we have here (tho we could give each estimator its own fudge factor). We should probably do some analysis of tons of records and see how known-rtt (when SSL and Connect timings are present) correlates with the various strategies used for coarse estimations. Is that worth the effort?

[adamraine]

Looks great! Thanks for digging into this!

[adamraine]

_estimateValueByOrigin still has one usage in _estimateResponseTimeByOrigin and shares a lot of the same implementation details as this function. Is it possible to create a common collectEstimates function used here and in _estimateResponseTimeByOrigin? Then we can remove _estimateValueByOrigin entirely.

[connorjclark]

Not without adding indirection which would make the main use case here harder to understand (in other words, closures are useful).

Not that it couldn't be done. I think it should be a separate PR though, and not very high priority.

[brendankenny]

Some snapshot changes in metrics-tests. Example: amp-m86.devtoolslog.json has a preconnect for fonts.gstatic.com, so the previous rtt estimate gave this origin a 0 (missing) estimate. Now it's {min: 0.024499999999999997, max: 4.13791712771959, avg: 1.6874723759065302, median: 0.8999999999999999}. The other one changed for the same reason.

Cn you add a specific test for this? Looking at the changes in the metrics-test snapshot, a similar change to this one but in the opposite direction wouldn't raise a red flag if someone accidentally reverted this (or replaced the trace) and this condition wasn't tested anymore.

[connorjclark]

Cn you add a specific test for this? Looking at the changes in the metrics-test snapshot, a similar change to this one but in the opposite direction wouldn't raise a red flag if someone accidentally reverted this (or replaced the trace) and this condition wasn't tested anymore.

I'm trying to add a new trace fixture from paulirish.com, since there's google fonts there. Same exact preconnect directive as amp-m86. But I'm getting timing values for dnsStart and connectStart, so it looks like the preconnect isn't being used. Any idea why that'd be?

EDIT: I pushed a fixture with a simple page that tries to get Chrome to do a preconnect. Even with just a dns-prefetch, it doesn't seem to work. If you go to the page (comment out the bit that appends the link) and then open
chrome://net-internals/#dns to clear your dns cache, you'd expect reloading the page to put fonts.gstatic.com in the dns cache. According to the DNS tool, it doesn't.

[connorjclark]

I had assumed that dns-prefetch should make dns timings -1, but that is not true. The connection timings (dns, ssl, connect) are only ever -1 all together, when a connection is established and usable for that page from the connection pool - and never piecemeal. Using preconnect should makes dnsStart/End be a very short amount of time (it puts it in your local dns cache, so it should take no more than a couple ms to retrieve it). But even for cached results, the time is still accounted for. (Aside: now that I think of it, including DNS as part of any RTT estimate to the server is nonsensical. Resolving a domain name doesn't even communicate with the server in question - that's the whole point! - and it can take a hugely variable amount of time, for example, if already cached locally or near-local).

I think amp-m86 log had timing information of this for the first request:

{
          "requestTime": 760623.281441,
          "proxyStart": -1,
          "proxyEnd": -1,
          "dnsStart": -1,
          "dnsEnd": -1,
          "connectStart": -1,
          "connectEnd": -1,
          "sslStart": -1,
          "sslEnd": -1,
          "workerStart": -1,
          "workerReady": -1,
          "workerFetchStart": -1,
          "workerRespondWithSettled": -1,
          "sendStart": 0.245,
          "sendEnd": 0.351,
          "pushStart": 0,
          "pushEnd": 0,
          "receiveHeadersEnd": 6.905
        }

Because an iframe had already setup a connection to fonts.google.com, not because it was preconnected.