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

feat(kube-eagle): Add pod labels to metrics #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xcode03
Copy link

@xcode03 xcode03 commented Sep 3, 2020

  • Add label information for each pod-related metrics

@weeco
Copy link
Member

weeco commented Sep 3, 2020

This PR adds a single label with the concatenated labels from the scraped pods.

I'm not entirely sure if and how it's possible but the feature request and my personal preference is having separated labels instead, see: #7

When all nodes in your Kubernetes cluster have a label: nodepool = default this should also be part of the exposed node metrics from Kube eagle. Likewise for pods.

@xcode03
Copy link
Author

xcode03 commented Sep 4, 2020

I understand what you mean, separate labels are indeed more beautiful and more readable. However, if there is a duplicate label (the same key but different value) in the extra label that is exposed by kube-eagle itself, then one label will be overwritten.
So I think it’s more practical to aggregate labels into "labels"

@weeco
Copy link
Member

weeco commented Sep 5, 2020

This is a valid concern, we could prefix the label_names though (e.g. with eagle_) to guarantee they will be unique and are not going to be overwritten. What do you think?

@xcode03
Copy link
Author

xcode03 commented Sep 7, 2020

I think your idea is very good, but I encountered another problem during the coding process: the key of each pod label is different, but prometheus seems unable to define dynamic labels. So I think it is more feasible to use the "labels" key to solidify all indicators. When extracting the label, you only need to cut according to the space to get the required data without too much extra work.

@liorfranko
Copy link

Any updates?

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.

3 participants