Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests(dbw): fix server latency flake #15729

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Jan 4, 2024

This has been happening a lot since this condition was added. Not entirely sure how this can happen. Maybe there is just higher variance since the sample size for the domain is just 1 request? Edit: see #15729 (comment)

@connorjclark
Copy link
Collaborator

Just in case there's a real bug here, I dug in a bit..

I grabbed the failing artifacts from: https://github.com/GoogleChrome/lighthouse/actions/runs/7415363551

And set a breakpoint in NetworkAnalysis, then ran node cli '-A=/Users/cjamcl/Downloads/Smokehouse (ubuntu; chrome stable)/2/dbw/' http://localhost:10200/dobetterweb/dbw_tester.html

the summary for this origin is all 0
image

Though we do have a RTT for it

image

Back to the summary timings - We only have one entry for the [::1] domain, which gets capped to 0 in _estimateResponseTimeByOrigin:

image

And that RTT estimate comes from one sample (we only have one network record for this origin...), which comes from this estimation:

return connectEnd - connectStart;

image

(protocol is http/1.1 btw)

I guess this code is pretty flawed for n=1 samples, but I'm not sure how exactly or if it extends to when there are more samples. We could try adding another request to this domain, but that may just be masking a problem.

@connorjclark
Copy link
Collaborator

connorjclark commented Jan 4, 2024

from my understanding, this being zero makes sense to me because: we must guess RTT w/ the worse case data, no SSL timing and just one sample. then we must derive the "server thinking time" which is the ttfb minus the rtt...and if this is all from the same record that's just not enough info to get what we want.

@adamraine
Copy link
Member Author

Yeah seems like a response time of 0 makes sense in this scenario. Doesn't seem like a problem, it just doesn't match the expectation for the other domain.

@devtools-bot devtools-bot merged commit 30d6200 into main Jan 4, 2024
29 checks passed
@devtools-bot devtools-bot deleted the fix-dbw-server-latency-flake branch January 4, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants