Carry #780: Export env variables as prometheus labels #1023

dqminh · 2015-12-16T15:07:03Z

This rebase the patch in #780 and add a few more fixes:

fix regex in test to do the correct thing
sanitize all label name before setting them as prometheus label

cadvisorJenkinsBot · 2015-12-16T15:07:05Z

Can one of the admins verify this patch?

googlebot · 2015-12-16T15:07:06Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

k8s-bot · 2015-12-16T15:08:02Z

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

dqminh · 2015-12-17T14:12:40Z

@vishh i've checked our CLAs and i think we are ok here. Let me know what you think.

vishh · 2015-12-17T17:56:06Z

container/docker/handler.go

Should we keep env as a separate field?

hmm in the end they all come back as metrics labels so i dont really think they are necessary.
Labels makes a lot of sense for metrics, env much less so. We are just stuck with env here because some orchestration platform doesnt support writing labels to docker container.

Sure. The data generated here can be exposed in many different ways - REST API, prometheus endpoint, storage sinks, etc. I'd prefer keeping env and labels separate at this layer.
If the prometheus endpoint chooses to combine labels and env, that's fine by me.

agreed. Updated it to add Envs as a source of metadata to ContainerSpec. PTAL

dqminh · 2016-01-04T12:30:04Z

ping @vishh

the previous regex wasnt able to match anything. This regex should hopefully do better. Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

vishh · 2016-01-05T14:27:34Z

container/docker/factory.go

This feels more like labels. Why not expose this additional metadata as labels instead?

Infact why not add it to docker containers directly? Is this needed to provide backwards compat with old docker versions?

main reason was noted here: #78

tldr: the orchestrator doesnt understand docker labels, and set their metadata as env variable. Until that's fixed, we need this to make sense of those containers.

Ok. Why add a flag to cadvisor?

this is to select the env that we want to publish as metrics labels ( I made a similar flag for logging in docker moby/moby#16961 )

For example, a mesos container has the following configs:

"Env": [ "GOMAXPROCS=64", "MARATHON_APP_VERSION=2015-11-18T22:55:53.442Z", "MARATHON_APP_DOCKER_IMAGE=docker/service", "MESOS_TASK_ID=service.3f90dfce-12314235", "MARATHON_APP_ID=/service", "MESOS_SANDBOX=/mnt/mesos/sandbox", "MESOS_CONTAINER_NAME=mesos-asdsfdfds", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", "container=docker" ], "Labels": {}, // sad :(

we only interest in MARATHON_APP_ID,MESOS_TASK_ID,MESOS_CONTAINER_NAME as the container identifiers.

A flag is reasonably useful for use as we only use 1 orchestrator i.e. mesos, so all containers will have the same set of env keys. This is the chosen approach in the original PR.

I can imagine an alternative approach would be to let the administrator defined their desired env keys as a well known label, for example:

// this will not be set as metrics in prometheus "Labels": { "cadvisor.metadata_env": "MARATHON_APP_ID,MESOS_TASK_ID,MESOS_CONTAINER_NAME" }

Thanks for explaining @dqminh. The flag name doesn't seem to convey the fact that it is a filter. Can you fix the naming and description to make it clear that it is a filter?

SGTM. What do you think of

-allowed-docker-env-as-metadata: a comma-separated list of allowed environment variable names for docker container that should have their name values set as the container's metadata

How about --docker-env-metadata-whitelist: a comma-separated list of environment variable keys that needs to be collected for docker containers?
And BTW will this default to all env variables? To me that feels like a good default.

--docker-env-metadata-whitelist: a comma-separated list of environment variable keys that needs to be collected for docker containers

👍

Hmm, i thinkcollecting all env variables as the default is debatable.

In term of metrics collection, many env variables are strictly for runtime usage and doesn't make much sense in term of metrics which is why the whitelist approach ( i.e. disabled by default ) feels right. I think enabling all env variables by default will likely bite some users in their foot if they are not careful.

However, if we are talking about collecting information of container for exposing to others i.e. cadvisor is the only place to discover containers' information on the system then collecting all env by default makes sense.

I do agree with you. Env variables sometimes contain auth information. Disabled by default SGTM.
If we were to support collecting all env variables, then we might have to support wildcards in the flag though.

dqminh · 2016-01-07T13:22:45Z

@vishh added references to API v2 and changed the flag.

I'm keeping the env whitelist i.e. not collecting all env variables by default. PTAL.

vishh · 2016-01-08T19:09:07Z

container/docker/factory.go

nit: dockerEnvWhitelist would be a more readable name for this variable.

vishh · 2016-01-08T19:11:00Z

Just a couple of nits. Otherwise LGTM! Thanks @dqminh

dqminh · 2016-01-12T15:46:40Z

@vishh updated

vishh · 2016-01-12T23:37:16Z

LGTM

vishh · 2016-01-12T23:37:39Z

can you squash your commits?

This add Envs to container spec as a metadata source. When using prometheus exposition format, they will be merged into the list of metrics' labels. Also changed the cli flag to docker_env_metadata_whitelist, and add refenrences of whitelist envs to API Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

dqminh · 2016-01-13T11:24:39Z

@vishh squashed the changes to envs into 1 commits.

vishh · 2016-01-13T18:45:04Z

LGTM

vishh · 2016-01-13T18:45:21Z

@k8s-bot test this please

k8s-bot · 2016-01-13T18:51:56Z

Jenkins GCE e2e

Build/test failed for commit e5b6bfa.

Build Log

vishh · 2016-01-13T19:15:06Z

@k8s-bot test this please

k8s-bot · 2016-01-13T19:17:54Z

Jenkins GCE e2e

Build/test passed for commit e5b6bfa.

Build Log

vishh · 2016-01-13T19:30:01Z

Merging this since e2e has passed.

Carry #780: Export env variables as prometheus labels

vishh reviewed Dec 17, 2015
View reviewed changes

Florian Pfitzer and others added 3 commits January 5, 2016 10:49

Export image name and env variables as prometheus labels

009761c

fix prometheus regex matching

9131bcb

the previous regex wasnt able to match anything. This regex should hopefully do better. Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

satitize label name before setting them as prometheus label

544b852

Signed-off-by: Daniel Dao <dqminh@cloudflare.com>

dqminh force-pushed the metadata_env branch from 3c87af0 to 211f545 Compare January 5, 2016 12:44

vishh reviewed Jan 5, 2016
View reviewed changes

vishh reviewed Jan 8, 2016
View reviewed changes

container/docker/factory.go Outdated

Copy link

Contributor

vishh Jan 8, 2016

Choose a reason for hiding this comment

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

nit: dockerEnvWhitelist would be a more readable name for this variable.

dqminh force-pushed the metadata_env branch from bc19250 to e5b6bfa Compare January 13, 2016 11:23

vishh closed this Jan 13, 2016

vishh reopened this Jan 13, 2016

vishh added a commit that referenced this pull request Jan 13, 2016

Merge pull request #1023 from dqminh/metadata_env

9cfc8ce

Carry #780: Export env variables as prometheus labels

vishh merged commit 9cfc8ce into google:master Jan 13, 2016

dqminh mentioned this pull request Jan 18, 2016

Initialize env mapping for labels when creating docker container #1063

Merged

r0fls mentioned this pull request Oct 17, 2017

Export image name and env variables as prometheus labels #780

Closed

Carry #780: Export env variables as prometheus labels #1023

Carry #780: Export env variables as prometheus labels #1023

Uh oh!

Conversation

dqminh commented Dec 16, 2015

Uh oh!

cadvisorJenkinsBot commented Dec 16, 2015

Uh oh!

googlebot commented Dec 16, 2015

Uh oh!

k8s-bot commented Dec 16, 2015

Uh oh!

dqminh commented Dec 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dqminh commented Jan 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dqminh commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishh commented Jan 8, 2016

Uh oh!

dqminh commented Jan 12, 2016

Uh oh!

vishh commented Jan 12, 2016

Uh oh!

vishh commented Jan 12, 2016

Uh oh!

dqminh commented Jan 13, 2016

Uh oh!

vishh commented Jan 13, 2016

Uh oh!

vishh commented Jan 13, 2016

Uh oh!

k8s-bot commented Jan 13, 2016

Uh oh!

vishh commented Jan 13, 2016

Uh oh!

k8s-bot commented Jan 13, 2016

Uh oh!

vishh commented Jan 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants