From cfc417e8ed23b5b7c04d9fd615f5e66ffbe9289a Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 21 Sep 2023 09:03:13 -0500 Subject: [PATCH 1/9] Removed high cardinality attributes from grpc metrics. --- CHANGELOG.md | 4 ++++ .../google.golang.org/grpc/otelgrpc/interceptor.go | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b149fa66d4..a6574b6ddb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Set the description for the `rpc.server.duration` metric in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#4302) - Add `NewServerHandler` and `NewClientHandler` that return a `grpc.StatsHandler` used for gRPC instrumentation in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#3002) +### Removed + +- The The high cardinality attributes `net.sock.peer.*` and `net.peer.*` were removed from the metrics generated by `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#????) + ## [1.19.0/0.44.0/0.13.0] - 2023-09-12 ### Added diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go index 561154061fe..fc9581565f5 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go @@ -368,7 +368,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { var statusCode grpc_codes.Code defer func(t time.Time) { elapsedTime := time.Since(t) / time.Millisecond - attr = append(attr, semconv.RPCGRPCStatusCodeKey.Int64(int64(statusCode))) + attr := append(metricAttributes(info.FullMethod), semconv.RPCGRPCStatusCodeKey.Int64(int64(statusCode))) o := metric.WithAttributes(attr...) cfg.rpcServerDuration.Record(ctx, int64(elapsedTime), o) }(time.Now()) @@ -498,6 +498,11 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { } } +func metricAttributes(fullMethod string) []attribute.KeyValue { + _, attrs := internal.ParseFullMethod(fullMethod) + return append(attrs, RPCSystemGRPC) +} + // spanInfo returns a span name and all appropriate attributes from the gRPC // method and peer address. func spanInfo(fullMethod, peerAddress string) (string, []attribute.KeyValue) { From 881ec592a9ae5f9607f10df203e8c7539db4663e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 21 Sep 2023 17:38:50 +0200 Subject: [PATCH 2/9] Update CHANGELOG.md Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6574b6ddb6..6949fda4946 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Removed -- The The high cardinality attributes `net.sock.peer.*` and `net.peer.*` were removed from the metrics generated by `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#????) +- The The high cardinality attributes `net.sock.peer.*` and `net.peer.*` were removed from the metrics generated by `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#4322) ## [1.19.0/0.44.0/0.13.0] - 2023-09-12 From 9b0efe5e047396409ffe20b28acf5cfe72fc03d7 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 21 Sep 2023 11:16:13 -0500 Subject: [PATCH 3/9] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6949fda4946..879e6794c78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Removed -- The The high cardinality attributes `net.sock.peer.*` and `net.peer.*` were removed from the metrics generated by `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#4322) +- The `net.sock.peer.*` and `net.peer.*` high cardinality attributes are removed from the metrics generated by `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#4322) ## [1.19.0/0.44.0/0.13.0] - 2023-09-12 From c68dd6b8a27e23445ea092145d9c7514fffcf24d Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Mon, 25 Sep 2023 14:56:27 -0500 Subject: [PATCH 4/9] reduce to one function call --- .../grpc/otelgrpc/interceptor.go | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go index fc9581565f5..cd506e6bec2 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go @@ -83,7 +83,7 @@ func UnaryClientInterceptor(opts ...Option) grpc.UnaryClientInterceptor { return invoker(ctx, method, req, reply, cc, callOpts...) } - name, attr := spanInfo(method, cc.Target()) + name, attr, _ := telemetryAttributes(method, cc.Target()) startOpts := append([]trace.SpanStartOption{ trace.WithSpanKind(trace.SpanKindClient), @@ -278,7 +278,7 @@ func StreamClientInterceptor(opts ...Option) grpc.StreamClientInterceptor { return streamer(ctx, desc, cc, method, callOpts...) } - name, attr := spanInfo(method, cc.Target()) + name, attr, _ := telemetryAttributes(method, cc.Target()) startOpts := append([]trace.SpanStartOption{ trace.WithSpanKind(trace.SpanKindClient), @@ -346,7 +346,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { } ctx = extract(ctx, cfg.Propagators) - name, attr := spanInfo(info.FullMethod, peerFromCtx(ctx)) + name, attr, metricAttrs := telemetryAttributes(info.FullMethod, peerFromCtx(ctx)) startOpts := append([]trace.SpanStartOption{ trace.WithSpanKind(trace.SpanKindServer), @@ -368,7 +368,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { var statusCode grpc_codes.Code defer func(t time.Time) { elapsedTime := time.Since(t) / time.Millisecond - attr := append(metricAttributes(info.FullMethod), semconv.RPCGRPCStatusCodeKey.Int64(int64(statusCode))) + attr := append(metricAttrs, semconv.RPCGRPCStatusCodeKey.Int64(int64(statusCode))) o := metric.WithAttributes(attr...) cfg.rpcServerDuration.Record(ctx, int64(elapsedTime), o) }(time.Now()) @@ -469,7 +469,7 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { } ctx = extract(ctx, cfg.Propagators) - name, attr := spanInfo(info.FullMethod, peerFromCtx(ctx)) + name, attr, _ := telemetryAttributes(info.FullMethod, peerFromCtx(ctx)) startOpts := append([]trace.SpanStartOption{ trace.WithSpanKind(trace.SpanKindServer), @@ -498,22 +498,18 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { } } -func metricAttributes(fullMethod string) []attribute.KeyValue { - _, attrs := internal.ParseFullMethod(fullMethod) - return append(attrs, RPCSystemGRPC) -} - -// spanInfo returns a span name and all appropriate attributes from the gRPC -// method and peer address. -func spanInfo(fullMethod, peerAddress string) (string, []attribute.KeyValue) { - name, mAttrs := internal.ParseFullMethod(fullMethod) +// telemetryAttributes returns a span name and span and metric attributes from +// the gRPC method and peer address. +func telemetryAttributes(fullMethod, peerAddress string) (string, []attribute.KeyValue, []attribute.KeyValue) { + name, methodAttrs := internal.ParseFullMethod(fullMethod) peerAttrs := peerAttr(peerAddress) - attrs := make([]attribute.KeyValue, 0, 1+len(mAttrs)+len(peerAttrs)) + attrs := make([]attribute.KeyValue, 0, 1+len(methodAttrs)+len(peerAttrs)) attrs = append(attrs, RPCSystemGRPC) - attrs = append(attrs, mAttrs...) + attrs = append(attrs, methodAttrs...) + metricAttrs := attrs[:1+len(methodAttrs)] attrs = append(attrs, peerAttrs...) - return name, attrs + return name, attrs, metricAttrs } // peerAttr returns attributes about the peer address. From 3843303f49f7e1e3b9d41a98d1c3b081f87a50c6 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Tue, 26 Sep 2023 14:19:16 -0500 Subject: [PATCH 5/9] Address PR feeback --- instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go index cd506e6bec2..cd8512374b8 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go @@ -368,7 +368,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { var statusCode grpc_codes.Code defer func(t time.Time) { elapsedTime := time.Since(t) / time.Millisecond - attr := append(metricAttrs, semconv.RPCGRPCStatusCodeKey.Int64(int64(statusCode))) + attr := append([]attribute.KeyValue{semconv.RPCGRPCStatusCodeKey.Int64(int64(statusCode))}, metricAttrs...) o := metric.WithAttributes(attr...) cfg.rpcServerDuration.Record(ctx, int64(elapsedTime), o) }(time.Now()) From 8f97eaf50d98f0748a8aa09d84c209fef370b4fa Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Mon, 6 Nov 2023 10:38:01 -0600 Subject: [PATCH 6/9] Fix linting --- instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go index ad06903beeb..c737d29dcd0 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go @@ -387,7 +387,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { elapsedTime := time.Since(before).Milliseconds() metricAttrs = append(metricAttrs, grpcStatusCodeAttr) - cfg.rpcDuration.Record(ctx, float64(elapsedTime), metric.WithAttributes(metricAttrs...)) + cfg.rpcDuration.Record(ctx, float64(elapsedTime), metric.WithAttributes(metricAttrs...)) return resp, err } From 55653daf2ad619bebc3ac44e8189f99e9cbfed92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Tue, 7 Nov 2023 00:04:45 +0100 Subject: [PATCH 7/9] Update grpc_test.go --- .../google.golang.org/grpc/otelgrpc/test/grpc_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go index c64f59c764f..026b75a659f 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go @@ -665,12 +665,6 @@ func checkUnaryServerRecords(t *testing.T, reader metric.Reader) { assert.NoError(t, err) require.Len(t, rm.ScopeMetrics, 1) - // TODO: Remove these #4322 - address, ok := findScopeMetricAttribute(rm.ScopeMetrics[0], semconv.NetSockPeerAddrKey) - assert.True(t, ok) - port, ok := findScopeMetricAttribute(rm.ScopeMetrics[0], semconv.NetSockPeerPortKey) - assert.True(t, ok) - want := metricdata.ScopeMetrics{ Scope: wantInstrumentationScope, Metrics: []metricdata.Metrics{ @@ -687,8 +681,6 @@ func checkUnaryServerRecords(t *testing.T, reader metric.Reader) { semconv.RPCService("grpc.testing.TestService"), otelgrpc.RPCSystemGRPC, otelgrpc.GRPCStatusCodeKey.Int64(int64(codes.OK)), - address, - port, ), }, { @@ -697,8 +689,6 @@ func checkUnaryServerRecords(t *testing.T, reader metric.Reader) { semconv.RPCService("grpc.testing.TestService"), otelgrpc.RPCSystemGRPC, otelgrpc.GRPCStatusCodeKey.Int64(int64(codes.OK)), - address, - port, ), }, }, From 58e33383a3973b46ae4b96b7a6327f6649a82341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Tue, 7 Nov 2023 00:12:12 +0100 Subject: [PATCH 8/9] Update grpc_test.go --- .../grpc/otelgrpc/test/grpc_test.go | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go index 026b75a659f..21295ee0830 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go @@ -708,26 +708,3 @@ func findAttribute(kvs []attribute.KeyValue, key attribute.Key) (attribute.KeyVa } return attribute.KeyValue{}, false } - -func findScopeMetricAttribute(sm metricdata.ScopeMetrics, key attribute.Key) (attribute.KeyValue, bool) { - for _, m := range sm.Metrics { - // This only needs to cover data types used by the instrumentation. - switch d := m.Data.(type) { - case metricdata.Histogram[int64]: - for _, dp := range d.DataPoints { - if kv, ok := findAttribute(dp.Attributes.ToSlice(), key); ok { - return kv, true - } - } - case metricdata.Histogram[float64]: - for _, dp := range d.DataPoints { - if kv, ok := findAttribute(dp.Attributes.ToSlice(), key); ok { - return kv, true - } - } - default: - panic(fmt.Sprintf("unexpected data type %T - name %s", d, m.Name)) - } - } - return attribute.KeyValue{}, false -} From 53ba6bd70dcae84c86ebf57ff52d245ecfa3f638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Tue, 7 Nov 2023 00:15:05 +0100 Subject: [PATCH 9/9] Update grpc_test.go --- .../google.golang.org/grpc/otelgrpc/test/grpc_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go index 21295ee0830..6d8a82676ba 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_test.go @@ -16,7 +16,6 @@ package test import ( "context" - "fmt" "net" "strconv" "testing"