feat(storage): introduce gRPC client-side metrics #10639

frankyn · 2024-08-06T21:48:25Z

Introduce client-side metrics for gRPC client only.

Pending

Discuss user facing options and default behaviors with @tritone
Upgrade gRPC version to 1.66 - chore(all): update grpc to 1.66 #10787
Run go work sync before merge
Upgrade OTel version to 1.28 (chore(all): update opentelemetry-go monorepo to v1.28.0 - autoclosed #10706)
Error logs are surfaced when permissions aren't granted to the exporter and can get noisy. Java silenced them after the initial error to prevent noise but allows recovery if permissions are granted after release into production.

tritone

Overall looks pretty good. We should talk about default behavior and user facing options.

tritone · 2024-08-08T18:13:44Z

storage/grpc_client.go

@@ -136,6 +152,9 @@ func newGRPCStorageClient(ctx context.Context, opts ...storageOption) (storageCl
 }

 func (c *grpcStorageClient) Close() error {
+	if c.settings.meterProvider != nil {


Is it problematic if this never gets called? e.g. will some data not be flushed?

I just ask because calling client.Close() is basically optional; the program could just terminate first. But maybe that's fine.

tritone · 2024-08-08T18:14:50Z

storage/grpc_client.go

@@ -116,6 +117,21 @@ type grpcStorageClient struct {
 func newGRPCStorageClient(ctx context.Context, opts ...storageOption) (storageClient, error) {
 	s := initSettings(opts...)
 	s.clientOption = append(defaultGRPCOptions(), s.clientOption...)
+	// TODO: detect project id


I am assuming we'd have to add an option to enable/disable metrics as well?

tritone · 2024-08-08T18:19:41Z

storage/grpc_metrics.go

+	return provider, nil
+}
+
+func togRPCDialOption(provider *sdkmetric.MeterProvider) option.ClientOption {


nit: name seems misleading, maybe clientOptionFromMeterProvider. Or just do this inline since the function is only called once.

tritone · 2024-08-08T18:22:32Z

storage/grpc_metrics.go

+		{Key: "host_id", Value: attribute.StringValue(mr_host_id_label_default)}}
+}
+
+func getPreparedResourceUsingGCPDetector(ctx context.Context, project string) (*resource.Resource, error) {


nit: This func also seems superfluous and might be clearer in-line

I wrote it this way to write tests; should I be doing something different to make it testable?

tritone · 2024-08-08T18:24:01Z

storage/grpc_metrics.go

+	metric_lb_locality_label        = "grpc.lb.locality"
+)
+
+func getMetricsToEnable() []estats.Metric {


This and getMetricsEnabledByDefault could probably be local vars rather than functions given that they are just static data.

tritone · 2024-08-08T18:26:08Z

storage/grpc_metrics_test.go

+
+func TestMetrics(t *testing.T) {
+	ctx := context.Background()
+	grpcClient, err := NewGRPCClient(ctx)


Does this work without auth? If not we should probably make it an integration test. Or try option.WithoutAuthentication

Requires auth; I need to update to not run with short tests.

If it's an integration test, move to the file with the other integration tests.

BrennaEpp · 2024-08-24T02:01:00Z

storage/doc.go

@@ -359,13 +359,50 @@ the following side-effect imports to your application:
 		_ "google.golang.org/grpc/xds/googledirectpath"
 	)

+The gRPC client emits metrics by default and will export the
+gRPC telemetry discussed in [gRFC/66] and [gRFC/78] to
+[Google Cloud Monitoring]. The metrics are accessible to through Cloud Monitoring API and you incur no additional cost for publishing the metrics.Google Cloud Support can use this information to more quickly diagnose


Suggested change

[Google Cloud Monitoring]. The metrics are accessible to through Cloud Monitoring API and you incur no additional cost for publishing the metrics.Google Cloud Support can use this information to more quickly diagnose

[Google Cloud Monitoring]. The metrics are accessible through Cloud Monitoring API and you incur no additional cost for publishing the metrics. Google Cloud Support can use this information to more quickly diagnose

BrennaEpp · 2024-08-27T05:21:15Z

storage/grpc_metrics.go

+}
+
+func enableClientMetrics(ctx context.Context, s *settings) (*internalMetricsContext, error) {
+	_, ep, err := htransport.NewClient(ctx, s.clientOption...)


I believe we have this endpoint captured under the client as xml host/scheme.

BrennaEpp · 2024-08-27T05:24:58Z

storage/grpc_metrics.go

+// lack of credentials after initial failure.
+// https://pkg.go.dev/go.opentelemetry.io/otel/sdk/metric@v1.28.0#Exporter
+func (e *exporterLogSuppressor) Export(ctx context.Context, rm *metricdata.ResourceMetrics) error {
+	if err := e.exporter.Export(ctx, rm); err != nil && !e.emittedFailure {


Maybe we should do the check for emittedFailure earlier? Or is the intent that the user can fix the permission issue while the code is still running without having to restart?

The intention is that once it's deployed it can recover once appropriate permissions are provided otherwise a redeployment would be required.

product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Aug 6, 2024

github-actions bot added the breaking change label Aug 6, 2024

frankyn force-pushed the grpc-storage-metrics branch 2 times, most recently from 4863273 to 892e55c Compare August 7, 2024 17:55

tritone reviewed Aug 8, 2024

View reviewed changes

frankyn added 6 commits August 15, 2024 09:10

feat: initial prototype testing client-side metrics

7df55c0

added tests and refactored to help test

73d1742

use 1.21 and refactor test

db28808

fix go.sum go.work.sum

4883da3

add option to disable client metrics

ddf0d41

wip

210280d

frankyn force-pushed the grpc-storage-metrics branch from ce08cc5 to 210280d Compare August 15, 2024 16:53

add endpoint and project detection

d25adfb

frankyn force-pushed the grpc-storage-metrics branch 2 times, most recently from 55f8e46 to e7abe02 Compare August 15, 2024 23:31

revert unnecessary changes

f91c109

frankyn force-pushed the grpc-storage-metrics branch from e7abe02 to f91c109 Compare August 15, 2024 23:32

frankyn marked this pull request as ready for review August 16, 2024 16:01

frankyn requested review from a team as code owners August 16, 2024 16:01

Merge branch 'main' into grpc-storage-metrics

8a1cb19

frankyn added the status: blocked Resolving the issue is dependent on other work. label Aug 16, 2024

frankyn requested a review from tritone August 19, 2024 15:53

frankyn and others added 7 commits August 20, 2024 16:23

Merge remote-tracking branch 'origin/main' into grpc-storage-metrics

09f37d6

Merge branch 'main' into grpc-storage-metrics

f64fb6a

update documentation

7d4e3fd

Merge branch 'main' into grpc-storage-metrics

e448378

remove unnecessary code and update docs

edf13f9

address vet workflow feedback

116afbd

Merge branch 'main' into grpc-storage-metrics

8b5bd88

const metricPrefix

1c53758

BrennaEpp reviewed Aug 27, 2024

View reviewed changes

frankyn added 3 commits August 27, 2024 14:25

doc edit

024e6d3

Merge remote-tracking branch 'origin' into grpc-storage-metrics

a0a8edc

add additional comments and remove metric descriptions; fix failing test

ccc7bd4

frankyn requested review from BrennaEpp and tritone August 28, 2024 16:43

Merge remote-tracking branch 'origin/main' into grpc-storage-metrics

a50c50c

frankyn mentioned this pull request Aug 28, 2024

chore(all): update grpc to 1.66 #10787

Merged

frankyn and others added 4 commits August 28, 2024 14:57

create ConnPool to access endpoint before GAPIC is initialized

41b469c

Merge branch 'main' into grpc-storage-metrics

be3f5d5

double up on ConnPool :(

498b6e0

Merge branch 'main' into grpc-storage-metrics

ad9a8c5

frankyn removed status: blocked Resolving the issue is dependent on other work. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Aug 28, 2024

frankyn added 2 commits August 28, 2024 15:53

go work sync

2334322

clean up DialPool creation for metrics

c0610c8

frankyn mentioned this pull request Aug 28, 2024

storage: determine gRPC endpoint without requiring workaround #10788

Closed

frankyn and others added 7 commits August 28, 2024 16:16

add todo to remove workaround

83bb31b

revert line removals

75e8f6b

make sure we emit RPC name and remove initializing second connpool

0348728

Merge branch 'main' into grpc-storage-metrics

5a5d8cb

remove endpoint magic

48c0fdf

edit docs

1fa2883

Merge branch 'main' into grpc-storage-metrics

dab45eb

frankyn enabled auto-merge (squash) August 29, 2024 16:40

danielduhh approved these changes Aug 29, 2024

View reviewed changes

frankyn merged commit 437bcb1 into main Aug 29, 2024
11 checks passed

frankyn deleted the grpc-storage-metrics branch August 29, 2024 18:45

release-please bot mentioned this pull request Aug 29, 2024

chore(main): release storage 1.44.0 #10522

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): introduce gRPC client-side metrics #10639

feat(storage): introduce gRPC client-side metrics #10639

frankyn commented Aug 6, 2024 •

edited

Loading

tritone left a comment

tritone Aug 8, 2024

tritone Aug 8, 2024

tritone Aug 8, 2024

tritone Aug 8, 2024

frankyn Aug 12, 2024

tritone Aug 8, 2024

tritone Aug 8, 2024

frankyn Aug 12, 2024

tritone Aug 26, 2024

BrennaEpp Aug 24, 2024

BrennaEpp Aug 27, 2024

BrennaEpp Aug 27, 2024

frankyn Aug 27, 2024

	[Google Cloud Monitoring]. The metrics are accessible to through Cloud Monitoring API and you incur no additional cost for publishing the metrics.Google Cloud Support can use this information to more quickly diagnose
	[Google Cloud Monitoring]. The metrics are accessible through Cloud Monitoring API and you incur no additional cost for publishing the metrics. Google Cloud Support can use this information to more quickly diagnose

feat(storage): introduce gRPC client-side metrics #10639

feat(storage): introduce gRPC client-side metrics #10639

Conversation

frankyn commented Aug 6, 2024 • edited Loading

tritone 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

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

frankyn commented Aug 6, 2024 •

edited

Loading