spec/ndt7: address several outstanding issues

[bassosimone]

This modification of the ndt7 spec is non backward compatible
therefore I have bumped the version accordingly.

When reading the spec after this changes have been applied, I am now
confident that we can make a first public release soon.

The following list describes what changed:

1. bumped the version number

2. added a non-normative section describing the design choices of
   ndt7 as recently discussed with @pboothe and @mattmathis

3. reorganized the protocol description to group handshake-related
   information and websocket related information

4. [API CHANGE] now the client MUST include an user-agent
   during the websocket handshake

5. [API CHANGE] removed ambiguity on how the server SHOULD process
   query string keys without a value: now the server SHOULD consider
   them like keys with an empty string as value

6. [API CHANGE] do not force the server to use 400 to indicate
   handshake failure, be relaxed and use 4xx

7. [API CHANGE] now the server MUST parse the query string

8. [API CHANGE] now the server MUST store the query string

9. reword the section where we describe usage of binary and textual
   websocket messages

10. [API CHANGE] say that implementations SHOULD follow the websocket
    RFC but concede that what they really MUST do is to be robust
    with respect to receiving errors once the test is started. This
    allows us to implement protections such that the duration of a
    test is precise as well as to allow servers to arbitrarily stop
    a transfer earlier than ten seconds if they need that.

11. [API CHANGE] switch to CamelCase for the measurement message

12. [API CHANGE] only document the minimum set of TCP_INFO variables
    that seem useful to OONI or Asurion and explicitly say that any
    other variable that MAY be there may disappear without notice

13. [API CHANGE] use natural kernel measurement units, e.g. bytes and
    microsecond, and do that consistently across the structure

14. [API CHANGE] clarify ConnectionInfo and add some extra optional
    fields the Go client was already using so that it will not conflict
    with future variables with same name and different semantics

15. add a section with an explicit example describing the messages
    exchanged both when there are and there are not measurement messages

16. clarify in mlab-ns section that we'll perform A/B testing and that
    implementing a client compatible with the specification is good
    precisely because it gives us flexibility in that respect

17. improve the example code describing exponential backoff to clearly
    indicate that exponential backoff is only to be done when mlabns
    returns the no-content signal

18. improve the reference implementations section to mention that
    simplified clients exist in this repository and to clarify that
    their main purpose is to test against this server

19. write an appendix section that is non normative and mention (1)
    the algorithm proposed by @pboothe for scaling the message size and
    (2) how the TCP_INFO variables included into this specification
    could be useful to people.

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 983

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.6%) to 75.151%

---

Totals
Change from base Build 972:	0.6%
Covered Lines:	1491
Relevant Lines:	1984

---

💛 - Coveralls

[pboothe]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone and @pboothe)

---

spec/ndt7-protocol.md, line 17 at r1 (raw file):

Ndt7 measures the application-level download and upload performance
using WebSockets over TLS. Each measurement is an independent so-called

Each test type is independent, and there are two types of test.

("measurement" is ambiguous as it could mean an instantaneous measurement or a whole-connection measurement. "test" is harder t confuse in this way.)

---

spec/ndt7-protocol.md, line 19 at r1 (raw file):

using WebSockets over TLS. Each measurement is an independent so-called
subtest; there are two subtests: the download and the upload subtests. Ndt7
always uses a single TCP connection. Where possible, ndt7 uses a recent

Whenever possible

---

spec/ndt7-protocol.md, line 20 at r1 (raw file):

subtest; there are two subtests: the download and the upload subtests. Ndt7
always uses a single TCP connection. Where possible, ndt7 uses a recent
version of TCP BBR. Writing a ndt7 client should always be easy. A minimal

Writing an ndt7 client is designed to be as simple as possible. A minimal Go language ndt7 client that performs a download test has been implemented in just XXX lines, using no code from the NDT server, and a minimal javascript client has been implemented in just YYY lines. It is a design goal of ndt7 that the size of a minimal client remains small over time.

---

spec/ndt7-protocol.md, line 31 at r1 (raw file):

connection (landline, Wi-Fi, 4G, etc.), the characteristics of
your ISP and possibly of other ISPs in the middle, and the server
being used. Also, the main measurement performed by ndt7 does not include

"Does not include" confuses me. Does this mean that the measurements do not factor these out? Does this mean that it is strictly a measure of goodput after all of these protocol pieces have added their overhead?

---

spec/ndt7-protocol.md, line 38 at r1 (raw file):

The presence of network issues (e.g. interference or congestion) should
cause ndt7 to yield "bad" measurement results, where bad is implicitly

replace 'cause ndt7 to yield "bad"...' with:

cause ndt7 to yield worse measurement results, relative to the expected speed of the end-to-end connection. The expected speed of a BBR connection on an unloaded network is the bitrate of the slowest hop in the measurement path, and the slowest hop is usually also the last hop. Extra ...

---

spec/ndt7-protocol.md, line 45 at r1 (raw file):

be the root cause of such performance issues.

Ndt7 should be easily extensible and future proof. We want to add new

Extensible? We don't want it to be extensible. We want it simple. Let's talk about this paragraph.

---

spec/ndt7-protocol.md, line 53 at r1 (raw file):

Ndt7 should consume few resources. The maximum runtime of a subtest should
be ten seconds, but the server should be able to determine if the performance

if performance has stabilized in less than ten seconds and end the test early.

---

spec/ndt7-protocol.md, line 54 at r1 (raw file):

Ndt7 should consume few resources. The maximum runtime of a subtest should
be ten seconds, but the server should be able to determine if the performance
would not change significantly and interrupt the subtest early. This could also

Delete "this could be partly implemented by clients" sentence.

---

spec/ndt7-protocol.md, line 78 at r1 (raw file):

identifies ndt7, which is `net.measurementlab.ndt.v7`. The URL in the
upgrade request MAY contain optional query-string parameters, which
will be saved as metadata describing the client. The client MUST also

How will the server enforce this? If it is not enforced, then it is SHOULD.

---

spec/ndt7-protocol.md, line 148 at r1 (raw file):

SHOULD close the underlying TLS connection.

Binary messages SHOULD contain 1 << 13 bytes by default. An implementation

MUST be of a size that is a power of two, and the size must be between...

[pboothe]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone)

---

spec/ndt7-protocol.md, line 31 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

"Does not include" confuses me. Does this mean that the measurements do not factor these out? Does this mean that it is strictly a measure of goodput after all of these protocol pieces have added their overhead?

Use the word goodput here

---

spec/ndt7-protocol.md, line 45 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Extensible? We don't want it to be extensible. We want it simple. Let's talk about this paragraph.

To ensure that clients continue to work, we make the design choice that clients should be maximally simple, and that all complexity should implemented on the server side.

---

spec/ndt7-protocol.md, line 107 at r1 (raw file):

is missing). Of course, also the query string MUST be parsed
and the upgrade MUST fail if the query string is not parseable
or not acceptable. The server MUST store the metadata sent by

SHOULD

---

spec/ndt7-protocol.md, line 148 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

MUST be of a size that is a power of two, and the size must be between...

Say what they MUST do, then say what they SHOULD do.

Updated language:

MUST be between XXX and YYY bytes in size, and SHOULD be a power of two and SHOULD by default initially be of size 1<<13

---

spec/ndt7-protocol.md, line 157 at r1 (raw file):

been running for at least fifteen seconds, an implementation MAY close the
underlying TLS connection. This is allowed to keep the overall duration
of each subtest within a fifteen-seconds upper bound. Ideally this SHOULD

12 or 13 seconds is preferable, IMO

---

spec/ndt7-protocol.md, line 172 at r1 (raw file):

while the client SHOULD wait and see whether the server closes it first.

Yet, in practice, both the client and the server SHOULD tolerate any

Remove "Yet,"

---

spec/ndt7-protocol.md, line 175 at r1 (raw file):

abrupt EOF, RST, or timeout/alarm error received when doing I/O with the
underlying TLS connection. Such events SHOULD be logged as warnings
and SHOULD be recorded into the results. Yet, for robustness, we do not

Remove "Yet,"

---

spec/ndt7-protocol.md, line 187 at r1 (raw file):

provide information useful to diagnose performance issues.

While in theory we could specify all `TCP_INFO` and `BBR_INFO` variables,

While in theory we could specify all TCP_INFO and BBR_INFO variables, different kernel versions provide different subsets of these measurements and we do not want to be needlessly restrictive regarding the underlying kernel for the server. Instead, our guiding principle is...

---

spec/ndt7-protocol.md, line 263 at r1 (raw file):

  has been performed by the client or by the server. This field SHOULD
  only be used when the entity that performed the measurement would otherwise
  be ambiguos;

"ambiguous"

Also, please end list items with a period. Some of them contain multiple sentences, which makes a semicolon confusing..

---

spec/ndt7-protocol.md, line 312 at r1 (raw file):

contain more `TCP_INFO` fields. Yet, only the fields described in this
specification are REQUIRED to be returned by a compliant, `TCP_INFO` enabled
implementation of ndt7. You SHOULD NOT rely on other fields.

A client MAY use other fields, but the absence of those fields in a server response MUST NOT be a fatal client error.

---

spec/ndt7-protocol.md, line 315 at r1 (raw file):

Also note that some kernels may not support all the above mentioned `TCP_INFO`
variables. In such case, the unsupported variables SHOULD be set to zero.

-1, not zero.

---

spec/ndt7-protocol.md, line 318 at r1 (raw file):

Moreover, note that all the variables presented above increase or otherwise
change consistently during a subtest. Therefore, the last measurement sample

Therefore, the most recent measurement sample is a suitable summary of all prior measurements, and the last measurement received should be treated as the summary message for the test.

---

spec/ndt7-protocol.md, line 325 at r1 (raw file):

the nearest `float64` value. A pedantic implementation MAY want to be overly
defensive and make sure that it does not emit values that a `int53` cannot
represent. If it decides to handle this OPTIONAL case, then the implementation

is "it" a client or server here? Can the other side tell if they are talking to such a client? If not, then please remove the capitalization of "MUST NOT" which otherwise is a little confusing.

---

spec/ndt7-protocol.md, line 418 at r1 (raw file):

provided by such infrastructure instead.

Clients:

Clients: -> Clients using the M-Lab NDT7 infrastructure:

---

spec/ndt7-protocol.md, line 423 at r1 (raw file):

https://locate.measurementlab.net/) server-discovery web API;

2. MUST query for the `ndt7` locate.measurementlab.net tool (see below for a

SHOULD. We can't enforce this.

---

spec/ndt7-protocol.md, line 426 at r1 (raw file):

description of how a real request would look like);

3. MUST set `User-Agent` to identify themselves;

SHOULD. We can't enforce this.

---

spec/ndt7-protocol.md, line 431 at r1 (raw file):

as the no-capacity signal (see below) and any other status as failure;

5. MUST NOT assume that the locate service would redirect them to a nearby,

MUST NOT assume that the locate service will always geographically locate them to the same server, or always to the strictly nearest server, or to a server running a particular version of the code, or to a server that will always run a 10 second test instead of a shorter test. Rollouts on the M-Lab platform happen gradually, and canary tests of new server versions happen regularly, which means that different servers may be running different code versions. Well-written clients should already be robust to these scenarios, but we note them here as an additional reminder.

---

spec/ndt7-protocol.md, line 581 at r1 (raw file):

https://github.com/m-lab/ndt-server). Such repository also contains a
simplified Go client implementation and a simplified JavaScript implementation,
which SHOULD only be used for testing server implementations.

, the library for which may be useful to others in building their own speed test websites. This code SHOULD also be used to test server implementations, and as a very basic client for occasional use.

---

spec/ndt7-protocol.md, line 634 at r1 (raw file):

surprisingly non-small

large

---

spec/ndt7-protocol.md, line 636 at r1 (raw file):

we're `SndBufLimited`

SndBufLimited is large

---

spec/ndt7-protocol.md, line 637 at r1 (raw file):

buffering to go faster and it is limiting our performance. Likewise, when
we're `SndBufLimited`, the sender's buffer is too small. (Because the kernel
may be autoscaling buffers, it may be that we need to use `sysctl` or other

Is "we" the server or the client here?

---

spec/ndt7-protocol.md, line 663 at r1 (raw file):

packet is uniformly distributed, which isn't likely the case. Yet, it
may be an useful first order information to characterise a network
as possibly very lossy.

Add text:
Some packet loss is normal and healthy, but too much packet loss is the sign of a network path with systemic problems.

[pboothe]

Back to you.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone)

[bassosimone]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe)

---

spec/ndt7-protocol.md, line 17 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Each test type is independent, and there are two types of test.

("measurement" is ambiguous as it could mean an instantaneous measurement or a whole-connection measurement. "test" is harder t confuse in this way.)

Done.

---

spec/ndt7-protocol.md, line 19 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Whenever possible

Done.

---

spec/ndt7-protocol.md, line 20 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Writing an ndt7 client is designed to be as simple as possible. A minimal Go language ndt7 client that performs a download test has been implemented in just XXX lines, using no code from the NDT server, and a minimal javascript client has been implemented in just YYY lines. It is a design goal of ndt7 that the size of a minimal client remains small over time.

Done. I made sure my numbers were correct by updating existing clients to use the last version of the spec. The Go client (with full error handling) is 151 lines. The JavaScript client (with enforcing of timeout using Workers) is 122 lines. Both implementations scale messages and have been seen performing at ~1 Gbit/s.

---

spec/ndt7-protocol.md, line 31 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Use the word goodput here

Done.

---

spec/ndt7-protocol.md, line 38 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

replace 'cause ndt7 to yield "bad"...' with:

cause ndt7 to yield worse measurement results, relative to the expected speed of the end-to-end connection. The expected speed of a BBR connection on an unloaded network is the bitrate of the slowest hop in the measurement path, and the slowest hop is usually also the last hop. Extra ...

Done.

---

spec/ndt7-protocol.md, line 45 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

To ensure that clients continue to work, we make the design choice that clients should be maximally simple, and that all complexity should implemented on the server side.

Done.

---

spec/ndt7-protocol.md, line 53 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

if performance has stabilized in less than ten seconds and end the test early.

Done.

---

spec/ndt7-protocol.md, line 54 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Delete "this could be partly implemented by clients" sentence.

Done.

---

spec/ndt7-protocol.md, line 78 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

How will the server enforce this? If it is not enforced, then it is SHOULD.

Done.

---

spec/ndt7-protocol.md, line 107 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

SHOULD

Done.

---

spec/ndt7-protocol.md, line 148 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Say what they MUST do, then say what they SHOULD do.

Updated language:

MUST be between XXX and YYY bytes in size, and SHOULD be a power of two and SHOULD by default initially be of size 1<<13

Done.

---

spec/ndt7-protocol.md, line 157 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

12 or 13 seconds is preferable, IMO

Done.

---

spec/ndt7-protocol.md, line 172 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Remove "Yet,"

Done.

---

spec/ndt7-protocol.md, line 175 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Remove "Yet,"

Done.

---

spec/ndt7-protocol.md, line 187 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

While in theory we could specify all TCP_INFO and BBR_INFO variables, different kernel versions provide different subsets of these measurements and we do not want to be needlessly restrictive regarding the underlying kernel for the server. Instead, our guiding principle is...

Done.

---

spec/ndt7-protocol.md, line 263 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

"ambiguous"

Also, please end list items with a period. Some of them contain multiple sentences, which makes a semicolon confusing..

Done.

---

spec/ndt7-protocol.md, line 312 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

A client MAY use other fields, but the absence of those fields in a server response MUST NOT be a fatal client error.

Done.

---

spec/ndt7-protocol.md, line 315 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

-1, not zero.

I don't like the suggestion of setting to -1. I have changed the wording to say that we know that m-lab will always return these variables and if you use 4.19+ you are fine, but if you use an older kernel or you don't have tcp_info on the system you should accordingly omit the variables you know are not supported or the whole of TCPInfo section of the message

---

spec/ndt7-protocol.md, line 318 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Therefore, the most recent measurement sample is a suitable summary of all prior measurements, and the last measurement received should be treated as the summary message for the test.

Done.

---

spec/ndt7-protocol.md, line 325 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

is "it" a client or server here? Can the other side tell if they are talking to such a client? If not, then please remove the capitalization of "MUST NOT" which otherwise is a little confusing.

Removed the entire sentence. What was said before was already enough.

---

spec/ndt7-protocol.md, line 418 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Clients: -> Clients using the M-Lab NDT7 infrastructure:

Done.

---

spec/ndt7-protocol.md, line 423 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

SHOULD. We can't enforce this.

Done.

---

spec/ndt7-protocol.md, line 426 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

SHOULD. We can't enforce this.

Done.

---

spec/ndt7-protocol.md, line 431 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

MUST NOT assume that the locate service will always geographically locate them to the same server, or always to the strictly nearest server, or to a server running a particular version of the code, or to a server that will always run a 10 second test instead of a shorter test. Rollouts on the M-Lab platform happen gradually, and canary tests of new server versions happen regularly, which means that different servers may be running different code versions. Well-written clients should already be robust to these scenarios, but we note them here as an additional reminder.

Done.

---

spec/ndt7-protocol.md, line 581 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

, the library for which may be useful to others in building their own speed test websites. This code SHOULD also be used to test server implementations, and as a very basic client for occasional use.

Done.

---

spec/ndt7-protocol.md, line 634 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

surprisingly non-small

large

Done.

---

spec/ndt7-protocol.md, line 636 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

we're `SndBufLimited`

SndBufLimited is large

Done.

---

spec/ndt7-protocol.md, line 637 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Is "we" the server or the client here?

Removed as it was too technical

---

spec/ndt7-protocol.md, line 663 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Add text:
Some packet loss is normal and healthy, but too much packet loss is the sign of a network path with systemic problems.

Done.

[bassosimone]

All done, please @pboothe take another look!

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe)

[bassosimone]

@pboothe with respect dealing with unknown fields and setting them to -1, I think I have changed my mind. I've just had a chat with @mattmathis where he suggested that there is a very effective way of setting everything to -1, which is memset(buf, 0xff, sizeof (buf)). I didn't think about this and it's very neat and future proof. So, I think -1 is really viable as a suggestion.

[pboothe]

Resolved most comments. Added a few much smaller comments.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone)

---

spec/ndt7-protocol.md, line 148 at r1 (raw file):

Previously, bassosimone (Simone Basso) wrote…

Done.

Message size is a MUST.

---

spec/ndt7-protocol.md, line 157 at r1 (raw file):

Previously, bassosimone (Simone Basso) wrote…

Done.

thirteen second upper bound (no pluralization on seconds - English is a terrible language)

---

spec/ndt7-protocol.md, line 315 at r1 (raw file):

Previously, bassosimone (Simone Basso) wrote…

I don't like the suggestion of setting to -1. I have changed the wording to say that we know that m-lab will always return these variables and if you use 4.19+ you are fine, but if you use an older kernel or you don't have tcp_info on the system you should accordingly omit the variables you know are not supported or the whole of TCPInfo section of the message

The more I think about it, the more I want to say "clients should be robust to missing data. Missing data should not cause a client to crash, although it is allowable for it to cause an inferior user experience." Choosing a sentinel value is just a bad idea.

---

spec/ndt7-protocol.md, line 162 at r3 (raw file):

underlying TLS connection. This is allowed to keep the overall duration
of each test within a thirteen-seconds upper bound. Ideally this SHOULD
be implemented so that immediately after thirteen-seconds have elapsed, the

no dash

---

spec/ndt7-protocol.md, line 183 at r3 (raw file):

gives the ndt7 protocol bizantine robustness, and acknowledges the
fact that the web is messy and a test may terminate more abruptly
than it used to happen in our controlled experiments.

than it used to happen > than happened in the past

---

spec/ndt7-protocol.md, line 265 at r3 (raw file):

      for this test within the Measurement Lab (M-Lab) platform. This field
      SHOULD be omitted by servers running on other platforms, unless they
      also have the concept of an UUID bound to a TCP connection.

an UUID -> a UUID (See previous regarding the terribleness of English)

The UUID is a global thing, and not tied to the M-Lab platform. Other server implementations should construct it from the TCP socket cookie and (optionally) the server hostname and (optionally) a configured unique string.

[bassosimone]

Addressed comments, please take another look!

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone and @pboothe)

---

spec/ndt7-protocol.md, line 162 at r3 (raw file):

Previously, pboothe (Peter Boothe) wrote…

no dash

Done.

---

spec/ndt7-protocol.md, line 183 at r3 (raw file):

Previously, pboothe (Peter Boothe) wrote…

than it used to happen > than happened in the past

Done.

---

spec/ndt7-protocol.md, line 265 at r3 (raw file):

Previously, pboothe (Peter Boothe) wrote…

an UUID -> a UUID (See previous regarding the terribleness of English)

The UUID is a global thing, and not tied to the M-Lab platform. Other server implementations should construct it from the TCP socket cookie and (optionally) the server hostname and (optionally) a configured unique string.

ah, right, a "ju-ju-ai-di"

[bassosimone]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe)

---

spec/ndt7-protocol.md, line 315 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

The more I think about it, the more I want to say "clients should be robust to missing data. Missing data should not cause a client to crash, although it is allowable for it to cause an inferior user experience." Choosing a sentinel value is just a bad idea.

Forgot to reply here; also this is done.

[pboothe]

Reviewable status: complete! 1 of 1 approvals obtained