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.

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
enable otel dial option to pass empty MetricOptions{}
  • Loading branch information
aranjans committed Dec 19, 2024
commit 56d4bedc32d951324be031c5aaea6c69dddbdad9
25 changes: 16 additions & 9 deletions stats/opentelemetry/client_metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package opentelemetry

import (
"context"
"go.opentelemetry.io/otel/sdk/metric"
"sync/atomic"
"time"

Expand All @@ -38,29 +39,35 @@ type clientStatsHandler struct {
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

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
}

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 = &NoopMetricsRecorder{}
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
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)
}
}
19 changes: 19 additions & 0 deletions stats/opentelemetry/opentelemetry.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,3 +394,22 @@ var (
func DefaultMetrics() *stats.MetricSet {
return defaultPerCallMetrics.Join(estats.DefaultMetrics)
}

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

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

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

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

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

// RecordInt64Gauge is a noop implementation of RecordInt64Gauge.
func (r *NoopMetricsRecorder) RecordInt64Gauge(*estats.Int64GaugeHandle, int64, ...string) {}
26 changes: 17 additions & 9 deletions stats/opentelemetry/server_metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package opentelemetry

import (
"context"
"go.opentelemetry.io/otel/sdk/metric"
"sync/atomic"
"time"

Expand All @@ -38,27 +39,34 @@ type serverStatsHandler struct {
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
}
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 = &NoopMetricsRecorder{}
h.options.MetricsOptions.MeterProvider = metric.NewMeterProvider()
h.setServerMetrics()
return
}

metrics, meter := h.setServerMetrics()
rm := &registryMetrics{
optionalLabels: h.options.MetricsOptions.OptionalLabels,
}
Expand Down