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: origin detection with container ID field (dogstatsd 1.2) #188

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

ahmed-mez
Copy link
Contributor

What does this PR do?

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

Description of the Change

  • 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 and the originDetectionEnabled param is not explicitly disabled, we try to get the app container ID by parsing /proc/self/cgroup. In case of success, the container field in will be set.

Verification Process

Wrote a small app importing the lib and sending metrics to agent 7.35 with origin detection client enabled - the metrics were tagged with container tags.

Additional Notes

@ahmed-mez ahmed-mez force-pushed the ahmed/dsd-1.2 branch 4 times, most recently from 03deab7 to 7c23cbe Compare March 11, 2022 12:05
.gitignore Outdated Show resolved Hide resolved
src/main/java/com/timgroup/statsd/ContainerID.java Outdated Show resolved Hide resolved
src/main/java/com/timgroup/statsd/ContainerID.java Outdated Show resolved Hide resolved
src/main/java/com/timgroup/statsd/ContainerID.java Outdated Show resolved Hide resolved
src/main/java/com/timgroup/statsd/ContainerID.java Outdated Show resolved Hide resolved
src/main/java/com/timgroup/statsd/ContainerID.java Outdated Show resolved Hide resolved
@ahmed-mez
Copy link
Contributor Author

Thank you for the review @vickenty ! I'll try to address the comments soon 👍

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.

LGTM, but if possible let's tighten visibility on the new items.

src/main/java/com/timgroup/statsd/CgroupReader.java Outdated Show resolved Hide resolved
src/main/java/com/timgroup/statsd/StatsDProcessor.java Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants