Repository: open-telemetry/opentelemetry-go-contrib (go)

Issue:
otelgrpc high metric cardinality from rpc.server.duration
Since upgrading to **v1.12.0/0.37.0/0.6.0** & specifically https://github.com/open-telemetry/opentelemetry-go-contrib/pull/2700 we've seen an explosion in the cardinality of our metrics.

1. Metric recorded: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/ad04d9c71cb8974374ba8e3ee454fdaaddc3e345/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L346
2. Attributes are taken from `spanInfo`: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/ad04d9c71cb8974374ba8e3ee454fdaaddc3e345/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L331
3. Peer attributes added: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/ad04d9c71cb8974374ba8e3ee454fdaaddc3e345/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L476-L479

Is the intention that these types of high cardinality issues are handled in userland, by us using a view (or blocking at the ingestion layer) to strip labels from certain metrics after we find them problematic? On one hand I can understand that being acceptable, on the other, this feels like pushing a problem downstream to consumers that will suddenly get hit by a metric that is most likely going to grow very wide.

CC @fatsheep9146



Pull request:
otelgrpc: Remove high cardinality metric attributes
Related to #3536 
Closes #3071

This removes Peer attributes from grpc.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 57306076f..5741c6543 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -46,6 +46,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Upgrade dependencies of OpenTelemetry Go to use the new [`v1.19.0`/`v0.42.0`/`v0.0.7` release](https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.19.0).
 - Use `grpc.StatsHandler` for gRPC instrumentation in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/example`. (#4325)
 
+### Removed
+
+- 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
 
 ### Added
diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go
index 031d1f4df..c737d29dc 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),
@@ -277,7 +277,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),
@@ -386,8 +386,8 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor {
 		span.SetAttributes(grpcStatusCodeAttr)
 
 		elapsedTime := time.Since(before).Milliseconds()
-		attr = append(attr, grpcStatusCodeAttr)
-		cfg.rpcDuration.Record(ctx, float64(elapsedTime), metric.WithAttributes(attr...))
+		metricAttrs = append(metricAttrs, grpcStatusCodeAttr)
+		cfg.rpcDuration.Record(ctx, float64(elapsedTime), metric.WithAttributes(metricAttrs...))
 
 		return resp, err
 	}
@@ -468,7 +468,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,17 +498,18 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor {
 	}
 }
 
-// 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.


Test patch:
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 c64f59c76..6d8a82676 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"
@@ -665,12 +664,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 +680,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 +688,6 @@ func checkUnaryServerRecords(t *testing.T, reader metric.Reader) {
 								semconv.RPCService("grpc.testing.TestService"),
 								otelgrpc.RPCSystemGRPC,
 								otelgrpc.GRPCStatusCodeKey.Int64(int64(codes.OK)),
-								address,
-								port,
 							),
 						},
 					},
@@ -718,26 +707,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
-}