Skip to content
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

Add support for DD_TAGS environment variable #703

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nrmitchi
Copy link

@nrmitchi nrmitchi commented Dec 1, 2021

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.

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.
@nrmitchi nrmitchi requested review from a team as code owners December 1, 2021 16:47
@jirikuncar
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@NouemanKHAL
Copy link
Member

Can you update these docstrings accordingly ?

:envvar DATADOG_TAGS: Tags to attach to every metric reported by ThreadStats client

:envvar DATADOG_TAGS: Tags to attach to every metric reported by dogstatsd client.

Thanks!

@therve
Copy link
Contributor

therve commented Dec 8, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@therve therve added the changelog/Added Added features results into a minor version bump label Dec 8, 2021
@hush-hush
Copy link
Member

hush-hush commented Dec 8, 2021

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.

@hush-hush hush-hush added the do-not-merge/HOLD Do not merge this PR label Dec 8, 2021
@github-actions
Copy link

github-actions bot commented Feb 3, 2022

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.

@github-actions github-actions bot added the stale Stale - Bot reminder label Feb 3, 2022
Copy link

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

(clearing agent-core review request)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump do-not-merge/HOLD Do not merge this PR stale Stale - Bot reminder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow use of DD_TAGS environment variable for constant tags
6 participants