gRPC Client Server #18

berkeli · 2022-10-25T16:17:21Z

No description provided.

Relates to #14

illicitonion

This is really nicely put together, good job!

grpc-client-server/prober/prober.proto

grpc-client-server/prober_client/main.go

illicitonion · 2022-10-30T18:39:39Z

grpc-client-server/prober_client/main.go

+
+func CreateProgressBar(w io.Writer, timeout time.Duration, endpoint string, results <-chan *Result, wg *sync.WaitGroup) {
+	defer wg.Done()
+	ticker := 200 * time.Millisecond


If I set my timeout to 1s, what will I see the progress bar do?

It's not ideal at the moment, it doesn't show the progress of requests but only the progress of timeout.

So if you provide 1s timeout it will have 5 ticks (20%, 40%, etc). If the request completes in 0.5s then it will just fill up to 100%.

In that sense, I guess it's more of a countdown than a progress bar :D

Will the first tick actually hit 20% though? How does the progress bar know that 1 tick is 20%?

So the progress bar starts with 0, then for ticker I have provided 200ms.

100% for the progress bar is int(timeout/ticker) for 1s this will be 5, so progress bar will start at 0 then 20%(1/5), 40%(2/5) etc.

The ticker doesn't add 200ms, it only adds 1

case <-time.After(ticker): bar.Add(1)

grpc-client-server/prober_client/main_test.go

illicitonion · 2022-10-30T18:51:29Z

grpc-client-server/prober_client/main_test.go

+	}
+}
+
+var lis = bufconn.Listen(1024 * 1024)


I don't think this is used?

illicitonion · 2022-10-30T18:53:19Z

grpc-client-server/prober_client/main_test.go

+				value: "foo",
+				want:  &ArrayFlag{"foo"},
+			},
+			"add another element": {


I'd say "add two elements" rather than "add another element" - another implies it's related to something else that already happened (e.g. that your two test cases depend on each other, which they don't)

It's actually adding another element, there's no option to add multiple at once

The difference I'm trying to highlight is that if you have two test cases: "add element" and "add another element", they read as if possibly related.

Can you think of a name which doesn't suggest that?

illicitonion · 2022-10-30T18:56:12Z

grpc-client-server/prober_client/main_test.go

+	})
+}
+
+func TestGRpc(t *testing.T) {


It's really useful to have these tests which test your test helpers (i.e. non-production code you're just using for tests), but often we'd put these in their own test file so it's easier to see at a glance what tests you have for your actual application vs test helpers.

illicitonion · 2022-10-30T18:59:13Z

grpc-client-server/prober_server/main_test.go

+			resp, err := client.DoProbes(context.Background(), tc.req)
+
+			if err == nil {
+				require.NotNil(t, resp.AverageResponseTime)


Can you think how you could actually test the response time as well?

(hint: you probably don't actually want to connect to Google when doing so!)

great suggestion, I have added a test now.

There could be another way of testing it, by extracting the prober and making it take a parameter for time object. I went with a solution where I mock time.Since, as I think it's clearner for this small usecase.

The mock time.Since works, but the golang HTTP stack has a built-in type for this (just like you used a custom Dialer, called an http.RoundTripper - try using it :)

illicitonion

Nice improvements!

illicitonion · 2022-11-01T20:50:16Z

grpc-client-server/prober_server/main.go

+	}
+
+	if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+		return ttfb, ttlb, fmt.Errorf("status code %d", resp.StatusCode)


What value will ttlb have here? Is that an accurate/useful value? Is there a better value we could use?

Yes using a null object would defenitely be best practice.

illicitonion · 2022-11-01T20:51:04Z

grpc-client-server/prober_server/main.go

+	_, err = io.ReadAll(resp.Body)
+
+	ttlb = timeSince(start)
+	resp.Body.Close()


You should make sure to close the body, even if you don't care about its contents.

Yes, but which one would be true TTLB? closing the body and then measuring time or measuring time and then closing body. There's 1-2ms difference in some cases

That's an interesting question (to which I'm actually not sure the answer - probably before the close makes more sense), but I was more thinking about not closing the body in the case of your early-return based on the status code :)

illicitonion · 2022-11-01T21:10:47Z

grpc-client-server/prober_server/main_test.go

+			resp, err := client.DoProbes(context.Background(), tc.req)
+
+			if err == nil {
+				require.NotNil(t, resp.AverageResponseTime)


The mock time.Since works, but the golang HTTP stack has a built-in type for this (just like you used a custom Dialer, called an http.RoundTripper - try using it :)

illicitonion · 2022-11-01T21:12:44Z

grpc-client-server/prober_server/main.go

+		ttlbTotal += ttlb
+	}
+	ttfbAverage := time.Duration(float32(ttfbTotal) / float32(in.NumberOfRequests))
+	ttlbAverage := time.Duration(float32(ttlbTotal) / float32(in.NumberOfRequests))


Do you think we want to average together the TTLB for both successful and unsuccessful responses?

Ah good find, we should defenitely not take them into account!

illicitonion

Looking great! A few final thoughts :)

grpc-client-server/prober_server/main.go

illicitonion · 2022-11-03T18:29:36Z

grpc-client-server/prober_server/main.go

+type TimedRoundTripper struct {
+	defaultTripper http.RoundTripper
+	recordTime     func(time.Duration)
+}


This is a pretty common pattern we run into in code, to have an implementation of an interface which wraps another, doing something special around it and using it - often called the decorator pattern.

Rather than calling the field here defaultTripper, we'd ordinarily call it something like delegate or underlying. The reason not to call it default is that we don't actually know or care that the one we delegate to is the http.DefaultTransport - maybe the one we delegate to also has some other special behaviour.

That's great, Thanks!

illicitonion · 2022-11-03T18:31:59Z

grpc-client-server/prober_server/main_test.go

+
+			resp, err := client.DoProbes(context.Background(), tc.req)
+
+			if err == nil {


We shouldn't be conditionally making assertions based on whether our real implementation returned an error - we should be asserting whether we expect the real implementation to return an error.

I had created a field in test cases for errors, no idea why I didn't use them. Thanks!

illicitonion · 2022-11-03T18:36:02Z

grpc-client-server/prober_server/main_test.go

+
+		client := pb.NewProberClient(conn)
+
+		timeSince = func(t time.Time) time.Duration {


Rather than setting a global variable here, can you make it so that when you construct your server, you pass in this value? (Or actually, you pass in the http.RoundTripper implementation you want to use? Think about how the http.Client struct works - if you construct one with a custom RoundTripper, it'll use it, if not, it'll use a default one).

That way, in your test you can pass in a custom RoundTripper without needing any global variables.

This is great, thanks! implemented now with timeSince mock for server and http client.

Not super happy with the implementation that client is referencing a variable from the parent function, will have to do some refactoring to remove that dependency and you suggesting of passing a custom transport to the server sounds like it might solve this issue..

ttfbTotal := time.Duration(0) ttlbTotal := time.Duration(0) failed := 0 latestTtfb := time.Duration(0) c := &http.Client{ Transport: &TimedRoundTripper{ underlying: http.DefaultTransport, recordTime: func(t time.Duration) { latestTtfb = t ttfbTotal += t }, timeSince: s.timeSince, }, }

illicitonion · 2022-11-03T18:37:40Z

grpc-client-server/prober_server/main_test.go

+		require.Greater(t, resp.TtfbAverageResponseTime.AsDuration(), time.Duration(0))
+		require.Equal(t, 2*time.Second, resp.TtlbAverageResponseTime.AsDuration())


One of these assertions is really specific, and the other really vague - what makes it hard to be specific in both?

The challenge was to do with 2 different timing methods, 1 for TTFB and 1 for TTLB and that they are in 2 different places.

I didn't want to measure TTLB inside the roundtripper as it will read everything from the body and return empty body. For our use case this would be fine, but I still think it's bad practice to be modified something that's expected to not be modified.

We could of course make a copy of the body and return that, but this would use up unwanted resources.

I have now implemented a solution for precise testing, which required using timeSince mock for both the server and Roundtripper.

berkeli added 5 commits October 24, 2022 17:08

basic implementation

76c9660

Added prometheous

1c4104c

add progress-bar, allow multiple endpoints

aaf859e

refactor

0778e1e

Add tests

5f62b7f

berkeli linked an issue Oct 26, 2022 that may be closed by this pull request

gRPC project #14

Open

berkeli pushed a commit that referenced this pull request Oct 28, 2022

Change uses of you/your to we/our (#18)

11f6f81

Relates to #14

illicitonion reviewed Oct 30, 2022

View reviewed changes

berkeli added 3 commits October 31, 2022 20:43

refactor

215b950

remove unused var

249825c

add ttlb/ttfb measurements

223f817

illicitonion reviewed Nov 1, 2022

View reviewed changes

berkeli added 2 commits November 2, 2022 18:58

refactor, use roundtripper instead of httptrace

bee1cf5

close body in every case

50df624

illicitonion reviewed Nov 3, 2022

View reviewed changes

berkeli added 2 commits November 4, 2022 11:15

refactor

712c81c

discard body, test for errors

6e606ce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC Client Server #18

gRPC Client Server #18

berkeli commented Oct 25, 2022

illicitonion left a comment

illicitonion Oct 30, 2022

berkeli Nov 1, 2022

illicitonion Nov 1, 2022

berkeli Nov 2, 2022 •

edited

Loading

illicitonion Oct 30, 2022

illicitonion Oct 30, 2022

berkeli Nov 1, 2022

illicitonion Nov 1, 2022

illicitonion Oct 30, 2022

illicitonion Oct 30, 2022

berkeli Oct 31, 2022

illicitonion Nov 1, 2022

illicitonion left a comment

illicitonion Nov 1, 2022

berkeli Nov 2, 2022

illicitonion Nov 1, 2022

berkeli Nov 2, 2022

illicitonion Nov 2, 2022

illicitonion Nov 1, 2022

illicitonion Nov 1, 2022

berkeli Nov 2, 2022

illicitonion left a comment

illicitonion Nov 3, 2022

berkeli Nov 4, 2022

illicitonion Nov 3, 2022

berkeli Nov 4, 2022

illicitonion Nov 3, 2022

berkeli Nov 4, 2022

illicitonion Nov 3, 2022

berkeli Nov 4, 2022


		resp, err := client.DoProbes(context.Background(), tc.req)

		if err == nil {


		client := pb.NewProberClient(conn)

		timeSince = func(t time.Time) time.Duration {

		require.Greater(t, resp.TtfbAverageResponseTime.AsDuration(), time.Duration(0))
		require.Equal(t, 2*time.Second, resp.TtlbAverageResponseTime.AsDuration())

gRPC Client Server #18

Are you sure you want to change the base?

gRPC Client Server #18

Conversation

berkeli commented Oct 25, 2022

illicitonion 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

berkeli Nov 2, 2022 • 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

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

illicitonion 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

illicitonion 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

berkeli Nov 2, 2022 •

edited

Loading