-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix hubble metrics label ordering with contextOptions #21732
Conversation
b23c843
to
6f6f8bc
Compare
The commit messages are empty. Would you mind adding the context around these changes from your PR description to the commits as well?
If the PR fixes something that isn't yet in any release, then you should use the |
/test Job 'Cilium-PR-K8s-1.24-kernel-4.19' failed: Click to show.Test Name
Failure Output
If it is a flake and a GitHub issue doesn't already exist to track it, comment |
/mlh new-flake Cilium-PR-K8s-1.24-kernel-4.19 👍 created #21740 |
/test-1.24-4.19 |
6f6f8bc
to
9f6bdec
Compare
Label names and label values need to be in the same order. Context label names were at the end of the labels list, but at the beginning of the label values so the label names were mismatched with their label values. Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
Label names and label values need to be in the same order. Refactor DNS metrics handler to be a bit simpler, with less branches and fix the label order mismatch between label names and label values. Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
Label names and label values need to be in the same order. Label names were at the end of the list while hte values were at the beginning, this fixes it so they both are at the beginning. Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
9f6bdec
to
8300d26
Compare
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!
/test |
marking ready to merge, 2 approvals from sig-hubble members. |
This fixes an issue introduced by d4d7368 in #21079 where metrics labels for drop, dns, and tcp metrics are incorrect when using any contextOptions. This is because the order labels are registered in, was different from the order the labels are used when incrementing the counters.
I had to refactor DNS a little bit because the logic was so complicated, and I tried to make it less "stateful" with the labels, which is part of the reason that one got done incorrectly in the first place. drop/tcp were simpler.
I'm not sure this actually needs a release note because I don't think the contextLabels PR has made it into a release yet, but I added one anyways.