-
Notifications
You must be signed in to change notification settings - Fork 174
Add jobLabel to annotation autodiscovery #1389
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
Conversation
It was missing.
Honestly, I don't know if this makes sense as the right job label. I think a more appropriate job label might be the first non-empty value:
|
Sorry I missed your notification here - we do have that Strictly, we already have a annotation check that defaults to k8s-monitoring-helm/charts/k8s-monitoring/charts/feature-annotation-autodiscovery/values.yaml Line 32 in 9536973
|
Just updated the PR to make it optional :) |
ping @petewall |
ping @petewall @nevermind89x @skl . Sorry for the spam! |
Like I mentioned in my previous comment, this change as it is now it not how I think this should be implemented. Before this change, the
I agree that the default job label is not excellent and we can improve this. Your PR would always set the job label to The correct method would be check in this order:
And skip the jobLabel altogether. Why this order? The Job label is quite important and is used to identify the different workloads. In our case, the workloads are not distinct based on the fact that they're scraped via annotations, but on something the user defines ( Also, why container name over pod name? because Pod names are often appended with random characters and are ephemeral. Container names are much more consistent and an appropriate default. |
So, after all that, what's the course of action? If you want to take this on, find the Alloy rule where we set
This will only set |
closing in favor of #1717 |
It was missing.