Saving websocket RTT samples by darkk · Pull Request #232 · m-lab/ndt-server

darkk · 2020-01-14T10:46:50Z

That's WIP for #192

I have a few questions I'm unsure about:

is it okay to carry start around the way it's implemented? It looks a bit messy to me, but I'm quite okay with that.
does it make sense to send WSInfo messages to client in /upload and /download? I feel like those may be eventually useful, those datapoints feed my curiosity about queuing, but I have no immediate use for the data.
should WSInfo be a separate optional sub-message in the spec or should it extend AppInfo with optional fields?
there is a feedback loop between the goroutines: sender (sends ping to) → [conn] (eventually delivers pong to) → receiver (sends RTT estimate back to client) → sender. The current implementation uses buffered channel for RTT estimates to avoid blocking on pongch <- rtt while sender is blocked on conn.send(hugeBlob) feeding the pipe with data. I don't like 14kB being allocated just to handle the worst-case, but I did not come up with a more elegant solution.

WIP:

ensure that at least one websocket ping is sent ahead of the blob in download.sender to get one unbiased sample of L7 RTT for each /download for "free"
update spec/ndt7-protocol.md

This change is

L7 ping may be significantly different from L4 pings and separate endpoint `/ping` is used to keep spec intact till further discussion happens. See 3520181 and m-lab#192

Also, move that measurement data into to WSInfo sub-object as it's currently unclear if AppInfo should be extended or not. It's part of ndt7 spec :) TODO: squash this commit into previous one

bassosimone · 2020-01-20T13:54:47Z

I am sorry, @darkk, I meant to update this PR to master but the end result was that I merged this PR to master, which I didn't want. Can you reopen/reissue it? My apologies 🙏

bassosimone · 2020-01-20T13:59:04Z

(Weirdly enough, I could merge a PR with broken CI in master without any prompt like "hey, you want merge this, you sure? You can override if you wish", which is what I'd expect @pboothe)

darkk · 2020-01-20T18:35:51Z

@bassosimone I'll reopen it, but I'd like to clean the code up a bit first. I've not found a good reason to send each and every WSInfo to the client. I think that the way to go is to send one sample for /download (the very first one) and every sample for /ping. It also solves the questionable buffer size for the rtt-passing pipe.

bassosimone · 2020-01-21T10:44:47Z

Let me take a look at the existing code and provide further feedback.

darkk · 2020-01-21T10:54:28Z

@bassosimone okay, it's #234. Keep in mind that I've not finished uprooting of the inelegant 14kb channel buffer yet, so that part is yet to be refactored.

bassosimone

Did read the diff, asked clarifying questions! (I think I have a slight preference for opening a new PR, because I'd rather have the discussion happen on a live PR rather than on a dead one, for future clarify.)

bassosimone · 2020-01-21T10:45:15Z

html/ndt7-ping.js

+      }
+    }
+  }
+}


I am ignoring the JavaScript part for now.

bassosimone · 2020-01-21T10:45:32Z

html/ndt7.html

+    runPing(function() { runDownload(function() { runUpload(); }); })
  </script>
 </body>
 </html>


I am ignoring the JavaScript part for now.

bassosimone · 2020-01-21T10:49:17Z

ndt7/download/download.go

-func Do(ctx context.Context, conn *websocket.Conn, resultfp *results.File) {
+// arguments are owned by the caller of this function. The start argument is
+// the test start time used to calculate ElapsedTime and deadlines.
+func Do(ctx context.Context, conn *websocket.Conn, resultfp *results.File, start time.Time) {


Passing in the start time is very useful to start ensuring the same zero is used everywhere.

bassosimone · 2020-01-21T10:50:36Z

ndt7/model/measurement.go

 	ConnectionInfo *ConnectionInfo `json:",omitempty" bigquery:"-"`
 	BBRInfo        *BBRInfo        `json:",omitempty"`
 	TCPInfo        *TCPInfo        `json:",omitempty"`
+	WSInfo         *WSInfo	       `json:",omitempty"`


This looks like PingInfo to me.

bassosimone · 2020-01-21T10:50:57Z

ndt7/model/wsinfo.go

+package model
+
+// WSInfo contains an application level (websocket) ping measurement data.
+// It may be melded into AppInfo.


No, I'd rather actually keep them separate

bassosimone · 2020-01-21T10:58:17Z

ndt7/download/download.go

-	receiverch := receiver.StartDownloadReceiver(wholectx, conn)
+	measurerch := measurer.Start(wholectx, conn, resultfp.Data.UUID, start)
+	receiverch, pongch := receiver.StartDownloadReceiver(wholectx, conn, start, measurerch)
+	senderch := sender.Start(conn, measurerch, start, pongch)


I am confused by the fact that a new channel has been introduced. Isn't it possible to use the measurerch for passing around information? I understood that the PING is another nullable pointer within a Measurement.

bassosimone · 2020-01-21T11:01:17Z

ndt7/receiver/receiver.go

+				LastRTT: int64(rtt / time.Microsecond),
+				MinRTT: int64(minRTT / time.Microsecond),
+			}
+			pongch <- wsinfo // Liveness: buffered (sender)


So, here I believe it should be possible to use dst to emit a measurement containing PingInfo, right?

bassosimone

Asked some questions

darkk added 2 commits January 13, 2020 14:27

ndt7: measure and report L7 RTT based on websocket ping

bfa6ccb

L7 ping may be significantly different from L4 pings and separate endpoint `/ping` is used to keep spec intact till further discussion happens. See 3520181 and m-lab#192

ndt7: fix liveness bug in L7-RTT measurement

3a88000

Also, move that measurement data into to WSInfo sub-object as it's currently unclear if AppInfo should be extended or not. It's part of ndt7 spec :) TODO: squash this commit into previous one

bassosimone added the review/triage label Jan 14, 2020

bassosimone and others added 3 commits January 14, 2020 12:07

Merge branch 'master' into l7-ping

49500fd

Fix compatibility with go1.12

6cee443

ndt7: take one L7-RTT sample right before flooding the connection

d7e8065

bassosimone merged commit f792464 into m-lab:master Jan 20, 2020

bassosimone mentioned this pull request Jan 20, 2020

Revert "Saving websocket RTT samples" #233

Merged

darkk mentioned this pull request Jan 21, 2020

Saving websocket RTT samples #234

Open

1 task

bassosimone reviewed Jan 21, 2020

View reviewed changes

Conversation

darkk commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bassosimone commented Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bassosimone commented Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darkk commented Jan 20, 2020

Uh oh!

bassosimone commented Jan 21, 2020

Uh oh!

darkk commented Jan 21, 2020

Uh oh!

bassosimone left a comment

Choose a reason for hiding this comment

Uh oh!

bassosimone Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

bassosimone Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

bassosimone Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

bassosimone Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

bassosimone Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

bassosimone Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

bassosimone Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

bassosimone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

darkk commented Jan 14, 2020 •

edited

Loading

bassosimone commented Jan 20, 2020 •

edited

Loading

bassosimone commented Jan 20, 2020 •

edited

Loading