Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stats/opentelemetry: enable otel dial option to pass empty MetricOptions{} #7952

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

stats/opentelemetry: enable otel dial option to pass empty MetricOptions{} #7952

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56d4bed
enable otel dial option to pass empty MetricOptions{}
aranjans Dec 19, 2024
101e23d
refactor
aranjans Dec 19, 2024
f2a9f2a
fix breaking tests
aranjans Dec 19, 2024
b18544c
make vet happy
aranjans Dec 19, 2024
82b760d
set the metrics in setClientMetrics to an empty set
aranjans Jan 3, 2025
f5af085
make vet happy
aranjans Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions stats/opentelemetry/client_metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

otelattribute "go.opentelemetry.io/otel/attribute"
otelmetric "go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/sdk/metric"
)

type clientStatsHandler struct {
Expand All @@ -38,29 +39,35 @@
clientMetrics clientMetrics
}

func (h *clientStatsHandler) initializeMetrics() {
// Will set no metrics to record, logically making this stats handler a
// no-op.
if h.options.MetricsOptions.MeterProvider == nil {
return
}
Comment on lines -44 to -46
Copy link
Contributor

@arjan-bal arjan-bal Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only changes needed may be to set the metrics and the MetricsProvider to no-op versions, rest of the changes can be reverted.

Suggested change
if h.options.MetricsOptions.MeterProvider == nil {
return
}
if h.options.MetricsOptions.MeterProvider == nil {
h.clientMetrics.attemptStarted = noop.Int64Counter{}
h.clientMetrics.attemptDuration = noop.Float64Histogram{}
h.clientMetrics.attemptSentTotalCompressedMessageSize = noop.Int64Histogram{}
h.clientMetrics.attemptRcvdTotalCompressedMessageSize = noop.Int64Histogram{}
h.clientMetrics.callDuration = noop.Float64Histogram{}
h.MetricsRecorder = &registryMetrics{}
return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set the metrics in setClientMetrics to an empty set using stats.NewMetricSet(), we end up getting no-op implementations for the clientMetrics and registryMetrics. We can use this fact to unify the code paths for MeterProvider == nil and MeterProvider != nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arjan-bal I have updated the PR with suggested change above. Could you clarify this comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current approach, the list of metrics (attemptStarted, attemptDuration, attemptSentTotalCompressedMessageSize etc.) is duplicated. If we set metrics to an empty set using stats.NewMetricSet(), the existing code will set every metric to the corresponding noop histogram, avoiding duplication.


func (h *clientStatsHandler) setClientMetrics() (*estats.Metrics, otelmetric.Meter) {
meter := h.options.MetricsOptions.MeterProvider.Meter("grpc-go", otelmetric.WithInstrumentationVersion(grpc.Version))
if meter == nil {
return
return nil, nil

Check warning on line 45 in stats/opentelemetry/client_metrics.go

View check run for this annotation

Codecov / codecov/patch

stats/opentelemetry/client_metrics.go#L45 

Added line #L45 was not covered by tests
}

metrics := h.options.MetricsOptions.Metrics
if metrics == nil {
metrics = DefaultMetrics()
}

h.clientMetrics.attemptStarted = createInt64Counter(metrics.Metrics(), "grpc.client.attempt.started", meter, otelmetric.WithUnit("attempt"), otelmetric.WithDescription("Number of client call attempts started."))
h.clientMetrics.attemptDuration = createFloat64Histogram(metrics.Metrics(), "grpc.client.attempt.duration", meter, otelmetric.WithUnit("s"), otelmetric.WithDescription("End-to-end time taken to complete a client call attempt."), otelmetric.WithExplicitBucketBoundaries(DefaultLatencyBounds...))
h.clientMetrics.attemptSentTotalCompressedMessageSize = createInt64Histogram(metrics.Metrics(), "grpc.client.attempt.sent_total_compressed_message_size", meter, otelmetric.WithUnit("By"), otelmetric.WithDescription("Compressed message bytes sent per client call attempt."), otelmetric.WithExplicitBucketBoundaries(DefaultSizeBounds...))
h.clientMetrics.attemptRcvdTotalCompressedMessageSize = createInt64Histogram(metrics.Metrics(), "grpc.client.attempt.rcvd_total_compressed_message_size", meter, otelmetric.WithUnit("By"), otelmetric.WithDescription("Compressed message bytes received per call attempt."), otelmetric.WithExplicitBucketBoundaries(DefaultSizeBounds...))
h.clientMetrics.callDuration = createFloat64Histogram(metrics.Metrics(), "grpc.client.call.duration", meter, otelmetric.WithUnit("s"), otelmetric.WithDescription("Time taken by gRPC to complete an RPC from application's perspective."), otelmetric.WithExplicitBucketBoundaries(DefaultLatencyBounds...))
return metrics, meter
}

func (h *clientStatsHandler) initializeMetrics() {
// Will set no metrics to record, logically making this stats handler a
// no-op.
if h.options.MetricsOptions.MeterProvider == nil {
h.MetricsRecorder = &OtelNoopMetricsRecorder{}
h.options.MetricsOptions.MeterProvider = metric.NewMeterProvider()
h.setClientMetrics()
return
}

metrics, meter := h.setClientMetrics()
rm := &registryMetrics{
optionalLabels: h.options.MetricsOptions.OptionalLabels,
}
Expand Down
48 changes: 48 additions & 0 deletions stats/opentelemetry/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,3 +582,51 @@ func pollForWantMetrics(ctx context.Context, t *testing.T, reader *metric.Manual

return fmt.Errorf("error waiting for metrics %v: %v", wantMetrics, ctx.Err())
}

// TestMetricsDisabled verifies that RPCs call succeed as expected when
// metrics option is disabled in the OpenTelemetry instrumentation.
func (s) TestMetricsDisabled(t *testing.T) {
ss := &stubserver.StubServer{
UnaryCallF: func(_ context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) {
return &testpb.SimpleResponse{Payload: &testpb.Payload{
Body: make([]byte, len(in.GetPayload().GetBody())),
}}, nil
},
FullDuplexCallF: func(stream testgrpc.TestService_FullDuplexCallServer) error {
for {
_, err := stream.Recv()
if err == io.EOF {
return nil
}
}
},
}

otelOptions := opentelemetry.Options{
MetricsOptions: opentelemetry.MetricsOptions{},
}

if err := ss.Start([]grpc.ServerOption{opentelemetry.ServerOption(otelOptions)}, opentelemetry.DialOption(otelOptions)); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()

// Make two RPCs, a unary RPC and a streaming RPC.
if _, err := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{Payload: &testpb.Payload{
Body: make([]byte, 10000),
}}); err != nil {
t.Fatalf("Unexpected error from UnaryCall: %v", err)
}
stream, err := ss.Client.FullDuplexCall(ctx)
if err != nil {
t.Fatalf("ss.Client.FullDuplexCall failed: %v", err)
}

stream.CloseSend()
if _, err = stream.Recv(); err != io.EOF {
t.Fatalf("stream.Recv received an unexpected error: %v, expected an EOF error", err)
}
}
21 changes: 21 additions & 0 deletions stats/opentelemetry/opentelemetry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,3 +394,24 @@
func DefaultMetrics() *stats.MetricSet {
return defaultPerCallMetrics.Join(estats.DefaultMetrics)
}

// OtelNoopMetricsRecorder is a noop MetricsRecorder to be used to prevent
// nil panics.
type OtelNoopMetricsRecorder struct{}

// RecordInt64Count is a noop implementation of RecordInt64Count.
func (r *OtelNoopMetricsRecorder) RecordInt64Count(*estats.Int64CountHandle, int64, ...string) {}

// RecordFloat64Count is a noop implementation of RecordFloat64Count.
func (r *OtelNoopMetricsRecorder) RecordFloat64Count(*estats.Float64CountHandle, float64, ...string) {

Check warning on line 406 in stats/opentelemetry/opentelemetry.go

View check run for this annotation

Codecov / codecov/patch

stats/opentelemetry/opentelemetry.go#L406 

Added line #L406 was not covered by tests
}

// RecordInt64Histo is a noop implementation of RecordInt64Histo.
func (r *OtelNoopMetricsRecorder) RecordInt64Histo(*estats.Int64HistoHandle, int64, ...string) {}

Check warning on line 410 in stats/opentelemetry/opentelemetry.go

View check run for this annotation

Codecov / codecov/patch

stats/opentelemetry/opentelemetry.go#L410 

Added line #L410 was not covered by tests

// RecordFloat64Histo is a noop implementation of RecordFloat64Histo.
func (r *OtelNoopMetricsRecorder) RecordFloat64Histo(*estats.Float64HistoHandle, float64, ...string) {

Check warning on line 413 in stats/opentelemetry/opentelemetry.go

View check run for this annotation

Codecov / codecov/patch

stats/opentelemetry/opentelemetry.go#L413 

Added line #L413 was not covered by tests
}

// RecordInt64Gauge is a noop implementation of RecordInt64Gauge.
func (r *OtelNoopMetricsRecorder) RecordInt64Gauge(*estats.Int64GaugeHandle, int64, ...string) {}

Check warning on line 417 in stats/opentelemetry/opentelemetry.go

View check run for this annotation

Codecov / codecov/patch

stats/opentelemetry/opentelemetry.go#L417 

Added line #L417 was not covered by tests
26 changes: 17 additions & 9 deletions stats/opentelemetry/server_metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

otelattribute "go.opentelemetry.io/otel/attribute"
otelmetric "go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/sdk/metric"
)

type serverStatsHandler struct {
Expand All @@ -38,27 +39,34 @@
serverMetrics serverMetrics
}

func (h *serverStatsHandler) initializeMetrics() {
// Will set no metrics to record, logically making this stats handler a
// no-op.
if h.options.MetricsOptions.MeterProvider == nil {
return
}

func (h *serverStatsHandler) setServerMetrics() (*estats.Metrics, otelmetric.Meter) {
meter := h.options.MetricsOptions.MeterProvider.Meter("grpc-go", otelmetric.WithInstrumentationVersion(grpc.Version))
if meter == nil {
return
return nil, nil

Check warning on line 45 in stats/opentelemetry/server_metrics.go

View check run for this annotation

Codecov / codecov/patch

stats/opentelemetry/server_metrics.go#L45 

Added line #L45 was not covered by tests
}
metrics := h.options.MetricsOptions.Metrics
if metrics == nil {
metrics = DefaultMetrics()
}

h.serverMetrics.callStarted = createInt64Counter(metrics.Metrics(), "grpc.server.call.started", meter, otelmetric.WithUnit("call"), otelmetric.WithDescription("Number of server calls started."))
h.serverMetrics.callSentTotalCompressedMessageSize = createInt64Histogram(metrics.Metrics(), "grpc.server.call.sent_total_compressed_message_size", meter, otelmetric.WithUnit("By"), otelmetric.WithDescription("Compressed message bytes sent per server call."), otelmetric.WithExplicitBucketBoundaries(DefaultSizeBounds...))
h.serverMetrics.callRcvdTotalCompressedMessageSize = createInt64Histogram(metrics.Metrics(), "grpc.server.call.rcvd_total_compressed_message_size", meter, otelmetric.WithUnit("By"), otelmetric.WithDescription("Compressed message bytes received per server call."), otelmetric.WithExplicitBucketBoundaries(DefaultSizeBounds...))
h.serverMetrics.callDuration = createFloat64Histogram(metrics.Metrics(), "grpc.server.call.duration", meter, otelmetric.WithUnit("s"), otelmetric.WithDescription("End-to-end time taken to complete a call from server transport's perspective."), otelmetric.WithExplicitBucketBoundaries(DefaultLatencyBounds...))

return metrics, meter
}

func (h *serverStatsHandler) initializeMetrics() {
// Will set no metrics to record, logically making this stats handler a
// no-op.
if h.options.MetricsOptions.MeterProvider == nil {
h.MetricsRecorder = &OtelNoopMetricsRecorder{}
h.options.MetricsOptions.MeterProvider = metric.NewMeterProvider()
h.setServerMetrics()
return
}

metrics, meter := h.setServerMetrics()
rm := &registryMetrics{
optionalLabels: h.options.MetricsOptions.OptionalLabels,
}
Expand Down
2 changes: 1 addition & 1 deletion test/creds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ func (s) TestWaitForReadyRPCErrorOnBadCertificates(t *testing.T) {
defer te.tearDown()

opts := []grpc.DialOption{grpc.WithTransportCredentials(clientAlwaysFailCred{})}
cc, err := grpc.NewClient(te.srvAddr, opts...)
cc, err := grpc.Dial(te.srvAddr, opts...)
if err != nil {
t.Fatalf("NewClient(_) = %v, want %v", err, nil)
}
Expand Down
Loading