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

Allow autoconf templates in docker labels #3451

Merged
merged 4 commits into from
Aug 10, 2017
Merged

Conversation

xvello
Copy link
Contributor

@xvello xvello commented Jul 26, 2017

What does this PR do?

Mirroring the kubernetes annotation autodiscovery source, lookup autoconf template in container labels. These templates are expected in the JSON format.

If k8s annotation are present, they take precedence. Else, if valid json is found in the following labels:

  • com.datadoghq.ad.check_names
  • com.datadoghq.ad.init_configs
  • com.datadoghq.ad.instances

Warning: the label names changed from the original PR, to reflect the s/ServiceDiscovery/AutoDiscovery/ transition

a template is rendered from them, bypassing the auto_conf files lookup.

If a K/V store backend is configured, this feature is disabled, like k8s annotations.

Testing Guidelines

  • Added the test_get_check_tpls_labels test method. We could go further with integration testing.
  • For manual testing, one can use the workbench recipe jmx:template_in_labels from add new template_in_labels jmx variant workbench-recipes#6 and the datadog/dev-dd-agent:jmx-xvello_template_in_labels image

@xvello xvello force-pushed the xvello/template_in_labels branch from bebc153 to 14192c1 Compare July 26, 2017 14:40
@xvello xvello added this to the 5.17 milestone Jul 27, 2017
@xvello xvello force-pushed the xvello/template_in_labels branch 3 times, most recently from 6b73eee to d3481f9 Compare July 27, 2017 15:30
@jhmartin
Copy link

jhmartin commented Aug 1, 2017

The / character in the label name will not work with Amazon ECS Task Definitions, as it enforces the label format recommendations at https://docs.docker.com/engine/userguide/labels-custom-metadata/. Attempting to add such a label in ECS results in:

Docker label key service-discovery.datadoghq.com/check_names contains invalid characters, does not match pattern ^[_\-a-zA-Z0-9.]+$

The strict pattern according to the docker page would be^[a-z0-9-.]+$, and until recently was the pattern required by AWS.

Additionally the reverse-dns style recommendation implies it should becom.datadoghq.service-discovery as the prefix, so perhaps com.datadoghq.service-discovery.check-names and so on.

@xvello xvello force-pushed the xvello/template_in_labels branch 2 times, most recently from 770b2dc to 6c19ba4 Compare August 2, 2017 12:07
@xvello
Copy link
Contributor Author

xvello commented Aug 2, 2017

Good catch, I'm renaming the labels to be valid. Here is an example label section for a basic jmx check:

    labels:
      com.datadoghq.sd.check_names: '["jmx"]'
      com.datadoghq.sd.init_configs: '[{}]'
      com.datadoghq.sd.instances: '[{"host": "%%host%%", "port": "%%port%%"}]'

The test docker image is rebuilding as I post this, could you give it another try?

@jhmartin
Copy link

jhmartin commented Aug 2, 2017

This appears to work.

My docker-compose.yml was:

version: "2"
services:
  tomcat:
    image: tomcat:8-alpine
    ports:
     - "8080:8080"
     - "8099:8099"
    environment:
     - JVM_HEAP=-Xmx512m -Xms512m
     - CATALINA_OPTS=-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=8099 -Dcom.sun.management.jmxremote.ssl=false -Dcom.sun.management.jmxremote.authenticate=false
    labels:
      com.datadoghq.sd.check_names: "[\"jmx\"]"
      com.datadoghq.sd.init_configs: "[{}]"
      com.datadoghq.sd.instances: "[{\"host\": \"%%host%%\", \"port\": \"%%port_1%%\",  \"tags\" : { \"env\" : \"jm\", \"app\" : \"appnamehere\" }}]"
  datadog:
    image: datadog/dev-dd-agent:jmx-xvello_template_in_labels
    depends_on:
      - tomcat
    environment:
     - API_KEY
     - SD_JMX_ENABLE=yes
     - SD_BACKEND=docker
     - DD_HOSTNAME=jmtest
     - LOG_LEVEL=DEBUG
    volumes:
     - /var/run/docker.sock:/var/run/docker.sock
     - /proc:/host/proc:ro
     - /sys/fs/cgroup:/host/sys/fs/cgroup:ro

I had to add escaping else compose complained about the YAML format.

Using this image and configuration I saw the JMX metrics appear in DataDog under app:appnamehere and env:jm.

A couple of things I noticed:
/etc/init.d/datadog-agent configtest isn't aware of this so shows no JMX metrics, same for /etc/init.d/datadog-agent jmx. It is not a fatal problem but does complicate troubleshooting.

@xvello xvello requested a review from hkaj August 3, 2017 08:40
@xvello
Copy link
Contributor Author

xvello commented Aug 3, 2017

Hi @jhmartin,

Thanks for your feedback. Happy to know this is working for you! As for escaping, if you surround your JSON with single quotes, you don't need to escape double quotes, please have a look at the example I posted

Indeed, JMX is kind of a special child for now as it runs in a separate java process, and does not appear in configtest. You can have a look at /opt/datadog-agent/run/jmx_status.yaml to make sure confs are indeed taken into account.

@xvello xvello changed the title [WIP] allow autoconf templates in docker labels Allow autoconf templates in docker labels Aug 4, 2017
if docker_labels:
kube_config = self._get_docker_config(identifier, docker_labels)
if kube_config is not None:
to_check.update(kube_config[0])
Copy link
Member

@hkaj hkaj Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kube_config --> docker_config same for the 2 lines above

docker_labels = kwargs.get(DOCKER_LABELS)
source = ""

kube_config = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, you can call it config or whatever but it's not k8s specific anymore

@hkaj
Copy link
Member

hkaj commented Aug 10, 2017

left nitpick comments and will give it a spin before we merge but it looks 👍

@xvello xvello force-pushed the xvello/template_in_labels branch from 6c19ba4 to 343e8d8 Compare August 10, 2017 13:49
@xvello xvello force-pushed the xvello/template_in_labels branch from e589e5b to 3f99823 Compare August 10, 2017 13:52
@xvello xvello merged commit 33ee4c2 into master Aug 10, 2017
@xvello
Copy link
Contributor Author

xvello commented Aug 10, 2017

Hi @jhmartin,

I'm just merging that PR, so 5.17 will have support for templates as labels.
Please note we changed the label names to improve consistency.

Regard

@jhmartin
Copy link

@xvello Saw that, no problem.

@jhmartin
Copy link

jhmartin commented Sep 8, 2017

@xvello
Please note that https://docs.docker.com/engine/userguide/labels-custom-metadata/#key-format-recommendations does say:

label key is the left-hand side of the key-value pair. Keys are alphanumeric strings which may contain periods (.) and hyphens (-).
...
These guidelines are not currently enforced and additional guidelines may apply to specific use cases.

It currently works, but relying on underscore in a label name may cause an issue in the future. Better to change it to -, or accept both.

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