Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wzy9607
Copy link

@wzy9607 wzy9607 commented Sep 15, 2024

The current append twice to conf.attrs approach in reportPoolStats may result in unexpected idleAttrs, due to append can mutate the underlying array of the original slice, as demonstrated at https://go.dev/play/p/jwRMofH91eQ?v=goprev and below.

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.

demonstration of the bug

package main

import (
	"fmt"

	"go.opentelemetry.io/otel/attribute"
)

func main() {
	a := make([]attribute.KeyValue, 0, 4)
	a = append(a, attribute.String("a", "a"), attribute.String("b", "b"), attribute.String("c", "c"))
	idle := append(a, attribute.String("state", "idle"))
	used := append(a, attribute.String("state", "used"))
	fmt.Printf("a: %v\n", a)
	fmt.Printf("idle: %v\n", idle)
	fmt.Printf("used: %v\n", used)
}

outputs

a: [{a {4 0 a <nil>}} {b {4 0 b <nil>}} {c {4 0 c <nil>}}]
idle: [{a {4 0 a <nil>}} {b {4 0 b <nil>}} {c {4 0 c <nil>}} {state {4 0 used <nil>}}]
used: [{a {4 0 a <nil>}} {b {4 0 b <nil>}} {c {4 0 c <nil>}} {state {4 0 used <nil>}}]

while the intended result in reportPoolStats usecase is
idle: [{a {4 0 a <nil>}} {b {4 0 b <nil>}} {c {4 0 c <nil>}} {state {4 0 idle <nil>}}].

@wzy9607 wzy9607 changed the title redisotel: fix buggy append in reportPoolStats and use WithAttributeSet redisotel: fix buggy append in reportPoolStats Sep 15, 2024
@wzy9607 wzy9607 changed the title redisotel: fix buggy append in reportPoolStats fix(redisotel): fix buggy append in reportPoolStats Sep 15, 2024
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>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant