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

[Docker check] V2 #1908

Merged
merged 3 commits into from
Sep 15, 2015
Merged

[Docker check] V2 #1908

merged 3 commits into from
Sep 15, 2015

Conversation

remh
Copy link
Contributor

@remh remh commented Sep 11, 2015

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

@remh remh force-pushed the remh/dockerv2 branch 2 times, most recently from 6921325 to 7efba53 Compare September 11, 2015 19:34
]

DEFAULT_CONTAINER_TAGS = [
"docker_image",
Copy link
Contributor

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?

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 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
Copy link
Contributor

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?)
Copy link
Member

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.

Copy link
Contributor

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
@remh remh added this to the 5.5.0 milestone Sep 15, 2015
@talwai
Copy link
Contributor

talwai commented Sep 15, 2015

👍 🎊

remh added a commit that referenced this pull request Sep 15, 2015
@remh remh merged commit e402e01 into master Sep 15, 2015
@@ -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.
Copy link
Contributor

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.

remh added a commit that referenced this pull request Sep 16, 2015
urosgruber pushed a commit to urosgruber/dd-agent that referenced this pull request Dec 23, 2015
urosgruber pushed a commit to urosgruber/dd-agent that referenced this pull request Dec 23, 2015
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.

[docker] TLS support in docker connection / docker-py
4 participants