From 813f766828a1c084fee2c597c401e311c1c17704 Mon Sep 17 00:00:00 2001 From: Wes Tarle Date: Wed, 11 Mar 2026 16:30:39 -0400 Subject: [PATCH 1/2] refactor: use getter-style methods for ClientMetrics defaults --- v2/telemetry.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/v2/telemetry.go b/v2/telemetry.go index dc0191d7..f67db168 100644 --- a/v2/telemetry.go +++ b/v2/telemetry.go @@ -192,6 +192,20 @@ func WithExplicitBucketBoundaries(boundaries []float64) ClientMetricsOption { return &boundariesOpt{boundaries: boundaries} } +func (config *clientMetricsOptions) meterProvider() metric.MeterProvider { + if config.provider != nil { + return config.provider + } + return otel.GetMeterProvider() +} + +func (config *clientMetricsOptions) bucketBoundaries() []float64 { + if len(config.explicitBucketBoundaries) > 0 { + return config.explicitBucketBoundaries + } + return defaultHistogramBoundaries +} + // NewClientMetrics initializes and returns a new ClientMetrics instance. // It is intended to be called once per generated client during initialization. func NewClientMetrics(opts ...ClientMetricsOption) *ClientMetrics { @@ -200,10 +214,7 @@ func NewClientMetrics(opts ...ClientMetricsOption) *ClientMetrics { opt.Resolve(&config) } - provider := config.provider - if provider == nil { - provider = otel.GetMeterProvider() - } + provider := config.meterProvider() var meterAttrs []attribute.KeyValue if val, ok := config.attributes[ClientService]; ok { @@ -220,10 +231,7 @@ func NewClientMetrics(opts ...ClientMetricsOption) *ClientMetrics { meter := provider.Meter(config.attributes[ClientArtifact], meterOpts...) - boundaries := config.explicitBucketBoundaries - if len(boundaries) == 0 { - boundaries = defaultHistogramBoundaries - } + boundaries := config.bucketBoundaries() duration, _ := meter.Float64Histogram( metricName, From e8806988694445ace77a5feeff31799a591e27b8 Mon Sep 17 00:00:00 2001 From: Wes Tarle Date: Mon, 16 Mar 2026 13:29:49 -0400 Subject: [PATCH 2/2] refactor: use sync.OnceValue for strict encapsulation of ClientMetrics lazy initialization and add logger --- v2/telemetry.go | 138 ++++++++++++++++++++++++++++--------------- v2/telemetry_test.go | 104 ++++++++++++++++++++++++++++---- 2 files changed, 180 insertions(+), 62 deletions(-) diff --git a/v2/telemetry.go b/v2/telemetry.go index f67db168..1e0320a2 100644 --- a/v2/telemetry.go +++ b/v2/telemetry.go @@ -31,6 +31,8 @@ package gax import ( "context" + "log/slog" + "sync" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -136,33 +138,38 @@ var defaultHistogramBoundaries = []float64{ // for a specific generated Google Cloud client library. // There should be exactly one ClientMetrics instance instantiated per generated client. type ClientMetrics struct { + get func() clientMetricsData +} + +type clientMetricsData struct { duration metric.Float64Histogram attr []attribute.KeyValue } -type clientMetricsOptions struct { +type telemetryOptions struct { provider metric.MeterProvider attributes map[string]string explicitBucketBoundaries []float64 + logger *slog.Logger } -// ClientMetricsOption is an option to configure a ClientMetrics instance. -// ClientMetricsOption works by modifying relevant fields of clientMetricsOptions. -type ClientMetricsOption interface { +// TelemetryOption is an option to configure a ClientMetrics instance. +// TelemetryOption works by modifying relevant fields of telemetryOptions. +type TelemetryOption interface { // Resolve applies the option by modifying opts. - Resolve(opts *clientMetricsOptions) + Resolve(opts *telemetryOptions) } type providerOpt struct { p metric.MeterProvider } -func (p providerOpt) Resolve(opts *clientMetricsOptions) { +func (p providerOpt) Resolve(opts *telemetryOptions) { opts.provider = p.p } // WithMeterProvider specifies the metric.MeterProvider to use for instruments. -func WithMeterProvider(p metric.MeterProvider) ClientMetricsOption { +func WithMeterProvider(p metric.MeterProvider) TelemetryOption { return &providerOpt{p: p} } @@ -170,12 +177,12 @@ type attrOpt struct { attrs map[string]string } -func (a attrOpt) Resolve(opts *clientMetricsOptions) { +func (a attrOpt) Resolve(opts *telemetryOptions) { opts.attributes = a.attrs } -// WithClientMetricsAttributes specifies the static attributes attachments. -func WithClientMetricsAttributes(attr map[string]string) ClientMetricsOption { +// WithTelemetryAttributes specifies the static attributes attachments. +func WithTelemetryAttributes(attr map[string]string) TelemetryOption { return &attrOpt{attrs: attr} } @@ -183,23 +190,36 @@ type boundariesOpt struct { boundaries []float64 } -func (b boundariesOpt) Resolve(opts *clientMetricsOptions) { +func (b boundariesOpt) Resolve(opts *telemetryOptions) { opts.explicitBucketBoundaries = b.boundaries } // WithExplicitBucketBoundaries overrides the default histogram bucket boundaries. -func WithExplicitBucketBoundaries(boundaries []float64) ClientMetricsOption { +func WithExplicitBucketBoundaries(boundaries []float64) TelemetryOption { return &boundariesOpt{boundaries: boundaries} } -func (config *clientMetricsOptions) meterProvider() metric.MeterProvider { +type loggerOpt struct { + l *slog.Logger +} + +func (l loggerOpt) Resolve(opts *telemetryOptions) { + opts.logger = l.l +} + +// WithTelemetryLogger specifies a logger to record internal telemetry errors. +func WithTelemetryLogger(l *slog.Logger) TelemetryOption { + return &loggerOpt{l: l} +} + +func (config *telemetryOptions) meterProvider() metric.MeterProvider { if config.provider != nil { return config.provider } return otel.GetMeterProvider() } -func (config *clientMetricsOptions) bucketBoundaries() []float64 { +func (config *telemetryOptions) bucketBoundaries() []float64 { if len(config.explicitBucketBoundaries) > 0 { return config.explicitBucketBoundaries } @@ -208,48 +228,68 @@ func (config *clientMetricsOptions) bucketBoundaries() []float64 { // NewClientMetrics initializes and returns a new ClientMetrics instance. // It is intended to be called once per generated client during initialization. -func NewClientMetrics(opts ...ClientMetricsOption) *ClientMetrics { - var config clientMetricsOptions +func NewClientMetrics(opts ...TelemetryOption) *ClientMetrics { + var config telemetryOptions for _, opt := range opts { opt.Resolve(&config) } - provider := config.meterProvider() - - var meterAttrs []attribute.KeyValue - if val, ok := config.attributes[ClientService]; ok { - meterAttrs = append(meterAttrs, attribute.KeyValue{Key: attribute.Key(keyGCPClientService), Value: attribute.StringValue(val)}) - } - - meterOpts := []metric.MeterOption{ - metric.WithInstrumentationVersion(config.attributes[ClientVersion]), - metric.WithSchemaURL(schemaURL), - } - if len(meterAttrs) > 0 { - meterOpts = append(meterOpts, metric.WithInstrumentationAttributes(meterAttrs...)) + return &ClientMetrics{ + get: sync.OnceValue(func() clientMetricsData { + provider := config.meterProvider() + + var meterAttrs []attribute.KeyValue + if val, ok := config.attributes[ClientService]; ok { + meterAttrs = append(meterAttrs, attribute.KeyValue{Key: attribute.Key(keyGCPClientService), Value: attribute.StringValue(val)}) + } + + meterOpts := []metric.MeterOption{ + metric.WithInstrumentationVersion(config.attributes[ClientVersion]), + metric.WithSchemaURL(schemaURL), + } + if len(meterAttrs) > 0 { + meterOpts = append(meterOpts, metric.WithInstrumentationAttributes(meterAttrs...)) + } + + meter := provider.Meter(config.attributes[ClientArtifact], meterOpts...) + + boundaries := config.bucketBoundaries() + + duration, err := meter.Float64Histogram( + metricName, + metric.WithDescription(metricDescription), + metric.WithUnit("s"), + metric.WithExplicitBucketBoundaries(boundaries...), + ) + if err != nil && config.logger != nil { + config.logger.Warn("failed to initialize OTel duration histogram", "error", err) + } + + var attr []attribute.KeyValue + if val, ok := config.attributes[URLDomain]; ok { + attr = append(attr, attribute.KeyValue{Key: attribute.Key(keyURLDomain), Value: attribute.StringValue(val)}) + } + if val, ok := config.attributes[RPCSystem]; ok { + attr = append(attr, attribute.KeyValue{Key: attribute.Key(keyRPCSystemName), Value: attribute.StringValue(val)}) + } + return clientMetricsData{ + duration: duration, + attr: attr, + } + }), } +} - meter := provider.Meter(config.attributes[ClientArtifact], meterOpts...) - - boundaries := config.bucketBoundaries() - - duration, _ := meter.Float64Histogram( - metricName, - metric.WithDescription(metricDescription), - metric.WithUnit("s"), - metric.WithExplicitBucketBoundaries(boundaries...), - ) - - var attr []attribute.KeyValue - if val, ok := config.attributes[URLDomain]; ok { - attr = append(attr, attribute.KeyValue{Key: attribute.Key(keyURLDomain), Value: attribute.StringValue(val)}) - } - if val, ok := config.attributes[RPCSystem]; ok { - attr = append(attr, attribute.KeyValue{Key: attribute.Key(keyRPCSystemName), Value: attribute.StringValue(val)}) +func (cm *ClientMetrics) durationHistogram() metric.Float64Histogram { + if cm == nil || cm.get == nil { + return nil } + return cm.get().duration +} - return &ClientMetrics{ - duration: duration, - attr: attr, +func (cm *ClientMetrics) attributes() []attribute.KeyValue { + if cm == nil || cm.get == nil { + return nil } + return cm.get().attr } diff --git a/v2/telemetry_test.go b/v2/telemetry_test.go index fa2c7514..a74ba03b 100644 --- a/v2/telemetry_test.go +++ b/v2/telemetry_test.go @@ -30,7 +30,9 @@ package gax import ( + "bytes" "context" + "log/slog" "os/exec" "strings" "testing" @@ -47,15 +49,15 @@ func TestNewClientMetrics(t *testing.T) { tests := []struct { name string - opts []ClientMetricsOption + opts []TelemetryOption wantScopeAttr map[string]string wantDataAttr map[string]string useCustom bool }{ { name: "default boundaries and global provider", - opts: []ClientMetricsOption{ - WithClientMetricsAttributes(map[string]string{ + opts: []TelemetryOption{ + WithTelemetryAttributes(map[string]string{ ClientArtifact: "test-lib", ClientVersion: "v1.0.0", }), @@ -65,8 +67,8 @@ func TestNewClientMetrics(t *testing.T) { }, { name: "custom provider and custom boundaries", - opts: []ClientMetricsOption{ - WithClientMetricsAttributes(map[string]string{ + opts: []TelemetryOption{ + WithTelemetryAttributes(map[string]string{ ClientArtifact: "test-lib-2", ClientVersion: "v1.0.1", }), @@ -78,8 +80,8 @@ func TestNewClientMetrics(t *testing.T) { }, { name: "with static attributes", - opts: []ClientMetricsOption{ - WithClientMetricsAttributes(map[string]string{ + opts: []TelemetryOption{ + WithTelemetryAttributes(map[string]string{ ClientArtifact: "test-lib-3", ClientVersion: "v1.0.2", ClientService: "myservice", @@ -96,6 +98,30 @@ func TestNewClientMetrics(t *testing.T) { "url.domain": "test.domain", }, }, + { + name: "with logger", + opts: []TelemetryOption{ + WithTelemetryAttributes(map[string]string{ + ClientArtifact: "test-lib", + ClientVersion: "v1.0.0", + }), + WithTelemetryLogger(slog.Default()), + }, + wantScopeAttr: map[string]string{}, + wantDataAttr: map[string]string{}, + }, + { + name: "with nil logger", + opts: []TelemetryOption{ + WithTelemetryAttributes(map[string]string{ + ClientArtifact: "test-lib", + ClientVersion: "v1.0.0", + }), + WithTelemetryLogger(nil), + }, + wantScopeAttr: map[string]string{}, + wantDataAttr: map[string]string{}, + }, } for _, tt := range tests { @@ -104,7 +130,7 @@ func TestNewClientMetrics(t *testing.T) { if tt.useCustom { cmOpts = append(cmOpts, WithMeterProvider(customProvider)) cm := NewClientMetrics(cmOpts...) - if cm == nil || cm.duration == nil { + if cm == nil || cm.durationHistogram() == nil { t.Fatalf("expected initialized metrics") } return // we can't observe noop provider, so just verify it doesn't panic @@ -121,12 +147,12 @@ func TestNewClientMetrics(t *testing.T) { if cm == nil { t.Fatalf("NewClientMetrics returned nil") } - if cm.duration == nil { + if cm.durationHistogram() == nil { t.Fatalf("expected Float64Histogram to be initialized, got nil") } // Record a dummy value so we can collect the metrics and inspect Scope attributes - cm.duration.Record(context.Background(), 1.0, metric.WithAttributes(cm.attr...)) + cm.durationHistogram().Record(context.Background(), 1.0, metric.WithAttributes(cm.attributes()...)) var rm metricdata.ResourceMetrics if err := reader.Collect(context.Background(), &rm); err != nil { @@ -181,8 +207,8 @@ func TestNewClientMetrics(t *testing.T) { } func TestNewClientMetrics_GlobalFallback(t *testing.T) { - opts := []ClientMetricsOption{ - WithClientMetricsAttributes(map[string]string{ + opts := []TelemetryOption{ + WithTelemetryAttributes(map[string]string{ ClientArtifact: "test-global-fallback", }), } @@ -190,11 +216,63 @@ func TestNewClientMetrics_GlobalFallback(t *testing.T) { if cm == nil { t.Fatalf("expected non-nil ClientMetrics") } - if cm.duration == nil { + if cm.durationHistogram() == nil { t.Errorf("expected non-nil duration histogram") } } +func TestNewClientMetrics_InitializationError(t *testing.T) { + // Setup SDK + reader := sdkmetric.NewManualReader() + provider := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) + + t.Run("with logger", func(t *testing.T) { + // Capture log output + var buf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&buf, nil)) + + opts := []TelemetryOption{ + WithMeterProvider(provider), + WithTelemetryAttributes(map[string]string{ + ClientArtifact: "test-error", + }), + WithExplicitBucketBoundaries([]float64{10.0, 5.0}), // Invalid boundaries trigger error + WithTelemetryLogger(logger), + } + + cm := NewClientMetrics(opts...) + if cm == nil { + t.Fatalf("expected non-nil ClientMetrics") + } + + // Trigger lazy initialization, which should fail and log + cm.durationHistogram() + + logOutput := buf.String() + if !strings.Contains(logOutput, "failed to initialize OTel duration histogram") { + t.Errorf("expected initialization error to be logged, got: %s", logOutput) + } + }) + + t.Run("without logger", func(t *testing.T) { + opts := []TelemetryOption{ + WithMeterProvider(provider), + WithTelemetryAttributes(map[string]string{ + ClientArtifact: "test-error", + }), + WithExplicitBucketBoundaries([]float64{10.0, 5.0}), // Invalid boundaries trigger error + } + + cm := NewClientMetrics(opts...) + if cm == nil { + t.Fatalf("expected non-nil ClientMetrics") + } + + // Trigger lazy initialization, which should fail but NOT panic + cm.durationHistogram() + }) +} + func TestTelemetryConfigKeys(t *testing.T) { // These keys are referenced by generated client code. They must not be changed, // otherwise generated code will fail to compile or pass attributes incorrectly.