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

Round Trip Latency implemention varies from RFC spec #10

Open
virtuallynathan opened this issue Mar 23, 2022 · 4 comments
Open

Round Trip Latency implemention varies from RFC spec #10

virtuallynathan opened this issue Mar 23, 2022 · 4 comments

Comments

@virtuallynathan
Copy link

virtuallynathan commented Mar 23, 2022

go func() {
roundTripCount := uint16(0)
before := time.Now()
/*
TODO: We are not going to measure round-trip times on the load-generating connection
right now because we are dealing with a massive amount of buffer bloat on the
Apple CDN.
c_a, err := saturated_client.Get(url)
if err != nil {
responseChannel <- GetLatency{Delay: 0, RTTs: 0, Err: err}
return
}
// TODO: Make this interruptable somehow
// by using _ctx_.
_, err = io.ReadAll(c_a.Body)
if err != nil {
responseChannel <- GetLatency{Delay: 0, RTTs: 0, Err: err}
return
}
roundTripCount += 5
c_a.Body.Close()
*/
c_b, err := new_client.Get(url)
if err != nil {
responseChannel <- GetLatency{Delay: 0, RoundTripCount: 0, Err: err}
return
}
// TODO: Make this interruptable somehow by using _ctx_.
_, err = io.ReadAll(c_b.Body)
if err != nil {
responseChannel <- GetLatency{Delay: 0, Err: err}
return
}
c_b.Body.Close()
// We use 1 here according to the wording in 4.2.1.
roundTripCount += 1
responseChannel <- GetLatency{Delay: time.Since(before), RoundTripCount: roundTripCount, Err: nil}

https://twitter.com/cpaasch11/status/1506730000644390916?s=20&t=rx0uKGBfvmLYvRyx1zKQ0Q

Currently, the code uses the time for the whole client.get() and response.ReadAll(body). Per the RFC, DNS Time, TCP time, TLS time, unloaded HTTP/2 time, and loaded HTTP/2 time should be used, creating an average of all of these values. These values can be obtained using net/http/httptrace WithClientTrace.


4.2.1.  Aggregating the Measurements

   The algorithm produces sets of 5 times for each probe, namely: DNS
   handshake, TCP handshake, TLS handshake, HTTP/2 request/response on
   separate (idle) connections, HTTP/2 request/response on load-
   generating connections.  This fine-grained data is useful, but not
   necessary for creating a useful metric.

   To create a single "Responsiveness" (e.g., RPM) number, this first
   iteration of the algorithm gives an equal weight to each of these
   values.  That is, it sums the five time values for each probe, and
   divides by the total number of probes to compute an average probe
   duration.  The reciprocal of this, normalized to 60 seconds, gives
   the Round-trips Per Minute (RPM).

@virtuallynathan virtuallynathan changed the title Round Trip Latency Definded impleneted differently than RFC spec Round Trip Latency implemention varies from RFC spec Mar 23, 2022
@virtuallynathan
Copy link
Author

I think the answer lies in https://pkg.go.dev/net/http/httptrace#WithClientTrace

@hawkinsw
Copy link
Member

I think the answer lies in https://pkg.go.dev/net/http/httptrace#WithClientTrace

Thank you!! That's exactly what @cpaasch and I settled on.

What's interesting is that there is a reasonable "hack" that could be done in the interim.

The time it takes to complete the entire process of doing an 1-byte HTTP GET using a fresh connection between the client and server is effectively the sum of the time it takes to perform 4 of the 5 of "these values". Moreover, because of an issue with the Apple CDN, we are already effectively ignoring that 5th and final value (until the issue is resolved). Therefore, the value that the go client calculates is already essentially:

That is, it sums the five time values for each probe ...

@cpaasch and I are also discussing the importance of defining "probe" in Section 4.2. "probe" and "RTT" seem to be confusingly (at least for my little brain) used in the context of calculating the RPM and it is not clear whether the RPM is the number of probes (each of which include 5 RTTs) per minute or whether it is the number of RTTs per minute. @cpaasch is going to help us clarify this confusion so that it is clearer for people like me who are clearly not as smart as he and the other authors.

All that is to say: I sincerely appreciate your feedback and we are definitely going to use the trace library that you suggested in order to fix the RPM calculation!

Thank you again!
Will

hawkinsw added a commit that referenced this issue Mar 26, 2022
See Issue #10.

Begin implementing the plumbing required to perform more precise RTT/RPM
measurements.
@hawkinsw
Copy link
Member

FYI: The beginning of the work needed to resolve this issue has been committed. See e563d7f

@hawkinsw
Copy link
Member

hawkinsw commented May 5, 2022

We are starting to address this: 2a9feb8 I hope that it helps!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants