core(network-request): loosen lightrider timing checksum #15330
Conversation
| @@ -502,11 +502,20 @@ class NetworkRequest { | |||
| const requestMs = requestMsHeader ? Math.max(0, parseInt(requestMsHeader.value)) : 0; | |||
| const responseMs = responseMsHeader ? Math.max(0, parseInt(responseMsHeader.value)) : 0; | |||
|
|
|||
| // Bail if the timings don't add up. | |||
| if (TCPMs + requestMs + responseMs !== totalMs) { | |||
| if (Number.isNaN(TCPMs + requestMs + responseMs + totalMs)) { | |||
There was a problem hiding this comment.
Not a new requirement, but now checking for NaNs explicitly because the below code no longer does that implicitly.
There was a problem hiding this comment.
nit: What if this just checked Number.isNaN(totalMs)? Then the next check would verify the rest. Up to you on which way is more readable. I just thought it isn't obvious that the isNaN gets distributed.
There was a problem hiding this comment.
I just thought it isn't obvious that the isNaN gets distributed.
It's the only property of NaN! :)
Any operation on NaN is still NaN.
There was a problem hiding this comment.
Except for NaN ** 0 apparently lol https://stackoverflow.com/a/17865241
| @@ -502,11 +502,20 @@ class NetworkRequest { | |||
| const requestMs = requestMsHeader ? Math.max(0, parseInt(requestMsHeader.value)) : 0; | |||
| const responseMs = responseMsHeader ? Math.max(0, parseInt(responseMsHeader.value)) : 0; | |||
|
|
|||
| // Bail if the timings don't add up. | |||
| if (TCPMs + requestMs + responseMs !== totalMs) { | |||
| if (Number.isNaN(TCPMs + requestMs + responseMs + totalMs)) { | |||
There was a problem hiding this comment.
nit: What if this just checked Number.isNaN(totalMs)? Then the next check would verify the rest. Up to you on which way is more readable. I just thought it isn't obvious that the isNaN gets distributed.
#15075 came back because we still sometimes have no timings, because of the checksum with used to verify the data as reasonable. While comments in the proto do suggest they should be equal, in practice we saw up to a 5ms difference.
Let's not drop everything when they don't match. Instead, reset the total time to be the sum of the components. Perhaps the stages internally are being done in parallel somehow, or they is some rounding issue, or the boundaries between these stages accidentally have gaps. Whatever the reason, we can afford to be somewhat lenient here.
b/283153039