diff --git a/CHANGELOG.md b/CHANGELOG.md index 99f30731d85..39479c4cc31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add unmarshaling and validation for `BatchLogRecordProcessor`, `BatchSpanProcessor`, and `PeriodicMetricReader` to v1.0.0 model in `go.opentelemetry.io/contrib/otelconf`. (#8049) - Add unmarshaling and validation for `TextMapPropagator` to v1.0.0 model in `go.opentelemetry.io/contrib/otelconf`. (#8052) +### Changed + +- Improve performance by reducing allocations in the gRPC stats handler in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#8035) + ### Removed - Drop support for [Go 1.23]. (#7831) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go index 29d7ab2bdac..278f6d0d99e 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go @@ -26,10 +26,11 @@ import ( type gRPCContextKey struct{} type gRPCContext struct { - inMessages int64 - outMessages int64 - metricAttrs []attribute.KeyValue - record bool + inMessages int64 + outMessages int64 + metricAttrs []attribute.KeyValue + metricAttrSet attribute.Set + record bool } type serverHandler struct { @@ -38,8 +39,8 @@ type serverHandler struct { tracer trace.Tracer duration rpcconv.ServerDuration - inSize rpcconv.ServerRequestSize - outSize rpcconv.ServerResponseSize + inSize int64Hist + outSize int64Hist inMsg rpcconv.ServerRequestsPerRPC outMsg rpcconv.ServerResponsesPerRPC } @@ -111,9 +112,12 @@ func (h *serverHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) cont } if record { + // Make a new slice to avoid aliasing into the same attrs slice used by metrics. + spanAttributes := make([]attribute.KeyValue, 0, len(attrs)+len(h.SpanAttributes)) + spanAttributes = append(append(spanAttributes, attrs...), h.SpanAttributes...) opts := []trace.SpanStartOption{ trace.WithSpanKind(trace.SpanKindServer), - trace.WithAttributes(append(attrs, h.SpanAttributes...)...), + trace.WithAttributes(spanAttributes...), } if h.PublicEndpoint || (h.PublicEndpointFn != nil && h.PublicEndpointFn(ctx, info)) { opts = append(opts, trace.WithNewRoot()) @@ -133,6 +137,7 @@ func (h *serverHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) cont metricAttrs: append(attrs, h.MetricAttributes...), record: record, } + gctx.metricAttrSet = attribute.NewSet(gctx.metricAttrs...) return context.WithValue(ctx, gRPCContextKey{}, &gctx) } @@ -157,8 +162,8 @@ type clientHandler struct { tracer trace.Tracer duration rpcconv.ClientDuration - inSize rpcconv.ClientResponseSize - outSize rpcconv.ClientRequestSize + inSize int64Hist + outSize int64Hist inMsg rpcconv.ClientResponsesPerRPC outMsg rpcconv.ClientRequestsPerRPC } @@ -219,11 +224,14 @@ func (h *clientHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) cont } if record { + // Make a new slice to avoid aliasing into the same attrs slice used by metrics. + spanAttributes := make([]attribute.KeyValue, 0, len(attrs)+len(h.SpanAttributes)) + spanAttributes = append(append(spanAttributes, attrs...), h.SpanAttributes...) ctx, _ = h.tracer.Start( ctx, name, trace.WithSpanKind(trace.SpanKindClient), - trace.WithAttributes(append(attrs, h.SpanAttributes...)...), + trace.WithAttributes(spanAttributes...), ) } @@ -231,6 +239,7 @@ func (h *clientHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) cont metricAttrs: append(attrs, h.MetricAttributes...), record: record, } + gctx.metricAttrSet = attribute.NewSet(gctx.metricAttrs...) return inject(context.WithValue(ctx, gRPCContextKey{}, &gctx), h.Propagators) } @@ -262,7 +271,7 @@ func (*clientHandler) HandleConn(context.Context, stats.ConnStats) { } type int64Hist interface { - Record(context.Context, int64, ...attribute.KeyValue) + RecordSet(context.Context, int64, attribute.Set) } func (c *config) handleRPC( @@ -286,7 +295,7 @@ func (c *config) handleRPC( case *stats.InPayload: if gctx != nil { messageId = atomic.AddInt64(&gctx.inMessages, 1) - inSize.Record(ctx, int64(rs.Length), gctx.metricAttrs...) + inSize.RecordSet(ctx, int64(rs.Length), gctx.metricAttrSet) } if c.ReceivedEvent && span.IsRecording() { @@ -302,7 +311,7 @@ func (c *config) handleRPC( case *stats.OutPayload: if gctx != nil { messageId = atomic.AddInt64(&gctx.outMessages, 1) - outSize.Record(ctx, int64(rs.Length), gctx.metricAttrs...) + outSize.RecordSet(ctx, int64(rs.Length), gctx.metricAttrSet) } if c.SentEvent && span.IsRecording() { @@ -343,6 +352,9 @@ func (c *config) handleRPC( var metricAttrs []attribute.KeyValue if gctx != nil { + // Don't use gctx.metricAttrSet here, because it requires passing + // multiple RecordOptions, which would call metric.mergeSets and + // allocate a new set for each Record call. metricAttrs = make([]attribute.KeyValue, 0, len(gctx.metricAttrs)+1) metricAttrs = append(metricAttrs, gctx.metricAttrs...) } diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_bench_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_bench_test.go index 1b3b04109b7..56bbc39390c 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_bench_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_bench_test.go @@ -10,6 +10,7 @@ import ( "time" metricnoop "go.opentelemetry.io/otel/metric/noop" + "go.opentelemetry.io/otel/sdk/metric" "go.opentelemetry.io/otel/sdk/trace" tracenoop "go.opentelemetry.io/otel/trace/noop" "google.golang.org/grpc/peer" @@ -29,7 +30,7 @@ func benchmarkServerHandlerHandleRPC(b *testing.B, stat stats.RPCStats) { WithTracerProvider(trace.NewTracerProvider( trace.WithSampler(trace.AlwaysSample()), )), - WithMeterProvider(metricnoop.NewMeterProvider()), + WithMeterProvider(metric.NewMeterProvider()), WithMessageEvents(ReceivedEvents, SentEvents), ) ctx := b.Context() diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_test.go index b67b44c75fd..3424247b796 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/embedded" "go.opentelemetry.io/otel/propagation" @@ -178,8 +179,8 @@ func TestNilInstruments(t *testing.T) { h := hIface.(*serverHandler) assert.NotPanics(t, func() { h.duration.Record(ctx, 0) }, "duration") - assert.NotPanics(t, func() { h.inSize.Record(ctx, 0) }, "inSize") - assert.NotPanics(t, func() { h.outSize.Record(ctx, 0) }, "outSize") + assert.NotPanics(t, func() { h.inSize.RecordSet(ctx, 0, *attribute.EmptySet()) }, "inSize") + assert.NotPanics(t, func() { h.outSize.RecordSet(ctx, 0, *attribute.EmptySet()) }, "outSize") assert.NotPanics(t, func() { h.inMsg.Record(ctx, 0) }, "inMsg") assert.NotPanics(t, func() { h.outMsg.Record(ctx, 0) }, "outMsg") }) @@ -192,8 +193,8 @@ func TestNilInstruments(t *testing.T) { h := hIface.(*clientHandler) assert.NotPanics(t, func() { h.duration.Record(ctx, 0) }, "duration") - assert.NotPanics(t, func() { h.inSize.Record(ctx, 0) }, "inSize") - assert.NotPanics(t, func() { h.outSize.Record(ctx, 0) }, "outSize") + assert.NotPanics(t, func() { h.inSize.RecordSet(ctx, 0, *attribute.EmptySet()) }, "inSize") + assert.NotPanics(t, func() { h.outSize.RecordSet(ctx, 0, *attribute.EmptySet()) }, "outSize") assert.NotPanics(t, func() { h.inMsg.Record(ctx, 0) }, "inMsg") assert.NotPanics(t, func() { h.outMsg.Record(ctx, 0) }, "outMsg") })