perf(bigtable): Refactor metric attributes for performance (#12445)

bhshkh · web-flow · commit f734ec685a78 · 2025-06-19T09:33:05.000-04:00
diff --git a/bigtable/bigtable.go b/bigtable/bigtable.go
@@ -2237,14 +2237,14 @@ func recordOperationCompletion(mt *builtinMetricsTracer) {
 
 	// Record operation_latencies
 	opLatAttrs, _ := mt.toOtelMetricAttrs(metricNameOperationLatencies)
-	mt.instrumentOperationLatencies.Record(mt.ctx, elapsedTimeMs, metric.WithAttributes(opLatAttrs...))
+	mt.instrumentOperationLatencies.Record(mt.ctx, elapsedTimeMs, metric.WithAttributeSet(opLatAttrs))
 
 	// Record retry_count
 	retryCntAttrs, _ := mt.toOtelMetricAttrs(metricNameRetryCount)
 	if mt.currOp.attemptCount > 1 {
 		// Only record when retry count is greater than 0 so the retry
 		// graph will be less confusing
-		mt.instrumentRetryCount.Add(mt.ctx, mt.currOp.attemptCount-1, metric.WithAttributes(retryCntAttrs...))
+		mt.instrumentRetryCount.Add(mt.ctx, mt.currOp.attemptCount-1, metric.WithAttributeSet(retryCntAttrs))
 	}
 }
 
@@ -2315,11 +2315,11 @@ func recordAttemptCompletion(mt *builtinMetricsTracer) {
 
 	// Record attempt_latencies
 	attemptLatAttrs, _ := mt.toOtelMetricAttrs(metricNameAttemptLatencies)
-	mt.instrumentAttemptLatencies.Record(mt.ctx, elapsedTime, metric.WithAttributes(attemptLatAttrs...))
+	mt.instrumentAttemptLatencies.Record(mt.ctx, elapsedTime, metric.WithAttributeSet(attemptLatAttrs))
 
 	// Record server_latencies
 	serverLatAttrs, _ := mt.toOtelMetricAttrs(metricNameServerLatencies)
 	if mt.currOp.currAttempt.serverLatencyErr == nil {
-		mt.instrumentServerLatencies.Record(mt.ctx, mt.currOp.currAttempt.serverLatency, metric.WithAttributes(serverLatAttrs...))
+		mt.instrumentServerLatencies.Record(mt.ctx, mt.currOp.currAttempt.serverLatency, metric.WithAttributeSet(serverLatAttrs))
 	}
 }
diff --git a/bigtable/metrics.go b/bigtable/metrics.go
@@ -66,6 +66,7 @@ const (
 	// Metric units
 	metricUnitMS    = "ms"
 	metricUnitCount = "1"
+	maxAttrsLen     = 12 // Monitored resource labels +  Metric labels
 )
 
 // These are effectively constant, but for testing purposes they are mutable
@@ -402,9 +403,10 @@ func (tf *builtinMetricsTracerFactory) createBuiltinMetricsTracer(ctx context.Co
 // - converts metric attributes values captured throughout the operation / attempt
 // to OpenTelemetry attributes format,
 // - combines these with common client attributes and returns
-func (mt *builtinMetricsTracer) toOtelMetricAttrs(metricName string) ([]attribute.KeyValue, error) {
+func (mt *builtinMetricsTracer) toOtelMetricAttrs(metricName string) (attribute.Set, error) {
+	attrKeyValues := make([]attribute.KeyValue, 0, maxAttrsLen)
 	// Create attribute key value pairs for attributes common to all metricss
-	attrKeyValues := []attribute.KeyValue{
+	attrKeyValues = append(attrKeyValues,
 		attribute.String(metricLabelKeyMethod, mt.method),
 
 		// Add resource labels to otel metric labels.
@@ -416,13 +418,13 @@ func (mt *builtinMetricsTracer) toOtelMetricAttrs(metricName string) ([]attribut
 		// use last attempt's cluster and zone
 		attribute.String(monitoredResLabelKeyCluster, mt.currOp.currAttempt.clusterID),
 		attribute.String(monitoredResLabelKeyZone, mt.currOp.currAttempt.zoneID),
-	}
+	)
 	attrKeyValues = append(attrKeyValues, mt.clientAttributes...)
 
 	// Get metric details
 	mDetails, found := metricsDetails[metricName]
 	if !found {
-		return attrKeyValues, fmt.Errorf("unable to create attributes list for unknown metric: %v", metricName)
+		return attribute.Set{}, fmt.Errorf("unable to create attributes list for unknown metric: %v", metricName)
 	}
 
 	status := mt.currOp.status
@@ -438,9 +440,10 @@ func (mt *builtinMetricsTracer) toOtelMetricAttrs(metricName string) ([]attribut
 		case metricLabelKeyStreamingOperation:
 			attrKeyValues = append(attrKeyValues, attribute.Bool(metricLabelKeyStreamingOperation, mt.isStreaming))
 		default:
-			return attrKeyValues, fmt.Errorf("unknown additional attribute: %v", attrKey)
+			return attribute.Set{}, fmt.Errorf("unknown additional attribute: %v", attrKey)
 		}
 	}
 
-	return attrKeyValues, nil
+	attrSet := attribute.NewSet(attrKeyValues...)
+	return attrSet, nil
 }
diff --git a/bigtable/metrics_test.go b/bigtable/metrics_test.go
@@ -452,24 +452,23 @@ func TestToOtelMetricAttrs(t *testing.T) {
 			desc:       "Unknown metric",
 			mt:         mt,
 			metricName: "unknown_metric",
-			wantAttrs: []attribute.KeyValue{
-				attribute.String(monitoredResLabelKeyTable, "my-table"),
-				attribute.String(metricLabelKeyMethod, "ReadRows"),
-				attribute.String(monitoredResLabelKeyCluster, clusterID1),
-				attribute.String(monitoredResLabelKeyZone, zoneID1),
-			},
-			wantError: fmt.Errorf("unable to create attributes list for unknown metric: unknown_metric"),
+			wantAttrs:  []attribute.KeyValue{}, // Expect empty slice on error
+			wantError:  fmt.Errorf("unable to create attributes list for unknown metric: unknown_metric"),
 		},
 	}
 
 	lessKeyValue := func(a, b attribute.KeyValue) bool { return a.Key < b.Key }
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			gotAttrs, gotErr := test.mt.toOtelMetricAttrs(test.metricName)
+			gotAttrSet, gotErr := test.mt.toOtelMetricAttrs(test.metricName)
 			if !equalErrs(gotErr, test.wantError) {
 				t.Errorf("error got: %v, want: %v", gotErr, test.wantError)
 			}
-			if diff := testutil.Diff(gotAttrs, test.wantAttrs,
+			gotAttrsSlice := gotAttrSet.ToSlice() // Convert Set to Slice
+			if gotAttrsSlice == nil {             // Ensure nil slice is treated as empty slice for comparison
+				gotAttrsSlice = []attribute.KeyValue{}
+			}
+			if diff := testutil.Diff(gotAttrsSlice, test.wantAttrs,
 				cmpopts.IgnoreUnexported(attribute.KeyValue{}, attribute.Value{}),
 				cmpopts.SortSlices(lessKeyValue)); diff != "" {
 				t.Errorf("got=-, want=+ \n%v", diff)
