Introduced Release Candidate v3.0 #206

thrawn01 · 2023-12-07T20:34:40Z

CHANGES:

Introduced HTTPClient
Integrating DUH-RPC
Updated tests with new client
Fixed some lint issues
GRPC is no more
OTEL tracing for new client and handler
Updated benchmarks
Isolate the trace benchmark

CHANGES: * Introduced HTTPClient * Integrating DUH-RPC * Updated tests with new client * Fixed some lint issues * GRPC is no more * OTEL tracing for new client and handler * Updated benchmarks * Isolate the trace benchmark

Baliedge

Love it!

Baliedge · 2023-12-08T19:53:49Z

benchmark_test.go

+		{HTTPAddress: "127.0.0.1:9883", DataCenter: cluster.DataCenterOne},
+	}); err != nil {
+		fmt.Println(err)
+		os.Exit(1)


Why not t.Fatal(msg)?

Baliedge · 2023-12-08T19:54:55Z

benchmark_test.go

+	err := conf.SetDefaults()
+	require.NoError(b, err, "Error in conf.SetDefaults")
+
+	//b.Run("Forward() NO_BATCHING", func(b *testing.B) {


These code blocks needed?

Baliedge · 2023-12-08T20:32:44Z

peer.go

+	defer func() { tracing.EndScope(ctx, err) }()
+	span := trace.SpanFromContext(ctx)
+	span.SetAttributes(
+		attribute.String("peer.HTTPAddress", p.conf.Info.HTTPAddress),


Pretty sure OTel/Honeycomb prefer dot.delimited.snake_case instead of PascalCase.

Suggest: peer.http_address

Baliedge · 2023-12-08T20:33:50Z

peer.go

+	}()
+
+	// If config asked for no batching
+	if HasBehavior(r.Behavior, Behavior_NO_BATCHING) {


Would also like the global "Disable batching" feature here.

Baliedge · 2023-12-08T20:35:55Z

peer.go

+	}
+
+	// Wait for a response or context cancel
+	ctx2 := tracing.StartNamedScopeDebug(ctx, "Wait for response")


nit: If you don't need original ctx value later, I would just overwrite it.

Baliedge · 2023-12-08T20:36:52Z

peer.go

+	return err
+}
+
+// TODO: Replace this with modern error handling


Define "modern"?

MatthewEdge · 2023-12-11T13:51:51Z

README.md

@@ -289,20 +278,10 @@ Example response:
 ```

 ### Deployment
-NOTE: Gubernator uses `etcd`, Kubernetes or round-robin DNS to discover peers and
+NOTE: Gubernator uses `memberlist` Kubernetes or round-robin DNS to discover peers and


Suggested change

NOTE: Gubernator uses `memberlist` Kubernetes or round-robin DNS to discover peers and

NOTE: Gubernator uses `memberlist`, Kubernetes, or round-robin DNS to discover peers and

Just to make it clear that memberlist != K8S

I would go as far as adding the Oxford comma, too.

MatthewEdge · 2023-12-11T13:55:03Z

README.md

@@ -59,7 +60,7 @@ $ curl http://localhost:9080/v1/GetRateLimits \

 ### ProtoBuf Structure

-An example rate limit request sent via GRPC might look like the following


I'm not sure if it's strictly necessary but it may be worth adding a note that gRPC was dropped? Just for anyone who may have been using Gub with gRPC has an explicit warning

MatthewEdge · 2023-12-11T13:59:55Z

client.go

+func (c *client) Forward(ctx context.Context, req *ForwardRequest, resp *ForwardResponse) error {
+	payload, err := proto.Marshal(req)
+	if err != nil {
+		return duh.NewClientError(fmt.Errorf("while marshaling request payload: %w", err), nil)


More a suggestion for duh - it might be helpful for this method to accept the error and wrap for you, like: duh.NewClientError(err, "while marshaling request payload"). Tiny tiny stylistic item for reducing code read time (and absolutely not required!)

This. ☝🏻

MatthewEdge · 2023-12-11T14:06:52Z

cluster/cluster.go

@@ -97,7 +101,7 @@ func Start(numInstances int) error {
 // Restart the cluster
 func Restart(ctx context.Context) error {
 	for i := 0; i < len(daemons); i++ {
-		daemons[i].Close()
+		daemons[i].Close(context.Background())


For both Restart() and Close() (and the others, it seems) - do these ctx instances need timeouts?

MatthewEdge · 2023-12-11T14:10:34Z

cmd/healthcheck/main.go

 )

 func main() {
 	url := os.Getenv("GUBER_HTTP_ADDRESS")
 	if url == "" {
 		url = "localhost:80"
 	}
-	resp, err := http.DefaultClient.Get(fmt.Sprintf("http://%s/v1/HealthCheck", url))
+	resp, err := http.DefaultClient.Get(fmt.Sprintf("http://%s/healthz", url))


Should this be /health.check? README mentions /health.check

I think those are two different endpoints. Could we document the purpose of each?

MatthewEdge · 2023-12-11T14:11:19Z

tls_test.go

 	require.NoError(t, err)
-	reqNoClientCertRequired, err := http.NewRequest(http.MethodGet, fmt.Sprintf("https://%s/v1/HealthCheck", conf.HTTPStatusListenAddress), nil)
+	reqNoClientCertRequired, err := http.NewRequest(http.MethodGet, fmt.Sprintf("https://%s/healthz", conf.HTTPStatusListenAddress), nil)
 	require.NoError(t, err)

 	// Test that a client without a cert can access /v1/HealthCheck at status address


Same question here for if this should be /health.check for README consistency

MatthewEdge · 2023-12-11T14:13:24Z

README.md


 # Make a rate limit request
-$ curl http://localhost:9080/v1/GetRateLimits \
+$ curl http://localhost:9080/v1/rate-limits.check \


Suggested change

$ curl http://localhost:9080/v1/rate-limits.check \

$ curl http://localhost:9080/v1/rate-limit.check \

According to handler.go, at least :D

Baliedge · 2024-04-19T14:05:28Z

PR moved to new repo at: gubernator-io/gubernator#8.

thrawn01 requested a review from Baliedge as a code owner December 7, 2023 20:34

thrawn01 force-pushed the release-candidate-v3.0 branch from ebd6407 to e885db6 Compare December 7, 2023 21:34

Introduced Release Candidate v3.0

b6caf07

CHANGES: * Introduced HTTPClient * Integrating DUH-RPC * Updated tests with new client * Fixed some lint issues * GRPC is no more * OTEL tracing for new client and handler * Updated benchmarks * Isolate the trace benchmark

thrawn01 force-pushed the release-candidate-v3.0 branch from e885db6 to b6caf07 Compare December 8, 2023 17:04

Baliedge suggested changes Dec 8, 2023

View reviewed changes

MatthewEdge reviewed Dec 11, 2023

View reviewed changes

philipgough mentioned this pull request Jan 18, 2024

Rate limiter: add an alternative implementation based on Redis. observatorium/api#620

Merged

thrawn01 mentioned this pull request Feb 21, 2024

fix: mutex deadlocks in PeerClient #223

Merged

Baliedge closed this Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced Release Candidate v3.0 #206

Introduced Release Candidate v3.0 #206

thrawn01 commented Dec 7, 2023

Baliedge left a comment

Baliedge Dec 8, 2023

Baliedge Dec 8, 2023

Baliedge Dec 8, 2023

Baliedge Dec 8, 2023

Baliedge Dec 8, 2023

Baliedge Dec 8, 2023

MatthewEdge Dec 11, 2023 •

edited

Loading

Baliedge Dec 11, 2023

MatthewEdge Dec 11, 2023

MatthewEdge Dec 11, 2023

Baliedge Dec 11, 2023

MatthewEdge Dec 11, 2023

MatthewEdge Dec 11, 2023

Baliedge Dec 11, 2023

MatthewEdge Dec 11, 2023

MatthewEdge Dec 11, 2023

Baliedge commented Apr 19, 2024

	NOTE: Gubernator uses `memberlist` Kubernetes or round-robin DNS to discover peers and
	NOTE: Gubernator uses `memberlist`, Kubernetes, or round-robin DNS to discover peers and

		@@ -59,7 +60,7 @@ $ curl http://localhost:9080/v1/GetRateLimits \

		### ProtoBuf Structure

		An example rate limit request sent via GRPC might look like the following

	$ curl http://localhost:9080/v1/rate-limits.check \
	$ curl http://localhost:9080/v1/rate-limit.check \

Introduced Release Candidate v3.0 #206

Introduced Release Candidate v3.0 #206

Conversation

thrawn01 commented Dec 7, 2023

Baliedge 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

MatthewEdge Dec 11, 2023 • 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

Choose a reason for hiding this comment

Baliedge commented Apr 19, 2024

MatthewEdge Dec 11, 2023 •

edited

Loading