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

[dsd] Support container ID field from dogstatsd 1.2 #10659

Merged
merged 10 commits into from
Feb 4, 2022

Conversation

ahmed-mez
Copy link
Contributor

@ahmed-mez ahmed-mez commented Jan 26, 2022

What does this PR do?

Extend origin detection on by supporting a new field in the dsd protocol, the new field c:<id> holds the container ID discovered by the client.

Motivation

  • More granular tags (container tags instead of pod tags)
  • Not limited to Kubernetes, and can be a good alternative in environments where origin detection on UDS is not an option (e.g Fargate)

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Deploy the agent with DD_DOGSTATSD_ORIGIN_DETECTION_CLIENT=true

echo -n "testing.dsd.container:1|g|#foo:bar,dd.internal.card:high|c:<container id>" | nc -u -w1 <Agent IP> 8125

You should be able to see container tags attached to the testing.dsd.container metric on the app.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • The appropriate team/.. label has been applied, if known.
  • Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
  • A release note has been added or the changelog/no-changelog label has been applied.
  • Changed code has automated tests for its functionality.
  • Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
  • If applicable, docs team has been notified or an issue has been opened on the documentation repo.
  • If applicable, the need-change/operator and need-change/helm labels have been applied.
  • If applicable, the config template has been updated.

@ahmed-mez ahmed-mez added this to the 7.35.0 milestone Jan 26, 2022
@ahmed-mez ahmed-mez changed the title [dsd] Support container tagging on UDP [dsd] Support container-level tagging on UDP Jan 26, 2022
@ahmed-mez ahmed-mez marked this pull request as ready for review January 26, 2022 14:04
@ahmed-mez ahmed-mez requested review from a team as code owners January 26, 2022 14:04
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/dogstatsd/enrich.go Outdated Show resolved Hide resolved
pkg/dogstatsd/parse.go Outdated Show resolved Hide resolved
Copy link
Contributor

@maycmlee maycmlee left a comment

Choose a reason for hiding this comment

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

Small edit.

releasenotes/notes/dsd-container-b2039d8e0e510fbf.yaml Outdated Show resolved Hide resolved
Copy link
Member

@hush-hush hush-hush left a comment

Choose a reason for hiding this comment

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

Looking good overall. We would need to update our official documentation here to add a new section for version 1.2.

releasenotes/notes/dsd-container-b2039d8e0e510fbf.yaml Outdated Show resolved Hide resolved
pkg/config/config_template.yaml Outdated Show resolved Hide resolved
pkg/config/config_template.yaml Outdated Show resolved Hide resolved
Copy link
Member

@hush-hush hush-hush left a comment

Choose a reason for hiding this comment

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

Added a nit-pick on the release note. Looks good otherwise

releasenotes/notes/dsd-container-b2039d8e0e510fbf.yaml Outdated Show resolved Hide resolved
@hush-hush hush-hush changed the title [dsd] Support container-level tagging on UDP [dsd] Support container ID field from dogstatsd 1.2 Feb 4, 2022
Copy link
Contributor

@vickenty vickenty left a comment

Choose a reason for hiding this comment

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

Would it make sense to c

// We use the UDS socket origin if no origin ID was specify in the tags
// or 'dogstatsd_entity_id_precedence' is set to False (default false).
if entityIDValue == "" || !entityIDPrecedenceEnabled {
if originFromTag == "" || !entityIDPrecedenceEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we skip originFromUDS if we are going to use originFromMsg? It would be nice to avoid tagging metrics with two container tags if a proxy is using UDS to send metrics on behalf of another container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UDS origin detection is opt-in and not enabled by default, I'd rather let the user have control.

Copy link
Contributor

Choose a reason for hiding this comment

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

This feature would probably interest users that use origin detection today, so they may be inclined to have both options enabled: use uds to tag traffic on older clients, while using benefits of client-side origin detection with newer clients. Unless double-tagging is a use case we explicitly want to support, I think it would make more sense to assign only one origin per metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep it simple as is currently and not make any assumptions.
We agreed that extending the protocol is always an option in the future, so if we want to turn on and off UDS origin detection on-demand it would be best to let the client do it via a new field.

pkg/config/config_template.yaml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants