Add support for DD_TAGS environment variable

nrmitchi commented 2 years ago

Falls back to reading global tags from the DD_TAGS env var if DATADOG_TAGS is not present.

DD_TAGS used for other datadog products, such as APM.

What does this PR do?

Adds support for using constant tags from the DD_TAGS environment variable, rather than the DATADOG_TAGS environment variable.

This PR closes #702

Description of the Change

If the DATADOG_TAGS environment variable is not present, fall back to the DD_TAGS environment variable for constant tags. This allows this library to match the behaviour of other datadog services (as well as the documentation).

By using DD_TAGS only if DATADOG_TAGS is not present, this change should be backwards compatible with existing usage.

Alternate Designs

An alternative was a straight replacement of the environment variable, however a fallback was chosen to maintain backwards compatibility and avoid dropping of existing tags.

Possible Drawbacks

It is possible that if a user is currently not using the DATADOG_TAGS environment variable for constant tags, but is using the DD_TAGS environment variable for a different datadog service (such as APM), those tags will be added to custom metrics. This could lead to an unexpected increase in metric cardinality.

Additional Notes

Currently duplicating all constant tags into both DD_TAGS and DATADOG_TAGS environment variables is prone to error.

Release Notes

If you are currently utilizing DD_TAGS environment variable for other datadog functionality, and not utilizing DATADOG_TAGS for this library, your existing DD_TAGS will be added to existing metrics.

Review checklist (to be filled by reviewers)

[ ] Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
[ ] PR title must be written as a CHANGELOG entry (see why)
[ ] Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
[ ] PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
[ ] PR should not have do-not-merge/ label attached.
[ ] If Applicable, issue must have kind/ and severity/ labels attached at least.

jirikuncar commented 2 years ago

/azp run

azure-pipelines[bot] commented 2 years ago

Azure Pipelines successfully started running 2 pipeline(s).

NouemanKHAL commented 2 years ago

Can you update these docstrings accordingly ? https://github.com/DataDog/datadogpy/blob/33f727cc51d637860a2a46677bf7151c8feb10fa/datadog/threadstats/base.py#L55 https://github.com/DataDog/datadogpy/blob/6bf457859e23da8847291536e54b00e3b7023d5b/datadog/dogstatsd/base.py#L118

Thanks!

therve commented 2 years ago

/azp run

azure-pipelines[bot] commented 2 years ago

Azure Pipelines successfully started running 2 pipeline(s).

hush-hush commented 2 years ago

Hi @nrmitchi,

Thanks for opening this PR. Adding support for DD_TAGS across all our DogStatsD clients is something we're looking into.

The main issue being that when DD_TAGS is set for the whole host and the agent/client are running locally it will be picked by both, so we have to be careful. This is especially important when using containers, VMs or k8s deployments. The last thing we want is for the clients or Agent to assign the wrong tags to a metric by default.

We're also looking into aligning the behavior between all clients around this, which is a bit all over the place right now.

In the meantime, clients that want to fetch tags from the env can easily do so using the library constructor. It's not the best but it's an easy workaround.

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity.

DataDog / datadogpy