search

DataDog / dd-trace-go

Datadog Go Library including APM tracing, profiling, and security monitoring.
https://docs.datadoghq.com/tracing/
Other
666 stars 437 forks source link

[BUG][1.48.0] contrib/go-redis/redis.v8 BeforeProcess Panics With Single Element Command #1855

Open vksrinivasan1 opened 1 year ago

vksrinivasan1 commented 1 year ago

Version of dd-trace-go v1.48.0

Describe what happened: We have an existing app that sends the PING command to redis (without any additional arguments). This worked in v1.47.0, but it results in a panic today

    /Users/vyas.srinivasan/.gimme/versions/go1.19.3.darwin.arm64/src/runtime/panic.go:884 +0x204
gopkg.in/DataDog/dd-trace-go.v1/contrib/go-redis/redis%2ev8.(*datadogHook).BeforeProcess(0x14000010a88, {0x104cea868, 0x140000ace40}, {0x104cf1600?, 0x140000acea0?})
    /Users/vyas.srinivasan/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.48.0/contrib/go-redis/redis.v8/redis.go:109 +0x7a8
github.com/go-redis/redis/v8.hooks.process({{0x1400037d9b0?, 0x10?, 0x105317b80?}}, {0x104cea868?, 0x140000ace40?}, {0x104cf1600, 0x140000acea0}, 0x14000187e48)
    /Users/vyas.srinivasan/go/pkg/mod/github.com/go-redis/redis/v8@v8.11.4/redis.go:63 +0x110
github.com/go-redis/redis/v8.(*Client).Process(...)
    /Users/vyas.srinivasan/go/pkg/mod/github.com/go-redis/redis/v8@v8.11.4/redis.go:596
github.com/go-redis/redis/v8.(*Client).Do(0x140003baf80, {0x104cea868?, 0x140000ace40}, {0x1400037d9f0, 0x1, 0x1})
...

It's panicking here while trying to extract the command name by looking for the first space in the raw command -- in single element commands like PING without arguments, there is no space so this fails. v.1.47.0 uses strings.Split() which would return a slice even if a space wasn't present. It looks like this was changed to indexing up to the first space in v1.48.0 for efficiency reasons.

Describe what you expected: I would not expect a panic to occur/I would expect the command to go through.

Steps to reproduce the issue:

func TestPanic(t *testing.T) {
    s := miniredis.RunT(t)
    opts := &redis.Options{Addr: s.Addr()}
    client := NewClient(opts, WithServiceName("test"))
    client.Do(context.TODO(), "PING").Result()
}

Additional environment details (Version of Go, Operating System, etc.):

vksrinivasan1 commented 1 year ago

Just looking at the code, I think it's reasonable to move strings.IndexByte(raw, ' ') before the opts creation and take the entire value of raw if there was no space found. I'm happy to make the change, lemme know

ajgajg1134 commented 1 year ago

Just looking at the code, I think it's reasonable to move strings.IndexByte(raw, ' ') before the opts creation and take the entire value of raw if there was no space found. I'm happy to make the change, lemme know

Yeah this change sounds good to me, if you have a minute to contribute it that's great, if not let me know and I can standup a PR from the details you've shared here