feat(storage): introduce dp detector based on grpc metrics #11100
Conversation
storage/storage.go
Outdated
| // CheckDirectConnectivitySupported checks if Direct Connectivity is available | ||
| // for a specific bucket. | ||
| // | ||
| // Implementation currently uses client-side metrics for gRPC to detect |
There was a problem hiding this comment.
Can leave out this sentence from the godoc and mention inline below, it's too technical for the public docs.
storage/integration_test.go
Outdated
| func TestIntegration_DetectDirectConnectivity(t *testing.T) { | ||
| ctx := skipHTTP("direct connectivity isn't available for json") | ||
| multiTransportTest(ctx, t, func(t *testing.T, ctx context.Context, bucket string, _ string, _ *Client) { | ||
| _, err := CheckDirectConnectivitySupported(ctx, bucket) |
There was a problem hiding this comment.
I get why we are not actually checking the result (because the test should still work off GCE) but it feels a little annoying to not be able to validate correctness at all. Any ideas on a way to do this?
There was a problem hiding this comment.
Yea, we can do ResourceDetection and if in GCE we expect direct path for a co-located bucket. Worth trying and seeing what happens.
storage/storage.go
Outdated
| @@ -233,6 +239,73 @@ func NewGRPCClient(ctx context.Context, opts ...option.ClientOption) (*Client, e | |||
| return &Client{tc: tc}, nil | |||
| } | |||
|
|
|||
| // CheckDirectConnectivitySupported checks if Direct Connectivity is available | |||
| // for a specific bucket. | |||
There was a problem hiding this comment.
for a specific bucket from the environment where the client is running.
storage/storage.go
Outdated
| @@ -233,6 +239,73 @@ func NewGRPCClient(ctx context.Context, opts ...option.ClientOption) (*Client, e | |||
| return &Client{tc: tc}, nil | |||
| } | |||
|
|
|||
| // CheckDirectConnectivitySupported checks if Direct Connectivity is available | |||
There was a problem hiding this comment.
I'd call it "gRPC direct connectivity" (mention gRPC & use lower case to match cloud docs).
storage/storage.go
Outdated
| // Implementation currently uses client-side metrics for gRPC to detect | ||
| // Direct Connectivity using grpc.lb.locality label. | ||
| // | ||
| // You may configure the client by passing in options from the [google.golang.org/api/option] |
There was a problem hiding this comment.
I'd specifically say to pass in any client options that you are planning on passing to [NewGRPCClient]
storage/storage.go
Outdated
| // | ||
| // You may configure the client by passing in options from the [google.golang.org/api/option] | ||
| // package. | ||
| func CheckDirectConnectivitySupported(ctx context.Context, bucket string, opts ...option.ClientOption) (bool, error) { |
There was a problem hiding this comment.
I wonder if we should just make this return an error, and nil error means DP worked?
I think we should wrap and return any errors from the metrics stuff to help debug if things go wrong.
There was a problem hiding this comment.
I like this idea; instead of returning a boolean emit an err when DP isn't available otherwise no error means DP is available.
storage/integration_test.go
Outdated
| t.Fatalf("resource.New: %v", err) | ||
| } | ||
| if v, present := detectedAttrs.Set().Value("cloud.platform"); present && v.AsString() == "gcp_compute_engine" { | ||
| err := CheckDirectConnectivitySupported(ctx, bucket) |
There was a problem hiding this comment.
I think we'd have to create the bucket in the test to make sure it is in the same region as the VM.
Does the resource detector allow parsing the region?
There was a problem hiding this comment.
good call addressed.
storage/integration_test.go
Outdated
| t.Fatalf("CheckDirectConnectivitySupported: region not detected") | ||
| } | ||
| region := v.AsString() | ||
| newBucketName := "go-integration-dc-" + region + "-" + uidSpace.New() |
There was a problem hiding this comment.
Use the prefix arg from multiTransportTest to name the bucket and insure it gets cleaned up.
storage/storage.go
Outdated
| combinedOpts := append(opts, WithDisabledClientMetrics(), option.WithGRPCDialOption(opentelemetry.DialOption(opentelemetry.Options{MetricsOptions: mo}))) | ||
| client, err := NewGRPCClient(ctx, combinedOpts...) | ||
| if err != nil { | ||
| return fmt.Errorf("checkDirectConnectivitySupported: %v", err) |
There was a problem hiding this comment.
These errors should reference the function being called for stack traces purposes, and use the %w verb for error wrapping (e.g. fmt.Errorf("storage.NewGRPCClient: %w", err)). Same below.
There was a problem hiding this comment.
@tritone i introduced an error named ErrDirectConnectivityNotDetected in the latest commit; IIUC the client has errors to make it easier to check rather than string checking the error.
WDYT?
Got a thumbs up from @zasweq from Go gRPC team.