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

feat: Support container ID field (dogstatsd 1.2) #250

Merged
merged 6 commits into from
Feb 4, 2022

Conversation

ahmed-mez
Copy link
Contributor

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

The PR does the following

  • Adds c:<id> field holding the container ID so the agent can attach container tags.

Brief explanation of how the new container field is handled

  • If DD_ENTITY_ID is set, we ignore the container field
  • If DD_ENTITY_ID is not set and DD_ORIGIN_DETECTION_ENABLED is not explicitly set to false or the WithoutOriginDetection() is not invoked, we try to get the app container ID by parsing /proc/self/cgroup. In case of success, the container field in will be set to .

Motivation

  • More granular tags (container tags instead of pod tags)
  • Works for containers outside of Kubernetes (e.g ECS, Fargate)
  • The goal of not reusing dd.internal.entity_id is to avoid parsing the value and guess the prefix on the agent side (avoid costly operations in the hot path)

Notes

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.

I added a few comments. We should wait on the final decision between tags and new field in dogstatsd protocol before going further.

statsd/container_test.go Outdated Show resolved Hide resolved
statsd/container_test.go Outdated Show resolved Hide resolved
statsd/end_to_end_udp_test.go Outdated Show resolved Hide resolved
statsd/container.go Outdated Show resolved Hide resolved
statsd/container.go Outdated Show resolved Hide resolved
statsd/end_to_end_udp_test.go Outdated Show resolved Hide resolved
statsd/statsd.go Outdated Show resolved Hide resolved
statsd/end_to_end_udp_test.go Outdated Show resolved Hide resolved
statsd/statsd.go Outdated Show resolved Hide resolved
statsd/container.go Outdated Show resolved Hide resolved
statsd/options.go 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.

I think it's almost good to go.

statsd/container.go Outdated Show resolved Hide resolved
statsd/end_to_end_udp_test.go Outdated Show resolved Hide resolved
statsd/options.go Show resolved Hide resolved
statsd/options.go Outdated Show resolved Hide resolved
statsd/options.go 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.

Thanks for the PR !

@ahmed-mez
Copy link
Contributor Author

Thank you for the review @hush-hush ! I'd like to get 👀 on the Agent PR before merging this one.

PTAL when you get a chance! DataDog/datadog-agent#10659

@ahmed-mez ahmed-mez changed the title feat: container origin detection on UDP feat: Support container ID field (dogstatsd 1.2) Feb 4, 2022
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.

3 participants