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

Add support for %%id%% and %%pid%% template variables #3404

Merged
merged 2 commits into from
Jul 7, 2017
Merged

Add support for %%id%% and %%pid%% template variables #3404

merged 2 commits into from
Jul 7, 2017

Conversation

sophaskins
Copy link
Contributor

What does this PR do?

This PR adds two new template variables to the service-discovery check configs:

  • pid the host PID namespace PID of a container processes' PID 0
  • id the docker container id hash

Motivation

We have a few custom integrations that collect information from /proc. When we were running only one instance of a process per-host (pre-containers), it was easy to determine the correct pid to interrogate. Now that we're using Kubernetes and Docker, there is often many such processes per machine. These variables help us find the process we want.

Notes

This PR Is actually authored by @jnewland - I'm submitting it upstream because I've been working on our dd-agent packaging setup.

@hkaj
Copy link
Member

hkaj commented Jun 30, 2017

Hi @sophaskins and @jnewland
Thanks for the contrib! I think I understand the use case for pid, what do you need the container ID for though? I'm a bit wary about adding it as a template variable as if it's used as a tag it could be very spammy for us.

@sophaskins
Copy link
Contributor Author

I took a look at how we were using the container id, and it's for something that we can definitely just use the pid for (in our case, we were using the container id to identify if processes were part of the container, rather than doing something simpler like "get all the children of %%pid%%)

Shall I remove the id part of the PR?

@xvello
Copy link
Contributor

xvello commented Jul 4, 2017

I'm ok with keeping id, as it could be used in custom checks, but I'd rename it cid for disambiguation.

Indeed, someone using either pid or cid as a tag value would give our backend a hard time, but I think we should find a way to forbid using them (and even host btw) in the tag array. What do you think @hkaj?

@hkaj
Copy link
Member

hkaj commented Jul 6, 2017

@sophaskins great, thanks for the details! Then yes, please remove the id part. @xvello I agree we should move in this direction, but we don't have this in place yet and this id variable is too tempting to use, it's bound to happen if we keep it. We can always add it back later once a tag filtering system is in place.

@sophaskins
Copy link
Contributor Author

Fixed!

@hkaj hkaj added this to the 5.15 milestone Jul 7, 2017
@hkaj hkaj merged commit 924bebc into DataDog:master Jul 7, 2017
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.

4 participants