Improvements after testing on 2G/3G/4G networks #33
Conversation
Support for metadata was added as part of #31. Bump client minor because we changed the settings structure.
I would like to see logs similar to the logs typically printed by Apache for access.log as far as the HTTPS port is concerned. For other ports, I would argue it does not matter much because they are basically non standard (others may disagree).
Allows us to update the UI while the testing is running. The previous code blocked the user interface until the test was complete. While there, remove now unused ndt7 options: adaptive and duration.
As we've just discussed with other NDT developers at M-Lab, better to run for ten seconds at this stage to learn how to best stop early.
We need such ID inside the table body. Using it also for the whole table is incorrect, even if it works.
Allows to unify the code for dealing with missing BBR support on Linux with the code for all other systems. Means we can get rid of the concept of "tolerate weird failures when trying to enable BBR because we MAY be on Linux". Really, just simplifies code.
Testing in several 2G/3G/4G scenarios, I have noticed that just one second of timeout is not enough. Better to raise it. I have picked seven because it is a number I like. It could also have been four. I'd say we can adjust this further when there's more data from more clients from diverse networks.
The client.go file will eventually become a regress test, so server.go is a better place to put that constant in.
[I recommend to use `git show -w` to inspect this diff.]
This commit changes the specification to provide the server the
choice of using either binary or textual messages to generate
network load. The rationale of using binary messages is that it
is more efficient. The rationale of using textual messages is
that every message received by the client is a meaningful piece
of measurements that can be used to perform inferences.
Since measurement messages are too small to generate network
load, I've modified the specification to add random padding to
JSON messages. In this way, messages are large enough.
I have also modified the server to use textual messages and
padding. While in general I believe one MAY want to have a
server sending binary messages, after several tests in {3,4,5}G
networks, I've come to the conclusion that:
- either we further complicate the server implementation with
more than a single goroutine for the sender, such that we
collect (and store) measurements every N milliseconds
- or we use padding an every message is meaningful
The specific issue is that, with a slow 2G/3G network, the
sender is blocked in sending the binary messages and does
not have many chances of sampling BBR variables before the
client closes the connection because it's running for too
long (consider that a 10 second test runs for around two
minutes on my 2G network).
Therefore, one possible choice would have been to have a
goroutine for sampling BBR variables. But, still, even with
this sampling, the client would not receive many updates
and much information.
On the contrary, if each send() carries also a measurement,
the user experience is much more pleasant because one at
least sees a stream of measurements, albeit delayed by the
buffering queue in the operator's 2G machinery.
(IIRC @pboothe proposed this strategy to me when we first
discussed ndt7. I initially choose to go for binary messages
because it looked like more efficient, however, putting
measurements and data together turned out to be a bit smarter,
because there is no head-of-line-blocking problem.)
This seems to provide a more pleasant UX.
Pull Request Test Coverage Report for Build 245
💛 - Coveralls |
While that was already clear in the documentation, it was not explicit in variable names and in JSON message fields. Yesterday, talking to @mattmathis, I made the mistake of considering the min-RTT being the RTT, which led me to draw wrong conclusions regarding a connection. For this reason, I have decided to go a bit more explicit with variable names. If the person who wrote the code, i.e. me, wrongly interprets variables, this may happen as well to someone less familiar with the codebase. Hence, it is clearly better to have a much less ambiguous naming. No need to bump the specification version because it has already been bumped as part of this PR.
Since version 0.3.0 of the spec, there is no min-measurement-interval as it's possible to use just measurement messages to load the net. Hence, just move the variable inside the client.go file. Sometime in the future, client.go will be moved in another place and become a unit test for the server.
There was a problem hiding this comment.
I am sorry this took me so long to get to.
It looks good to me, so 
Reviewed 2 of 10 files at r1, 4 of 9 files at r2.
Reviewable status:complete! 1 of 1 LGTMs obtained
html/libndt7.js, line 62 at r2 (raw file):
} url += '/ndt/v7/download?' for (let key in settings.meta) {
I recommend the URL and URLSearchParams classes here. They will ensure that you don't make malformed URLs and that you don't have to think about whether your URL is valid, and they will URLEncode any non-ASCII (but legitimate) values like the Chinese name of the Chinese version of Windows.
|
@pboothe, thanks for your review! I have been seriously swamped lately because the upcoming end of my fellowship, so I could not add the requested changes yet. |
1. use `URL` and `URLSearchParams` 2. notice that we can simplify further and just pass around `window.location.href` as the base URL 3. disable a `console.log` statement that, with an open console, was tearing down my browser See: #33 (review)
|
@pboothe I have implemented you suggestions, please take a look! |
|
Then changes look good, although perhaps things can be even 4 lines simpler in the JS? However you decide to go, this change looks good to me. |
html/libndt7.js
Outdated
| if (typeof value === 'number' || typeof value === 'boolean') { | ||
| value += '' // force conversion to string | ||
| } | ||
| if (typeof value !== 'string' || !value.match(re)) { |
There was a problem hiding this comment.
I think we should be okay with unicode values and that it is okay to remove the value.match(re) check now that we can be sure that the URLencoding will be okay in all circumstances.
There was a problem hiding this comment.
Whether or not unicode keys make sense, I leave to you. But these checks feel redundant to me.
There was a problem hiding this comment.
So, my understanding is that strings in JavaScript are UTF-16. Reading the polyfill code for URLSearchParams, I additionally understand that it should call encodeURIComponent. In turn, that should serialise to UTF-8.
Is my understanding correct?
I am concerned that JavaScript strings are UTF-16 while Go strings are UTF-8. So, I would like to be sure that removing the checks enforcing ASCII we do not end up with URL params that we cannot use in Go.
(This is probably just me being paranoid and not knowing enough about the Web.)
There was a problem hiding this comment.
As a clarification, I am not thinking about using such params to build file names. But my code in another branch is serialising such params to JSON as part of saving results. Hence my concern before applying the objective improvement that you suggest. (As a note to self, if I do that, I shall also update the spec.)
There was a problem hiding this comment.
package main
import (
"encoding/json"
"fmt"
"log"
"net/url"
"strings"
)
func main() {
m, err := url.ParseQuery(`x=1&y=2&w=%D1%88%D0%B5%D0%BB%D0%BB%D1%8B&y=3;z`)
if err != nil {
log.Fatal(err)
}
fmt.Println(toJSON(m))
}
func toJSON(m interface{}) string {
js, err := json.Marshal(m)
if err != nil {
log.Fatal(err)
}
return strings.Replace(string(js), ",", ", ", -1)
}results in:
{"w":["шеллы"], "x":["1"], "y":["2", "3"], "z":[""]}
So it looks like UTF16 bytes are handled by Go just fine.
There was a problem hiding this comment.
Excellent, thanks! So, I'll remove these four lines and merge :-)
1. use `URL` and `URLSearchParams` 2. notice that we can simplify further and just pass around `window.location.href` as the base URL 3. disable a `console.log` statement that, with an open console, was tearing down my browser See: m-lab/ndt-server#33 (review)
I've run some more tests in 2G/3G/4G networks. This led me to the following improvements:
using a worker for the web client, so the UI is not locked
modify the protocol to allow for sending measurement messages with padding, so that the client receives more measurements regardless of the delay caused by 2G/3G/4G buffers
other, smaller, miscellaneous, useful fixes
I have also experimented with
TCP_NOTSENT_LOWATbut that diff is not ready to be included in this PR yet, and I feel like the code in here has been delayed for too much time already. I'll probably ask about this option to @mattmathis later today or in the next days.This change is