-
Notifications
You must be signed in to change notification settings - Fork 814
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
[Docker check] V2 #1908
[Docker check] V2 #1908
Conversation
6921325
to
7efba53
Compare
] | ||
|
||
DEFAULT_CONTAINER_TAGS = [ | ||
"docker_image", |
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.
Do we want to have docker_image
as a default tag?
We wanted to replace it with the combo image_name
/ image_tag
(same cardinality but much more flexible), and kept docker_image
just for compatibility (as said in the config file).
Did we change our mind?
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 think it makes sense to keep it so we don't break everything (dashboards monitors) and as you said it's the same cardinality as image_name / image_tag so it's not creating more contexts, i think it's a good compromise.
|
||
# Set filtering settings | ||
if not instance.get("exclude"): | ||
self._filtering_enabled = False |
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.
Since it seems include
is meaningless without exclude
, should we show a warning for the case instance.get('include') and not instance.get('exclude')
? I can see that being a common mistake
# | ||
url: "unix://var/run/docker.sock" | ||
|
||
# Cgroup root (HIDDEN OPTION?) |
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.
We should remove it since we auto-detect the path. If a new platform with a different mount point arises we'll add its path in dockerutil.py so that it's supported out of the box anyway.
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.
The point about this option was to allow users on a new platform/with a new cgroup pattern to configure the Agent, without waiting for a new Agent release.
That's pretty minor, maybe not worth spending the time to add this feature,
This is a new version of the docker check, it's called docker_daemon (name can be changed). The previous check "docker" is now deprecated and won't receive further support. In terms of features, this adds: - Support for TLS connections to the daemon - New metrics: - Network metrics - Memory limits - Container size (rootfs) - Image size - Support for labels (convert them into tags). Off by default, uses a list of labels that should be converted. - Support for ECS tags: task name and task version Backward incompatible changes: - docker.disk.size metric is renamed to docker.container.size_rw - Old optional metrics: https://github.com/DataDog/dd-agent/blob/5.4.x/checks.d/docker.py#L29-L38 Are not collected anymore - Old old tags are not supported anymore (e.g. `name` instead of container_name) fix: #1299 #1648 #1739 #1742 #1896
👍 🎊 |
@@ -67,6 +67,6 @@ instances: | |||
# collect_all_metrics: false | |||
|
|||
# Collect images stats | |||
# Number of available active images and intermediate images as gauges. Default: true. | |||
# Number of available active images and intermediate images as gauges. Default: false. |
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.
It looks like the default in the code is still True
.
See comments on DataDog#1908
See comments on DataDog#1908
This is a new version of the docker check, it's called docker_daemon (name can be changed).
The previous check "docker" is now deprecated and won't receive further support.
In terms of features, this adds:
Backward incompatible changes:
fix #1299 #1648 #1739 #1742