Skip to content
This repository has been archived by the owner on Aug 12, 2024. It is now read-only.

Rejig dogstatsd config #108

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

noqcks
Copy link
Contributor

@noqcks noqcks commented Mar 15, 2021

Currently, kvexpress says that dogstatsd config is available through --dogstatsd_address but this is not true, since godspeed.NewDefault() uses the default statsd host/post.

This PR parses DogStatsdAddress into Host and Port for use w/ godspeed.New() to setup statsd host and port.

I chose to leave DogStatsdAddress as an entrypoint rather than creating new cli options for host and post so as to be backwards compatible.

Copy link

@brandon-dd brandon-dd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's a commands/config_test.go file as well, maybe adding a lil something there to (for example) make sure the parsing works as you expect would be good?

Copy link

@bonnefoa bonnefoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: you could get rid of the DogStatsdAddress variable and directly check for DogStatsdHost instead
Otherwise, looks good to go

@noqcks noqcks merged commit 36dea07 into DataDog:master Mar 16, 2021
@noqcks noqcks deleted the benji.visser/update-dogstatsd-init branch March 16, 2021 17:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants