-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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(inputs.prometheus): pod_annotation_template and pod_label_template #10946
feat(inputs.prometheus): pod_annotation_template and pod_label_template #10946
Conversation
7192d18
to
61b53d5
Compare
@imranismail thank you for the pull request, could you resolve the merge conflict? |
plugins/inputs/prometheus/README.md
Outdated
# pod_label_template = "pod.label/{{ . }}" | ||
# pod_annotation_template = "pod.annotation/{{ . }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to make these prefixes instead of templates? That way users wouldn't need to worry about template syntax when they're prepending static strings.
fb3251e
to
4dbdd47
Compare
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @imranismail! I have some comments and second @reimda's question regarding only using a prefix string instead of a whole template. Furthermore, I suggest to manipulate the tag-key at the origin instead of looping the logic though the code.
## Pod label & annotation tag template | ||
## Default is "{{ . }}" | ||
## Useful to glob match labels & annotations in "tagexclude" or "taginclude" | ||
# pod_label_template = "{{ . }}" | ||
# pod_annotation_template = "{{ . }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to make this a template or can we get away with defining a prefix? I'm asking because this adds complexity that might not be required and furthermore it's not completely intuitive to use a template, usually use to generate stuff, for parsing...
+ pod_label_template = 'pod.label/{{ . }}' | ||
+ pod_annotation_template = 'pod.annotation/{{ . }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, we could cover this with just
pod_label_prefix = 'pod.label'
pod_annotation_prefix = 'pod.annotation'
extraTags[tagName] = buffer.String() | ||
extraTags[tagName] = Tag{Value: buffer.String()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
tags := map[string]Tag{} | ||
tags["pod_name"] = Tag{Value: pod.Name} | ||
tags["namespace"] = Tag{Value: pod.Namespace} | ||
// add annotation as metrics tags | ||
tags := pod.Annotations | ||
if tags == nil { | ||
tags = map[string]string{} | ||
for k, v := range pod.Annotations { | ||
tags[k] = Tag{Value: v, Template: p.podAnnotationTmpl} | ||
} | ||
tags["pod_name"] = pod.Name | ||
tags["namespace"] = pod.Namespace | ||
// add labels as metrics tags | ||
for k, v := range pod.Labels { | ||
tags[k] = v | ||
tags[k] = Tag{Value: v, Template: p.podLabelTmpl} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to directly apply the modification here instead of looping this through all the code?
Hi @imranismail are you able to work on the changes @srebhan and I requested? Thanks! |
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Page. Thank you! |
Required for all PRs:
Closes 10945