Add throughput1 client library #26

robertodauria · 2023-09-25T16:38:01Z

This PR adds the client library for MSAK's throughput1 protocol. It's meant to be a (fully functional) MVP, with more nice-to-have features to be added in subsequent PRs. To test it:

# defaults: 2 BBR streams, 3s duration, no delay, plaintext, production Locate instance and servers
go run ./cmd/msak-client

Things not included:

A flag for multi-server testing (to run each stream against a different server)
Aggregate throughput in HumanReadable's output -- unclear how it should be computed, since each stream intentionally excludes the TLS and WS handshakes, while it would be more correct to include them in this case. I'm considering to add a second network byte counter that includes that overhead, but that will involve a design update and a separate PR.

This change is

This commit adds application-level measurements and namespaces to better separate the two kind of measurements in the Measurement object. It is a breaking change and will need a schema update.

coveralls · 2023-09-27T16:48:07Z

Pull Request Test Coverage Report for Build 6354243872

41 of 231 (17.75%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-12.9%) to 70.837%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/client/emitter.go	0	23	0.0%
pkg/client/client.go	41	208	19.71%

Totals
Change from base Build 6345174327:	-12.9%
Covered Lines:	838
Relevant Lines:	1183

💛 - Coveralls

cristinaleonr

Thank you for setting up this client. My first instinct would be for the client to have its own repository that integrations can clone independently.

Reviewable status: 0 of 1 approvals obtained (waiting on @robertodauria)

cmd/msak-client/client.go line 17 at r3 (raw file):

var (
	clientName    = "msak-client-go"

Should this be a const?

cmd/msak-client/client.go line 28 at r3 (raw file):

	flagDuration = flag.Duration("duration", 3*time.Second, "Length of the last stream")
	flagScheme   = flag.String("scheme", "ws", "Websocket scheme (wss or ws)")
	flagMID      = flag.String("mid", uuid.NewString(), "Measurement ID to use")

Does this need to be unique for all measurements?

cmd/msak-client/client.go line 36 at r3 (raw file):

	flag.Parse()

	if float64(*flagStreams-1)*flagDelay.Seconds() >= flagDuration.Seconds() {

Why -1? Perhaps add a comment.

cmd/msak-client/client.go line 37 at r3 (raw file):

	if float64(*flagStreams-1)*flagDelay.Seconds() >= flagDuration.Seconds() {
		log.Print("Invalid configuration: please check streams, delay and duration and make sure they make sense.")

Would log.Fatal be more concise?

cmd/msak-client/client.go line 55 at r3 (raw file):

	cl.Emitter = &client.HumanReadable{
		Debug: *flagDebug,
	}

You could combine these opts in a ClientOpts object that is passed into client.New to be set. That way all the Client fields can be set in one place.

Code quote:

	cl.Dialer.TLSClientConfig = &tls.Config{
		InsecureSkipVerify: *flagNoVerify,
	}
	cl.Server = *flagServer
	cl.CongestionControl = *flagCC
	cl.NumStreams = *flagStreams
	cl.Scheme = *flagScheme
	cl.MeasurementID = *flagMID
	cl.Length = *flagDuration
	cl.Delay = *flagDelay

	cl.Emitter = &client.HumanReadable{
		Debug: *flagDebug,
	}

cmd/msak-client/client.go line 58 at r3 (raw file):

	cl.Download(context.Background())
	cl.Upload(context.Background())

Does it make sense to have a function that calls both of these?

Code quote:

	cl.Download(context.Background())
	cl.Upload(context.Background())

pkg/client/client.go line 91 at r3 (raw file):

	MeasurementID string

	// Emitter is the emitter used to emit results.

This sentence sounds a bit redundant. I'm not sure I understand what the Emitter does from the description alone.

pkg/client/client.go line 34 at r4 (raw file):

	// DefaultLength is the default test duration for a new client.
	DefaultLength = 5 * time.Second

What's the rationale for these defaults being different from those in the main package.

Code quote:

	// DefaultStreams is the default number of streams for a new client.
	DefaultStreams = 5

	// DefaultLength is the default test duration for a new client.
	DefaultLength = 5 * time.Second

pkg/client/client.go line 53 at r4 (raw file):

// Throughput1Client is a client for the throughput1 protocol.
type Throughput1Client struct {
	// ClientName is the name of the client as sent to the server as part of the user-agent.

nit: ClientName is the name of the client as sent to the server as part of the user-agent.

Same below.

pkg/client/client.go line 107 at r4 (raw file):

	Throughput float64
	Elapsed    time.Duration
	MinRTT     uint32

Please add docstrings for these.

Code quote:

	Goodput    float64
	Throughput float64
	Elapsed    time.Duration
	MinRTT     uint32

pkg/client/client.go line 136 at r4 (raw file):

			},
		},
		Scheme:     "wss",

Add this as a default in the set of default constants defined above?

pkg/client/client.go line 185 at r4 (raw file):

		fmt.Println(c.targets[c.tIndex[k]].URLs)
		r := c.targets[c.tIndex[k]].URLs[k]
		c.tIndex[k]++

What is this doing?

Code quote:

	if c.tIndex[k] < len(c.targets) {
		fmt.Println(c.targets[c.tIndex[k]].URLs)
		r := c.targets[c.tIndex[k]].URLs[k]
		c.tIndex[k]++

pkg/client/client.go line 198 at r4 (raw file):

	if c.Server != "" {
		c.Emitter.OnDebug(fmt.Sprintf("using server provided via flags %s", c.Server))
		path := getPathForSubtest(subtest)

What if the result for this call is "invalid"?

pkg/client/client.go line 210 at r4 (raw file):

	// If a service URL was provided, use it as-is.
	if c.ServiceURL != nil {

What flag sets ServiceURL?

pkg/client/client.go line 217 at r4 (raw file):

	}

	// If no service URL nor server was provided, use the Locate API.

nit: "neither ... nor". Alternatively "no ... or".

pkg/client/client.go line 240 at r4 (raw file):

	go func() {
		t := time.NewTicker(100 * time.Millisecond)

What happens if the test ends before 100ms (e.g., early exit)?

pkg/client/emitter.go line 10 at r4 (raw file):

)

type Emitter interface {

Please add docstrings.

pkg/client/emitter.go line 62 at r4 (raw file):

}

var _ Emitter = &HumanReadable{}

What is this for?

- Make client name const - Use client package consts for flags' default values - Use log.Fatal

robertodauria

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)

cmd/msak-client/client.go line 17 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Should this be a const?

Done.

cmd/msak-client/client.go line 28 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Does this need to be unique for all measurements?

The only case when -mid is passed directly (as opposed to coming from Locate) is when running the client with -server=<hostname> against a server with token verification disabled -- basically, only during development. A random UUID seemed a better default than a static string, or having to provide a fake MID manually every time.

cmd/msak-client/client.go line 36 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Why -1? Perhaps add a comment.

flagDelay adds a delay between one stream and the next one, and this check is to prevent configurations such as -delay 5s -duration 5s -streams 2 which would not allow the second stream to even start. The number of delays is streams - 1.

cmd/msak-client/client.go line 37 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Would log.Fatal be more concise?

Done.

pkg/client/client.go line 91 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

This sentence sounds a bit redundant. I'm not sure I understand what the Emitter does from the description alone.

:) how about now?

pkg/client/client.go line 34 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

What's the rationale for these defaults being different from those in the main package.

Now the command-line flags' defaults are set to these consts.

pkg/client/client.go line 107 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Please add docstrings for these.

Done.

pkg/client/client.go line 136 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Add this as a default in the set of default constants defined above?

Done.

pkg/client/client.go line 185 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

What is this doing?

That is magic I borrowed from ndt7-client-go 😄

targets is a slice of Locate's v2.Target that caches results from Locate's invocation the first time nextURLFromLocate is called.
tIndex maps strings like (ws|wss):///throughput1/v1/download to an index which is used to get the next target from targets on each subsequent call to nextURLFromLocate. It takes me a good couple of minutes to figure out how it works every time, to be honest. What do you think would help making it clearer?

pkg/client/client.go line 198 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

What if the result for this call is "invalid"?

Considering that value is always set in this file to either spec.SubtestDownload or spec.SubtestUpload, I changed getPathForSubtest to panic on an invalid subtest.

pkg/client/client.go line 210 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

What flag sets ServiceURL?

I've actually removed it entirely.

pkg/client/client.go line 240 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

What happens if the test ends before 100ms (e.g., early exit)?

At least one Measurement from the receiver side (client to server for download test, server to client for upload tests) should be available to show a result. 🤔

I can implement the same solution you implemented in ndt7-client-go and that takes care of download tests. However, I am not sure how upload tests could work in this case. Trusting Application.BytesSent or Network.BytesSent is generally unreliable, even more so on such a short test, but the client will likely not receive any counterflow message before ~250ms.

pkg/client/emitter.go line 62 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

What is this for?

Done. (added comment)

robertodauria

Moving this to a separate repo is something I'm considering as well. Meanwhile, I think I've addressed all your comments -- PTAL again? Thanks!

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)

cmd/msak-client/client.go line 55 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

You could combine these opts in a ClientOpts object that is passed into client.New to be set. That way all the Client fields can be set in one place.

Done. I called it client.Config but the general idea is the same. Thoughts?

cmd/msak-client/client.go line 58 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Does it make sense to have a function that calls both of these?

Hmm. I'm going to add two -download and -upload flags to enable or disable the corresponding test, so those two will always be called separately in main().

pkg/client/client.go line 91 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

:) how about now?

(This field and the corresponding docstring have been moved to config.go)

pkg/client/emitter.go line 10 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Please add docstrings.

Done.

cristinaleonr

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

cmd/msak-client/client.go line 17 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

Done.

Thanks! I've been thinking about how we've struggled to track down the exact client integration responsible for the periodic clients because the identifier is the default "ndt7-client-go-cmd/0.5.0 ndt7-client-go/0.5.0". What do you think about forcing the client to set this field to a non-empty string?

cmd/msak-client/client.go line 28 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

The only case when -mid is passed directly (as opposed to coming from Locate) is when running the client with -server=<hostname> against a server with token verification disabled -- basically, only during development. A random UUID seemed a better default than a static string, or having to provide a fake MID manually every time.

Makes sense. I guess I was thinking more so what happens to the data/BQ analyses if, say, there is a client that always passes in the same -mid or there is a collision for some reason?

cmd/msak-client/client.go line 36 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

flagDelay adds a delay between one stream and the next one, and this check is to prevent configurations such as -delay 5s -duration 5s -streams 2 which would not allow the second stream to even start. The number of delays is streams - 1.

I see. Thanks for adding a comment.

cmd/msak-client/client.go line 55 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

Done. I called it client.Config but the general idea is the same. Thoughts?

Looks great. Thanks!

pkg/client/client.go line 91 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

(This field and the corresponding docstring have been moved to config.go)

Perfect, thank you!

pkg/client/client.go line 185 at r4 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

That is magic I borrowed from ndt7-client-go 😄

targets is a slice of Locate's v2.Target that caches results from Locate's invocation the first time nextURLFromLocate is called.
tIndex maps strings like (ws|wss):///throughput1/v1/download to an index which is used to get the next target from targets on each subsequent call to nextURLFromLocate. It takes me a good couple of minutes to figure out how it works every time, to be honest. What do you think would help making it clearer?

I knew this looked familiar. Maybe we can add a comment explaining that so we'll all remember when we see it next time?

pkg/client/client.go line 210 at r4 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

I've actually removed it entirely.

Thanks! In the future, this functionality would be helpful to add back (e.g., my machine will not pass my location to AppEngine in the Locate request, so I would need to pass in a custom Locate URL with my location in the URL params).

robertodauria

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

cmd/msak-client/client.go line 17 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Thanks! I've been thinking about how we've struggled to track down the exact client integration responsible for the periodic clients because the identifier is the default "ndt7-client-go-cmd/0.5.0 ndt7-client-go/0.5.0". What do you think about forcing the client to set this field to a non-empty string?

I suspect in that case the client integration might just be running the ndt7-client-go binary itself.
I changed client.New() to panic if passed an empty string and added a corresponding unit test.

cmd/msak-client/client.go line 28 at r3 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Makes sense. I guess I was thinking more so what happens to the data/BQ analyses if, say, there is a client that always passes in the same -mid or there is a collision for some reason?

A client cannot pass a custom MID unless token verification is disabled on the server. When token verification is enabled, the MID is extracted from the signed JWT that comes from Locate, and the measurement won't start if the signature doesn't match. Does this address your concern?

pkg/client/client.go line 210 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Thanks! In the future, this functionality would be helpful to add back (e.g., my machine will not pass my location to AppEngine in the Locate request, so I would need to pass in a custom Locate URL with my location in the URL params).

Already discussed on Slack, but for awareness: this can be done via -locate.url, which is still available.

cristinaleonr

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

cmd/msak-client/client.go line 28 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

A client cannot pass a custom MID unless token verification is disabled on the server. When token verification is enabled, the MID is extracted from the signed JWT that comes from Locate, and the measurement won't start if the signature doesn't match. Does this address your concern?

Yes!

robertodauria added 30 commits March 24, 2023 00:30

Add handler

61325e2

Save archival data from deferred func

d3016f7

Add main, fix handler

49058f2

Add script to generate local test certs

1fa18bd

Add build script and Dockerfile.

7017664

Update go.mod/go.sum

d4e2a97

Add basic client for testing

7518b48

Swap logger

ce51f3d

Swap logger in measurer, too

21c2780

Remove files that don't belong to this PR

053476d

Remove more files that don't belong here

83b937b

go mod tidy

d3fb5d6

Change metadata length limits

b160cd9

Add access_token to the knownOptions map.

2116117

Add partial tests and prom metrics

d2d87a9

Duration should be milliseconds

af21b15

Increase test coverage

93227ad

Update go.mod

824fb95

Update go.mod go.sum

c169da4

Add client code

f0c6635

Merge branch 'main' into sandbox-roberto-client

5f64a9e

Rename ndt8 -> throughput1

1a75668

Separate network-level and application-level measurements.

6654601

This commit adds application-level measurements and namespaces to better separate the two kind of measurements in the Measurement object. It is a breaking change and will need a schema update.

Fix client

aa112c7

Merge branch 'main' into sandbox-roberto-client

60cdacd

Merge branch 'add-application-byte-counters' into sandbox-roberto-client

053fc14

Merge branch 'temp-test' into sandbox-roberto-client

711dd07

Make output more detailed, add server-side messages

3a531ad

Make application-level byte counters atomic.

da29c0a

Merge branch 'add-application-byte-counters' into sandbox-roberto-client

748e449

robertodauria added 3 commits September 27, 2023 18:56

Fix race condition.

61c3820

Refactor code, only output every 100ms

b9d430c

Remove extra \n from OnDebug messages

0f90997

cristinaleonr reviewed Sep 28, 2023

View reviewed changes

robertodauria added 7 commits September 28, 2023 18:13

Address code review comments

708874e

- Make client name const - Use client package consts for flags' default values - Use log.Fatal

Update defaults, add comments

09a3a76

Include

e663ea9

Add DefaultScheme const and more docstrings

bfef85d

Replace fmt.Print with Emitter.OnDebug

93f6c41

Panic on invalid subtests

8ff66c4

Remove ServiceURL

303308f

robertodauria commented Sep 28, 2023

View reviewed changes

robertodauria added 5 commits September 29, 2023 00:42

Move client config to a dedicated struct

bfe00cd

Rename client.ClientConfig to client.Config

298b354

More refactoring and more comments/docstrings.

24be2a2

Add TLSClientConfig to defaultDialer.

6ecdd3c

Add docstrings in Emitter interface.

c991115

robertodauria requested a review from cristinaleonr September 28, 2023 23:36

robertodauria commented Sep 28, 2023

View reviewed changes

robertodauria added 2 commits September 29, 2023 14:24

Merge branch 'main' into sandbox-roberto-client

ec3744b

FromTCPConn -> FromTCPLikeConn

fc287cc

cristinaleonr requested changes Sep 29, 2023

View reviewed changes

robertodauria added 2 commits September 29, 2023 17:00

Panic on empty clientName/Version in client.New()

a9e343b

Add comments in nextURLFromLocate and remove extra printf

26694cb

robertodauria commented Sep 29, 2023

View reviewed changes

robertodauria requested a review from cristinaleonr September 29, 2023 16:20

cristinaleonr approved these changes Sep 29, 2023

View reviewed changes

robertodauria merged commit 21baee2 into main Sep 29, 2023
6 checks passed

robertodauria deleted the sandbox-roberto-client branch September 29, 2023 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add throughput1 client library #26

Add throughput1 client library #26

robertodauria commented Sep 25, 2023 •

edited

Loading

coveralls commented Sep 27, 2023 •

edited

Loading

cristinaleonr left a comment

robertodauria left a comment

robertodauria left a comment

cristinaleonr left a comment

robertodauria left a comment

cristinaleonr left a comment

Add throughput1 client library #26

Add throughput1 client library #26

Conversation

robertodauria commented Sep 25, 2023 • edited Loading

coveralls commented Sep 27, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6354243872

💛 - Coveralls

cristinaleonr left a comment

Choose a reason for hiding this comment

robertodauria left a comment

Choose a reason for hiding this comment

robertodauria left a comment

Choose a reason for hiding this comment

cristinaleonr left a comment

Choose a reason for hiding this comment

robertodauria left a comment

Choose a reason for hiding this comment

cristinaleonr left a comment

Choose a reason for hiding this comment

robertodauria commented Sep 25, 2023 •

edited

Loading

coveralls commented Sep 27, 2023 •

edited

Loading