Add summary at the end of the test #39

robertodauria · 2020-02-12T15:35:14Z

This change adds a summary at the end of the ndt7 test.

To achieve that, the ndt7 Client now saves the results of each test in a TestData struct containing:

The latest Measurement sent by the server
The latest Measurement sent by the client
The latest ConnectionInfo sent by the server

These fields are filled while the test is running, since tests can be interrupted at any time and we do not know which Measurement will be the last one.

Here is how the summary looks like, in interactive mode:

$ ./ndt7-client
[...]
         Server: ndt-iupui-mlab3-mil02.measurement-lab.org
         Client: 93.188.101.116
        Latency:    19.8 milliseconds
       Download:   351.9 Mbit/s
         Upload:    55.2 Mbit/s
 Retransmission:    7.98 %

And in -batch mode

$ ./ndt7-client -batch
[...]
{
  "Server": "ndt-iupui-mlab2-mil05.measurement-lab.org",
  "Client": "93.188.101.116",
  "Download": {
    "value": 364.0964194856322,
    "unit": "Mbit/s"
  },
  "upload": {
    "value": 50.754748581668935,
    "unit": "Mbit/s"
  },
  "downloadRetrans": {
    "value": 6.206705982788881,
    "unit": "%"
  },
  "rtt": {
    "value": 18.643,
    "unit": "milliseconds"
  }
}

Also, to allow for summary-only output, a -quiet flag has been introduced to hide everything but the summary and/or errors.

This change is

…nning.

cmd/ndt7-client/main.go

cmd/ndt7-client/internal/emitter/interactive.go

cmd/ndt7-client/main.go

bassosimone

Whether it's -json or -format=json or -batch, I think we should make sure that the documentation is in sync with the accepted command flags. (That said, I find -format=json rather cool.)

Ah, and also, I would be much more pleased if we could keep the coverage to 100%.

cmd/ndt7-client/internal/summary.go

ndt7.go

bassosimone · 2020-02-12T18:10:13Z

ndt7.go

@@ -53,6 +53,14 @@ type testFn = func(
 // by NewClient in the Client.Dialer.HandshakeTimeout field.
 const DefaultWebSocketHandshakeTimeout = 7 * time.Second

+// TestData contains the latest Measurement send by the server and the client,
+// plus the latest ConnectionInfo sent by the server.
+type TestData struct {


I don't quite like this name because I find it too generic. How about LatestMeasurement? (My understanding is that this data structure is that and, after we fix server, we wont need separate ConnectionInfo, do I understand it correctly?)

Your understanding is correct: if ndt-server started sending ConnectionInfo with every measurement, rather than just on the first one, then we could theoretically remove the ConnectionInfo field here and just keep the latest Measurement.

However, I feel like we should not rely on the ndt-server specific implementation but rather follow the protocol specification, which at the moment goes in the opposite direction:

Servers MUST send this message exactly once. Clients SHOULD cache the first received instance of this message, and discard any subsequently received instance of this message.

To clarify: I like the idea of sending ConnectionInfo every time, I just think we should also update the specification.

Yeah, what you say is sensible.

bassosimone · 2020-02-12T18:15:34Z

cmd/ndt7-client/main.go

 	flagNoVerify = flag.Bool("no-verify", false, "skip TLS certificate verification")
 	flagHostname = flag.String("hostname", "", "optional ndt7 server hostname")
 	flagTimeout  = flag.Duration(
 		"timeout", defaultTimeout, "time after which the test is aborted")
+	flagQuiet = flag.Bool("quiet", false, "emit summary and errors only")


I quite liked the suggestion by @pboothe to have -format=json|human|brief because it seems to me this kinda solves the annoying issue we had with composing emitters in libndt. I am also not so happy with breaking existing scripts that may exist, but, after discussing this with @robertodauria, I understand the potential of this -format= design and I am fully on board regarding it. However, please, @robertodauria:

if you don't change to -format=json, there's a comment above mentioning it, can you fix the comment?

if you change the command line arguments, can you also change the docstring and, if needed, the README?

Following a suggestion you gave me in a private chat, I've kept -batch as an alias to -format=json but marked it as deprecated, so it shouldn't disrupt anyone's workflow. Also, I've updated the documentation to reflect these changes.

I would argue, however, that it's hard to provide any kind of API stability guarantee when the client is relatively new, and people directly using master should expect breaking changes, especially at this stage.

Using semantic versioning + advising users to depend on tagged releases would make this a non-issue. Maybe it's something it would be good to discuss with the team?

P.S.: I'll leave it to you to decide if this is solved by the changes I've made and click on "Resolve", the documentation changes look good to me but I might've missed something :)

Before providing a more comprehensive response, let me say that the issue we're dealing with here is mostly theoretical, since just me and @mattmathis probably use this client.

Speaking from a theoretical point of view, there is clearly a tension here between moving quickly and breaking things that people may be using. When there is a single repository for all the source code, you can move more quickly because you know that you are not breaking expectations. When there are multiple repositories this is more complex and I believe, from the theoretical point of view, that deleting immediately old code is bad.

Speaking of good practices here, I believe we should consider what the abseil.io team do in terms of compatibility: "We will support language versions, compilers, platforms, and workarounds as needed for 5 years after their replacement is available, when possible". My suggestion in this regard would be to more modestly strive for keeping the previous public interface for three months. In INTP fashion, of course, here I can also argue that three months equal zero months, because people do not read deprecation notices. Trying to find a synthesis between those two points of view, I'd say we either remove the old interface immediately, or we make it such that people notices the deprecation warning.

(You may also notice that my response here is much more comprehensive than the one I gave you in private as I am making an effort towards not using my system 1 brain when providing feedback. I think specifically the "three months equal zero months" argument is system 2 thinking.)

[...]

Using semantic versioning + advising users to depend on tagged releases would make this a non-issue.

I disagree that it is a non-issue. Assuming semantic versioning is done correctly, a semantic bump only tells you that you are going to have some kind of trouble. (In this regard, I believe API stability is impossible, and for this reason it probably makes sense to never provide strong API stability, i.e., never release v.1.0.0.)

Maybe it's something it would be good to discuss with the team?

This discussion is dangerous material, because certainly it has the potential to go down to a rabbit hole. At the same time, it's also a topic that has to do with engineering, and I may end up learning something, so, yes please!

cmd/ndt7-client/internal/emitter/batch.go

This broke main_test.go with relied on being able to create a custom emitter.JSON with a non-standard writer. I've added NewJSONWithWriter() to still allow that from external packages.

bassosimone

I did another pass and provided some (I hope) useful feedback. There is just one-two places in which I asked you whether you could apply minimal changes, hence using "Request Changes".

cmd/ndt7-client/internal/emitter/humanreadable.go

cmd/ndt7-client/internal/emitter/quiet.go

bassosimone · 2020-02-13T13:23:35Z

cmd/ndt7-client/internal/summary.go

+	Server string
+
+	// IP address of the client.
+	Client string


Would you mind naming this variable ClientIP?

At that point I would also need to rename Server to ServerFQDN, for the same reason.

I originally had ServerFQDN and ClientIP as field names there, which translated to server and client in the JSON and you advised me to avoid using the json:"fieldname" annotation but rather re-use the same field names in the source code.

Did you change your mind, or you're suggesting to have {"ServerFQDN": ..., "ClientIP": ...} in the summary JSON, too?

When I mentioned to avoid using JSON annotations, I said this because we applied a similar change to the ndt-server in the past, so I wanted to help to enforce consistency in this respect. I don't like the fact that in a JSON variables have upper case names, but I understand how the annotation complicates maintainability.

I believe my comment regarding changing the name of the variable was caused by the documentation string, which starts with "IP", which led me to think in terms of the ClientIP, I guess.

Anyway, variable names can be changed at any time and hence it does not matter.

cmd/ndt7-client/internal/summary.go

bassosimone · 2020-02-13T13:26:03Z

cmd/ndt7-client/internal/summary.go

+			}
+		}
+		if dl.Server.TCPInfo != nil {
+			if dl.Server.TCPInfo.BytesSent > 0 {


I appreciate that there are checks for the value being not-zero. Optionally we can write tests for such error condition?

bassosimone · 2020-02-13T13:40:37Z

cmd/ndt7-client/main.go

 	flagNoVerify = flag.Bool("no-verify", false, "skip TLS certificate verification")
 	flagHostname = flag.String("hostname", "", "optional ndt7 server hostname")
 	flagTimeout  = flag.Duration(
 		"timeout", defaultTimeout, "time after which the test is aborted")
+	flagQuiet = flag.Bool("quiet", false, "emit summary and errors only")


Before providing a more comprehensive response, let me say that the issue we're dealing with here is mostly theoretical, since just me and @mattmathis probably use this client.

Speaking from a theoretical point of view, there is clearly a tension here between moving quickly and breaking things that people may be using. When there is a single repository for all the source code, you can move more quickly because you know that you are not breaking expectations. When there are multiple repositories this is more complex and I believe, from the theoretical point of view, that deleting immediately old code is bad.

Speaking of good practices here, I believe we should consider what the abseil.io team do in terms of compatibility: "We will support language versions, compilers, platforms, and workarounds as needed for 5 years after their replacement is available, when possible". My suggestion in this regard would be to more modestly strive for keeping the previous public interface for three months. In INTP fashion, of course, here I can also argue that three months equal zero months, because people do not read deprecation notices. Trying to find a synthesis between those two points of view, I'd say we either remove the old interface immediately, or we make it such that people notices the deprecation warning.

(You may also notice that my response here is much more comprehensive than the one I gave you in private as I am making an effort towards not using my system 1 brain when providing feedback. I think specifically the "three months equal zero months" argument is system 2 thinking.)

[...]

Using semantic versioning + advising users to depend on tagged releases would make this a non-issue.

I disagree that it is a non-issue. Assuming semantic versioning is done correctly, a semantic bump only tells you that you are going to have some kind of trouble. (In this regard, I believe API stability is impossible, and for this reason it probably makes sense to never provide strong API stability, i.e., never release v.1.0.0.)

Maybe it's something it would be good to discuss with the team?

This discussion is dangerous material, because certainly it has the potential to go down to a rabbit hole. At the same time, it's also a topic that has to do with engineering, and I may end up learning something, so, yes please!

cmd/ndt7-client/main.go

bassosimone · 2020-02-13T13:43:06Z

cmd/ndt7-client/main.go

+	}
+
+	s := internal.NewSummary(r.client.FQDN, r.client.Results())
+	r.emitter.OnSummary(s)


I agree that it's tricky to decide whether to show a summary if there was an error

bassosimone · 2020-02-13T13:44:03Z

ndt7.go

 	return ch, nil
 }

+func (c *Client) collectData(ctx context.Context, f testFn, conn websocketx.Conn, outch chan<- spec.Measurement) {


When we were discussing in private this file's implementation, you mentioned that you'd rather see this piece of code written using interfaces rather than in this function-oriented JavaScript-like fashion. Maybe open an issue?

I would like to revisit my opinion about this function-oriented approach after writing the same summary for the ndt5-client (which, AFAIK, is more OO-like?). At the moment I don't feel strongly enough about this to suggest writing it differently.

Sure. I should actually learn to mind my business more. It's natural that the coding style conventions change when the maintainer of the code changes, and this has already happened. I hope you find ndt5 less functional.

bassosimone · 2020-02-13T13:46:40Z

ndt7.go

+		case spec.OriginServer:
+			// The server only sends ConnectionInfo once at the beginning of
+			// the test, thus if we want to know the client IP and test UUID
+			// we need to store it separately.


Edited m-lab/ndt-server#194 (comment) such that we remember to change this code here.

@robertodauria

We noticed how annoying is not doing that with @robertodauria during the review of m-lab/ndt7-client-go#39. As part of such work, we also double checked that every TCPInfo field described by the spec is a meaningful ndt7 summary. As a result we can now close #194.

There should only be one reviewer, removing myself from the line

@robertodauria

We noticed how annoying is not doing that with @robertodauria during the review of m-lab/ndt7-client-go#39. As part of such work, we also double checked that every TCPInfo field described by the spec is a meaningful ndt7 summary. As a result we can now close #194.

…ype. Also, add NewHumanReadableWithWriter(io.Writer).

bassosimone · 2020-02-13T18:40:54Z

I apologise for stepping into the review, which I was not asked.

pboothe

Dismissed @bassosimone and @robertodauria from 9 discussions.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pboothe and @robertodauria)

cmd/ndt7-client/main.go, line 107 at r2 (raw file):

Previously, bassosimone (Simone Basso) wrote…

Before providing a more comprehensive response, let me say that the issue we're dealing with here is mostly theoretical, since just me and @mattmathis probably use this client.

Speaking from a theoretical point of view, there is clearly a tension here between moving quickly and breaking things that people may be using. When there is a single repository for all the source code, you can move more quickly because you know that you are not breaking expectations. When there are multiple repositories this is more complex and I believe, from the theoretical point of view, that deleting immediately old code is bad.

Speaking of good practices here, I believe we should consider what the abseil.io team do in terms of compatibility: "We will support language versions, compilers, platforms, and workarounds as needed for 5 years after their replacement is available, when possible". My suggestion in this regard would be to more modestly strive for keeping the previous public interface for three months. In INTP fashion, of course, here I can also argue that three months equal zero months, because people do not read deprecation notices. Trying to find a synthesis between those two points of view, I'd say we either remove the old interface immediately, or we make it such that people notices the deprecation warning.

(You may also notice that my response here is much more comprehensive than the one I gave you in private as I am making an effort towards not using my system 1 brain when providing feedback. I think specifically the "three months equal zero months" argument is system 2 thinking.)

[...]

Using semantic versioning + advising users to depend on tagged releases would make this a non-issue.

I disagree that it is a non-issue. Assuming semantic versioning is done correctly, a semantic bump only tells you that you are going to have some kind of trouble. (In this regard, I believe API stability is impossible, and for this reason it probably makes sense to never provide strong API stability, i.e., never release v.1.0.0.)

Maybe it's something it would be good to discuss with the team?

This discussion is dangerous material, because certainly it has the potential to go down to a rabbit hole. At the same time, it's also a topic that has to do with engineering, and I may end up learning something, so, yes please!

For this client, this discussion is immaterial. Go change it to the right way and don't worry. When a tool is used outside of m-lab, we should think more deeply. Feel free to delete the -batch flag now or later.

cmd/ndt7-client/internal/emitter/humanreadable.go, line 86 at r7 (raw file):

%15s: %7.1f %s
%15s: %7.1f %s
%15s: %7.2f %s`

nitpick - end the quote character on the next line to avoid having to add "\n" later

cmd/ndt7-client/internal/emitter/humanreadable_test.go, line 223 at r7 (raw file):

func TestHumanReadableOnSummary(t *testing.T) {
	expected := "         Server: test\n" +

Another great opportunity for the "`"-style string

cmd/ndt7-client/internal/emitter/json_test.go, line 15 at r7 (raw file):

func TestJSONOnStarting(t *testing.T) {
	sw := &mocks.SavingWriter{}
	j := jsonEmitter{sw}

Use the NewJSONWithWriter method, please

cmd/ndt7-client/internal/emitter/json_test.go, line 280 at r7 (raw file):

func TestNewJSONConstructor(t *testing.T) {
	j := NewJSON()

Does any code still use this? Should NewJSON just take a writer and then you can delete NewJSONWithWriter?

pboothe · 2020-02-13T22:11:54Z

This looks good to me with minor questions. I have faith that, upon considering the questions, you will come to a good conclusion one way or the other, so this PR Looks Good To Me.

robertodauria

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pboothe and @robertodauria)

cmd/ndt7-client/internal/emitter/humanreadable_test.go, line 223 at r7 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Another great opportunity for the "`"-style string

Done. I'm not sure why it did not work the first time, vscode seemed to treat multiline strings as code and replaced groups of 4 spaces with a \t. What was here was a workaround, but it's not needed anymore.

cmd/ndt7-client/internal/emitter/json_test.go, line 15 at r7 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Use the NewJSONWithWriter method, please

Done (here and in many other places in this file.)

cmd/ndt7-client/internal/emitter/json_test.go, line 280 at r7 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Does any code still use this? Should NewJSON just take a writer and then you can delete NewJSONWithWriter?

Not in the emitter package, but it's still used in main.go:

if *flagBatch || flagFormat.Value == "json" {
	e = emitter.NewJSON()

@robertodauria

We noticed how annoying is not doing that with @robertodauria during the review of m-lab/ndt7-client-go#39. As part of such work, we also double checked that every TCPInfo field described by the spec is a meaningful ndt7 summary. As a result we can now close #194.

robertodauria added 7 commits February 12, 2020 15:06

Create a new summary emitter and collect results while the test is ru…

524d52b

…nning.

Add -quiet flag, rename Summary emitter to Quiet and add Summary struct.

ace70e4

Add NewSummary and use it in the Batch emitter.

ab2fed7

Add server FQDN + interactive summary.

673b441

Update comments.

399e53f

Add ConnectionInfo and units in the JSON

65d73ac

Use CamelCase names in the JSON.

503832a

robertodauria requested a review from pboothe February 12, 2020 15:35

Fix tests in main_test.go.

5ced86e

pboothe reviewed Feb 12, 2020

View reviewed changes

cmd/ndt7-client/main.go Outdated Show resolved Hide resolved

cmd/ndt7-client/internal/emitter/interactive.go Outdated Show resolved Hide resolved

cmd/ndt7-client/main.go Outdated Show resolved Hide resolved

Start renaming stuff. Batch -> JSON

e1408ad

bassosimone suggested changes Feb 12, 2020

View reviewed changes

pboothe reviewed Feb 12, 2020

View reviewed changes

cmd/ndt7-client/internal/emitter/batch.go Outdated Show resolved Hide resolved

robertodauria added 11 commits February 13, 2020 13:16

Rename batch.go -> json.go.

983d9e9

Add -format=(human|json) flag and deprecate -batch.

df41915

Rename Interactive -> HumanReadable.

b199937

One Big Fprintf®.

0b769c2

Update documentation in main.go to reflect flag changes.

7d47d65

Update comments in summary.go

5d081b4

Add comment on Client.Results().

22766ae

Remove empty line.

cca9f3b

Add empty lines.

f7ab865

TestData -> LatestMeasurements.

2953870

Make NewJSON return an Emitter rather than a concrete type.

8512c86

This broke main_test.go with relied on being able to create a custom emitter.JSON with a non-standard writer. I've added NewJSONWithWriter() to still allow that from external packages.

bassosimone previously requested changes Feb 13, 2020

View reviewed changes

bassosimone mentioned this pull request Feb 13, 2020

ndt7: always include ConnectionInfo m-lab/ndt-server#261

Merged

Use flagx for -format.

a4c5112

robertodauria added 3 commits February 13, 2020 17:21

Update comments.

f305e38

Change NewHumanReadable to return an Emitter rather than a concrete t…

4ff0f8c

…ype. Also, add NewHumanReadableWithWriter(io.Writer).

Sort imports.

ab58a41

Move summary.go into emitter/ and write unit tests.

f1fc165

robertodauria requested a review from pboothe February 13, 2020 21:51

pboothe approved these changes Feb 13, 2020

View reviewed changes

Use NewJSONWithWriter whenever possible + fix multiline strings.

d353f3c

robertodauria commented Feb 13, 2020

View reviewed changes

robertodauria merged commit c4da0e9 into master Feb 13, 2020

robertodauria deleted the sandbox-roberto-summary branch February 13, 2020 22:44

robertodauria mentioned this pull request Feb 14, 2020

Add summary at the end of the ndt5 test m-lab/ndt5-client-go#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add summary at the end of the test #39

Add summary at the end of the test #39

robertodauria commented Feb 12, 2020 •

edited by pboothe

Loading

bassosimone left a comment •

edited

Loading

bassosimone Feb 12, 2020 •

edited

Loading

robertodauria Feb 13, 2020

bassosimone Feb 13, 2020

bassosimone Feb 12, 2020

robertodauria Feb 13, 2020 •

edited

Loading

bassosimone Feb 13, 2020

bassosimone left a comment

bassosimone Feb 13, 2020

robertodauria Feb 13, 2020

bassosimone Feb 13, 2020

bassosimone Feb 13, 2020

bassosimone Feb 13, 2020

bassosimone Feb 13, 2020

bassosimone Feb 13, 2020

robertodauria Feb 13, 2020

bassosimone Feb 13, 2020

bassosimone Feb 13, 2020

bassosimone commented Feb 13, 2020

pboothe left a comment

pboothe commented Feb 13, 2020

robertodauria left a comment

Add summary at the end of the test #39

Add summary at the end of the test #39

Conversation

robertodauria commented Feb 12, 2020 • edited by pboothe Loading

bassosimone left a comment • edited Loading

Choose a reason for hiding this comment

bassosimone Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertodauria Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassosimone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassosimone commented Feb 13, 2020

pboothe left a comment

Choose a reason for hiding this comment

pboothe commented Feb 13, 2020

robertodauria left a comment

Choose a reason for hiding this comment

robertodauria commented Feb 12, 2020 •

edited by pboothe

Loading

bassosimone left a comment •

edited

Loading

bassosimone Feb 12, 2020 •

edited

Loading

robertodauria Feb 13, 2020 •

edited

Loading