-
Notifications
You must be signed in to change notification settings - Fork 813
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
[AD] Add docker labels as tags in AD #3564
Conversation
8c83a8a
to
ba514ca
Compare
ba514ca
to
0ce92d4
Compare
0ce92d4
to
21c9092
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic LGTM, two nitpicks
datadog.conf.example
Outdated
# Docker labels as tags | ||
# We can extract docker labels and add them as tags to all metrics reported by service discovery. | ||
# All you have to do is supply a comma-separated list of label names to extract from containers when found. | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a reference to docker_daemon's collect_labels_as_tags
for docker/kube metrics
for test in no_label_test_data: | ||
self.assertEqual(test[1], DockerUtil().extract_container_tags(test[0], [])) | ||
|
||
for test in labeled_test_data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
utils/dockerutil.py
Outdated
"""Returns a list of tags based on a container and a label name list""" | ||
tags = [] | ||
labels = ctr.get('Config', {}).get('Labels', {}) | ||
for lbl_name, lbl_val in labels.iteritems(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be extra-safe, let's add a if not labels: return tags
, I had the issue of a Config/Env being present but None instead of missing. See #3528
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌮
unrelated CI failure, merging. |
* [AD] Add docker labels as tags * Add tests * address review comments
Note: Please remember to review the Datadog Contribution Guidelines
if you have not yet done so.
What does this PR do?
Add docker labels as tags for auto discovery.
Motivation
User request.
Testing Guidelines
Added unit tests for it.
Additional Notes
Anything else we should know when reviewing?