fix(redisotel): fix buggy append in reportPoolStats (#3122)

wzy9607 · ndyakov · web-flow · commit 4767d9dfaf5b · 2025-08-05T15:15:34.000+03:00
The current append twice to `conf.attrs` approach in `reportPoolStats` may result in unexpected idleAttrs, due to `append` [can mutate](golang/go#29115 (comment)) the underlying array of the original slice, as demonstrated at <https://go.dev/play/p/jwRMofH91eQ?v=goprev>. Also, I replaced `metric.WithAttributes` in `reportPoolStats` with `metric.WithAttributeSet`, since `WithAttributes` is just `WithAttributeSet` with some extra works that are not needed here, see <https://pkg.go.dev/go.opentelemetry.io/otel/metric@v1.22.0#WithAttributes>. Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>
diff --git a/extra/redisotel/metrics.go b/extra/redisotel/metrics.go
@@ -113,10 +113,15 @@ func registerClient(rdb *redis.Client, conf *config, state *metricsState) error
 	return nil
 }
 
+func poolStatsAttrs(conf *config) (poolAttrs, idleAttrs, usedAttrs attribute.Set) {
+	poolAttrs = attribute.NewSet(conf.attrs...)
+	idleAttrs = attribute.NewSet(append(poolAttrs.ToSlice(), attribute.String("state", "idle"))...)
+	usedAttrs = attribute.NewSet(append(poolAttrs.ToSlice(), attribute.String("state", "used"))...)
+	return
+}
+
 func reportPoolStats(rdb *redis.Client, conf *config) (metric.Registration, error) {
-	labels := conf.attrs
-	idleAttrs := append(labels, attribute.String("state", "idle"))
-	usedAttrs := append(labels, attribute.String("state", "used"))
+	poolAttrs, idleAttrs, usedAttrs := poolStatsAttrs(conf)
 
 	idleMax, err := conf.meter.Int64ObservableUpDownCounter(
 		"db.client.connections.idle.max",
@@ -179,16 +184,16 @@ func reportPoolStats(rdb *redis.Client, conf *config) (metric.Registration, erro
 		func(ctx context.Context, o metric.Observer) error {
 			stats := rdb.PoolStats()
 
-			o.ObserveInt64(idleMax, int64(redisConf.MaxIdleConns), metric.WithAttributes(labels...))
-			o.ObserveInt64(idleMin, int64(redisConf.MinIdleConns), metric.WithAttributes(labels...))
-			o.ObserveInt64(connsMax, int64(redisConf.PoolSize), metric.WithAttributes(labels...))
+			o.ObserveInt64(idleMax, int64(redisConf.MaxIdleConns), metric.WithAttributeSet(poolAttrs))
+			o.ObserveInt64(idleMin, int64(redisConf.MinIdleConns), metric.WithAttributeSet(poolAttrs))
+			o.ObserveInt64(connsMax, int64(redisConf.PoolSize), metric.WithAttributeSet(poolAttrs))
 
-			o.ObserveInt64(usage, int64(stats.IdleConns), metric.WithAttributes(idleAttrs...))
-			o.ObserveInt64(usage, int64(stats.TotalConns-stats.IdleConns), metric.WithAttributes(usedAttrs...))
+			o.ObserveInt64(usage, int64(stats.IdleConns), metric.WithAttributeSet(idleAttrs))
+			o.ObserveInt64(usage, int64(stats.TotalConns-stats.IdleConns), metric.WithAttributeSet(usedAttrs))
 
-			o.ObserveInt64(timeouts, int64(stats.Timeouts), metric.WithAttributes(labels...))
-			o.ObserveInt64(hits, int64(stats.Hits), metric.WithAttributes(labels...))
-			o.ObserveInt64(misses, int64(stats.Misses), metric.WithAttributes(labels...))
+			o.ObserveInt64(timeouts, int64(stats.Timeouts), metric.WithAttributeSet(poolAttrs))
+			o.ObserveInt64(hits, int64(stats.Hits), metric.WithAttributeSet(poolAttrs))
+			o.ObserveInt64(misses, int64(stats.Misses), metric.WithAttributeSet(poolAttrs))
 			return nil
 		},
 		idleMax,
diff --git a/extra/redisotel/metrics_test.go b/extra/redisotel/metrics_test.go
@@ -0,0 +1,54 @@
+package redisotel
+
+import (
+	"reflect"
+	"testing"
+
+	"go.opentelemetry.io/otel/attribute"
+)
+
+func Test_poolStatsAttrs(t *testing.T) {
+	t.Parallel()
+	type args struct {
+		conf *config
+	}
+	tests := []struct {
+		name          string
+		args          args
+		wantPoolAttrs attribute.Set
+		wantIdleAttrs attribute.Set
+		wantUsedAttrs attribute.Set
+	}{
+		{
+			name: "#3122",
+			args: func() args {
+				conf := &config{
+					attrs: make([]attribute.KeyValue, 0, 4),
+				}
+				conf.attrs = append(conf.attrs, attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"))
+				conf.attrs = append(conf.attrs, attribute.String("pool.name", "pool1"))
+				return args{conf: conf}
+			}(),
+			wantPoolAttrs: attribute.NewSet(attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"),
+				attribute.String("pool.name", "pool1")),
+			wantIdleAttrs: attribute.NewSet(attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"),
+				attribute.String("pool.name", "pool1"), attribute.String("state", "idle")),
+			wantUsedAttrs: attribute.NewSet(attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"),
+				attribute.String("pool.name", "pool1"), attribute.String("state", "used")),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotPoolAttrs, gotIdleAttrs, gotUsedAttrs := poolStatsAttrs(tt.args.conf)
+			if !reflect.DeepEqual(gotPoolAttrs, tt.wantPoolAttrs) {
+				t.Errorf("poolStatsAttrs() gotPoolAttrs = %v, want %v", gotPoolAttrs, tt.wantPoolAttrs)
+			}
+			if !reflect.DeepEqual(gotIdleAttrs, tt.wantIdleAttrs) {
+				t.Errorf("poolStatsAttrs() gotIdleAttrs = %v, want %v", gotIdleAttrs, tt.wantIdleAttrs)
+			}
+			if !reflect.DeepEqual(gotUsedAttrs, tt.wantUsedAttrs) {
+				t.Errorf("poolStatsAttrs() gotUsedAttrs = %v, want %v", gotUsedAttrs, tt.wantUsedAttrs)
+			}
+		})
+	}
+}
