DataDog / datadog-go

go dogstatsd client library for datadog
MIT License
350 stars 134 forks source link

Race condition when setting statsd.Client.Namespace as per doc example #162

Closed AndersonQ closed 2 years ago

AndersonQ commented 4 years ago

I run my application with -race and found out a race condition when setting the namespace as per the docs:

// Create the client
c, err := statsd.New("127.0.0.1:8125")
...
// Prefix every metric with the app name
c.Namespace = "flubber."

I wrote a wee snippet to reproduce the problem:

package main

import (
    "log"
    "time"

    "github.com/DataDog/datadog-go/statsd"
)

func main() {
    client, err := statsd.New(
        "localhost:8888")
    if err != nil {
        log.Println("could not get datadog client")
        panic("could not get datadog client:" + err.Error())
    }

    client.Namespace = "another namespace"

    time.Sleep(time.Minute)
}

go run -race main.go

output

``` $ go run -race main.go ================== WARNING: DATA RACE Read at 0x00c000146008 by goroutine 19: github.com/DataDog/datadog-go/statsd.(*Client).send() /Users/aqueiroz/devel/go/pkg/mod/github.com/!data!dog/datadog-go@v3.7.1+incompatible/statsd/statsd.go:467 +0x99 github.com/DataDog/datadog-go/statsd.(*Client).telemetry() /Users/aqueiroz/devel/go/pkg/mod/github.com/!data!dog/datadog-go@v3.7.1+incompatible/statsd/statsd.go:402 +0x252 github.com/DataDog/datadog-go/statsd.newWithWriter.func2() /Users/aqueiroz/devel/go/pkg/mod/github.com/!data!dog/datadog-go@v3.7.1+incompatible/statsd/statsd.go:357 +0xa7 Previous write at 0x00c000146008 by main goroutine: main.main() /Users/aqueiroz/devel/github.com/blacklane/ride-matcher/ddrace/main.go:20 +0x85 Goroutine 19 (running) created at: github.com/DataDog/datadog-go/statsd.newWithWriter() /Users/aqueiroz/devel/go/pkg/mod/github.com/!data!dog/datadog-go@v3.7.1+incompatible/statsd/statsd.go:354 +0xfe6 github.com/DataDog/datadog-go/statsd.New() /Users/aqueiroz/devel/go/pkg/mod/github.com/!data!dog/datadog-go@v3.7.1+incompatible/statsd/statsd.go:272 +0x2b4 main.main() /Users/aqueiroz/devel/github.com/blacklane/ride-matcher/ddrace/main.go:11 +0x58 ================== Found 1 data race(s) exit status 66 ```

Using statsd.WithNamespace instead of setting it latter on does not cause the race condition:

    client, err := statsd.New(
        "localhost:8888",
        statsd.WithNamespace("myApp."),
    )

Also statsd.Client states it's goroutine safe.

I'd suggest to do not export the Namespace field.

However it'd be a breaking change, so maybe deprecate the field, adjust the dos and put a warning on the statsd.Client doc?

hush-hush commented 3 years ago

Hi @AndersonQ,

Right now, we updated the doc. We're currently planning a new major for the client where a lot of useless public field would become private (among other things) as you suggested.

There is no ETA right now for this but we're hopping for Q2.

hush-hush commented 2 years ago

HI @AndersonQ,

The new major version was delayed more than we expected but it's finally released. The client.Namespace has been removed in favor of statsd.WithNamespace.

See the CHANGELOG for all the breaking changed in the new v5 version.

I'm closing this issue but feel free to reopen it if needed.