-
Notifications
You must be signed in to change notification settings - Fork 132
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
Support configuring the client from standard environment variables #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic LGTM, left a few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one super small note. You may merge at will.
Looks 👌and thank you for the tests!
statsd/statsd.go
Outdated
return client, nil | ||
} | ||
|
||
// NewBuffered returns a Client that buffers its output and sends it in chunks. | ||
// Buflen is the length of the buffer in number of commands. | ||
// When addr is empty, the client will use the DD_AGENT_HOST and (optionally) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like these comments belong in the UDP client part where they actually apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal here is for it to show in godoc
, in addition to the readme. Unfortunately, newUDPWriter
is private API, so it won't show up in godoc. Would the readme be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a legit reason to have it here. For clarity, can you be a little more explicit regarding what will happen? ie:
// When addr is empty, the client will default to a UDP client and use the DD_AGENT_HOST
// and (optionally) the DD_DOGSTATSD_PORT environment variables to build the target address.
Other than that, this is good to go!
### Supported environment variables | ||
|
||
- The client can use the `DD_AGENT_HOST` and (optionally) the `DD_DOGSTATSD_PORT` environment variables to build the target address if the `addr` parameter is empty. | ||
- If the `DD_ENTITY_ID` enviroment variable is found, its value will be injected as a global `dd.internal.entity_id` tag. This tag will be used by the Datadog Agent to insert container tags to the metrics. You should only `append` to the `c.Tags` slice to avoid overwriting this global tag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately client.Tags
is exported, so this is kind of easy to mess up by user's code - no real great solution to that problem without making breaking changes 😞 - thanks for adding this note here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, if a major version is in the works, we might want to move to a setter method instead of an exported field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will probably be working on a major in the mid term, but probably not yet, so let's merge this as-is. Keep it in mind for when a mejor does come around, though. I'll try to remember to bring this up when the time comes. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📦
* DD_AGENT_ADDR to DD_AGENT_HOST * DD_AGENT_PORT to DD_DOGSTATSD_PORT Following the same standard upstream DataDog/datadog-go#78
In order to ease dogstatsd usage in containerized environments, we are implementing standard environment variables common to all client libraries (both dogstatsd and trace). This PR is the first implementation for dsd.
Auto configuration of the dsd server address via envvars
Introduce support of the
DD_AGENT_HOST
andDD_DOGSTATSD_PORT
environment variables, that can be set via the orchestrator on the applicative container. Logic is as following:addr
param) always take over. If it's empty, run the autoconfiguration logicDD_AGENT_HOST
is required, configuration fails if it's not setDD_DOGSTATSD_PORT
is optionnal, library defaults to the default8125
port if not setContainer tagging via the entity ID
Support the
DD_ENTITY_ID
envvar, injected by the orchestrator in the applicative container. If found, it is added as a constant_dd.entity_id
tag on all metrics. Dogstatsd will replace this tag by container tags from the tagger on intake.