Skip to content

Commit b9ac847

Browse files
committed
fix(redisotel): fix buggy append in reportPoolStats
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/[email protected]#WithAttributes>.
1 parent 8fadbef commit b9ac847

File tree

2 files changed

+68
-9
lines changed

2 files changed

+68
-9
lines changed

extra/redisotel/metrics.go

+14-9
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,15 @@ func InstrumentMetrics(rdb redis.UniversalClient, opts ...MetricsOption) error {
8282
}
8383
}
8484

85+
func poolStatsAttrs(conf *config) (poolAttrs, idleAttrs, usedAttrs attribute.Set) {
86+
poolAttrs = attribute.NewSet(conf.attrs...)
87+
idleAttrs = attribute.NewSet(append(poolAttrs.ToSlice(), attribute.String("state", "idle"))...)
88+
usedAttrs = attribute.NewSet(append(poolAttrs.ToSlice(), attribute.String("state", "used"))...)
89+
return
90+
}
91+
8592
func reportPoolStats(rdb *redis.Client, conf *config) error {
86-
labels := conf.attrs
87-
idleAttrs := append(labels, attribute.String("state", "idle"))
88-
usedAttrs := append(labels, attribute.String("state", "used"))
93+
poolAttrs, idleAttrs, usedAttrs := poolStatsAttrs(conf)
8994

9095
idleMax, err := conf.meter.Int64ObservableUpDownCounter(
9196
"db.client.connections.idle.max",
@@ -132,14 +137,14 @@ func reportPoolStats(rdb *redis.Client, conf *config) error {
132137
func(ctx context.Context, o metric.Observer) error {
133138
stats := rdb.PoolStats()
134139

135-
o.ObserveInt64(idleMax, int64(redisConf.MaxIdleConns), metric.WithAttributes(labels...))
136-
o.ObserveInt64(idleMin, int64(redisConf.MinIdleConns), metric.WithAttributes(labels...))
137-
o.ObserveInt64(connsMax, int64(redisConf.PoolSize), metric.WithAttributes(labels...))
140+
o.ObserveInt64(idleMax, int64(redisConf.MaxIdleConns), metric.WithAttributeSet(poolAttrs))
141+
o.ObserveInt64(idleMin, int64(redisConf.MinIdleConns), metric.WithAttributeSet(poolAttrs))
142+
o.ObserveInt64(connsMax, int64(redisConf.PoolSize), metric.WithAttributeSet(poolAttrs))
138143

139-
o.ObserveInt64(usage, int64(stats.IdleConns), metric.WithAttributes(idleAttrs...))
140-
o.ObserveInt64(usage, int64(stats.TotalConns-stats.IdleConns), metric.WithAttributes(usedAttrs...))
144+
o.ObserveInt64(usage, int64(stats.IdleConns), metric.WithAttributeSet(idleAttrs))
145+
o.ObserveInt64(usage, int64(stats.TotalConns-stats.IdleConns), metric.WithAttributeSet(usedAttrs))
141146

142-
o.ObserveInt64(timeouts, int64(stats.Timeouts), metric.WithAttributes(labels...))
147+
o.ObserveInt64(timeouts, int64(stats.Timeouts), metric.WithAttributeSet(poolAttrs))
143148
return nil
144149
},
145150
idleMax,

extra/redisotel/metrics_test.go

+54
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package redisotel
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
7+
"go.opentelemetry.io/otel/attribute"
8+
)
9+
10+
func Test_poolStatsAttrs(t *testing.T) {
11+
t.Parallel()
12+
type args struct {
13+
conf *config
14+
}
15+
tests := []struct {
16+
name string
17+
args args
18+
wantPoolAttrs attribute.Set
19+
wantIdleAttrs attribute.Set
20+
wantUsedAttrs attribute.Set
21+
}{
22+
{
23+
name: "#3122",
24+
args: func() args {
25+
conf := &config{
26+
attrs: make([]attribute.KeyValue, 0, 4),
27+
}
28+
conf.attrs = append(conf.attrs, attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"))
29+
conf.attrs = append(conf.attrs, attribute.String("pool.name", "pool1"))
30+
return args{conf: conf}
31+
}(),
32+
wantPoolAttrs: attribute.NewSet(attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"),
33+
attribute.String("pool.name", "pool1")),
34+
wantIdleAttrs: attribute.NewSet(attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"),
35+
attribute.String("pool.name", "pool1"), attribute.String("state", "idle")),
36+
wantUsedAttrs: attribute.NewSet(attribute.String("foo1", "bar1"), attribute.String("foo2", "bar2"),
37+
attribute.String("pool.name", "pool1"), attribute.String("state", "used")),
38+
},
39+
}
40+
for _, tt := range tests {
41+
t.Run(tt.name, func(t *testing.T) {
42+
gotPoolAttrs, gotIdleAttrs, gotUsedAttrs := poolStatsAttrs(tt.args.conf)
43+
if !reflect.DeepEqual(gotPoolAttrs, tt.wantPoolAttrs) {
44+
t.Errorf("poolStatsAttrs() gotPoolAttrs = %v, want %v", gotPoolAttrs, tt.wantPoolAttrs)
45+
}
46+
if !reflect.DeepEqual(gotIdleAttrs, tt.wantIdleAttrs) {
47+
t.Errorf("poolStatsAttrs() gotIdleAttrs = %v, want %v", gotIdleAttrs, tt.wantIdleAttrs)
48+
}
49+
if !reflect.DeepEqual(gotUsedAttrs, tt.wantUsedAttrs) {
50+
t.Errorf("poolStatsAttrs() gotUsedAttrs = %v, want %v", gotUsedAttrs, tt.wantUsedAttrs)
51+
}
52+
})
53+
}
54+
}

0 commit comments

Comments
 (0)