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

fix(k8s-views-namespaces): filter metrics to prevent reporting double cpu/memory #61

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

uhthomas
Copy link
Contributor

Pull Request

Required Fields

🔎 What kind of change is it?

  • fix

🎯 What has been changed and why do we need it?

Most queries already use label matcher. The metric reports usage for each individual image, and the whole pod which will result in incorrect reporting if not filtered correctly.

Optional Fields

✔️ Which issue(s) this PR fixes?

Fixes: #60

💬 Additional information?

I'm using = rather than != as it should be more efficient to fetch only the needed metrics rather than individual metrics for each image. I don't think Grafana does any deduplication, so I don't imagine the same query in multiple places will be beneficial. I could be wrong.

… cpu/memory

Most queries already use label matcher. The metric reports usage for each
individual image, and the whole pod which will result in incorrect reporting if
not filtered correctly.

Fixes: dotdc#60
@uhthomas uhthomas requested a review from dotdc as a code owner July 24, 2023 22:56
@uhthomas
Copy link
Contributor Author

Before

image

After

image

@dotdc
Copy link
Owner

dotdc commented Jul 25, 2023

Hi @uhthomas,

Thanks for the fix, I noticed the issue but didn't had time to look into it.
I understand and agree on your point of using = rather than !=, but I don't know if the image="" series get created on all setups. I feel like their could be cases where the image="" series doesn't exist or get dropped, but I could be wrong...
Do you know anything about that?

Currently, all PromQL queries like that are on this format, so I would rather keep them like that for consistency, unless we are confident it will not break for some users, in which case, we could change them all.

Let me know.

@dotdc dotdc self-assigned this Jul 25, 2023
@uhthomas
Copy link
Contributor Author

@dotdc I don't really have a preference. I think there are advantages to both. The label selector image="" should be more efficient as it should be working with fewer overall metrics. Though keeping it the same as all the other queries could actually be better due to caching.

With respect to your question about whether it's safe to use, I believe it is. A cursory search across GitHub doesn't seem to show any instances of this metric being dropped at all.

https://github.com/search?q=container_cpu_usage_seconds_total+image+action%3A+drop+language%3AYAML&type=code

@dotdc
Copy link
Owner

dotdc commented Jul 25, 2023

I'll merge this as is because I like your approach better. Will uniform later if there's no side effects.

Thanks @uhthomas !

@dotdc dotdc merged commit d95e46b into dotdc:master Jul 25, 2023
1 check passed
@dotdc
Copy link
Owner

dotdc commented Sep 4, 2023

@uhthomas
Just switched back to != because it can lead to issues on some setups.
See: #62

@uhthomas
Copy link
Contributor Author

uhthomas commented Sep 4, 2023

@uhthomas

Just switched back to != because it can lead to issues on some setups.

See: #62

Interesting, thanks for letting me know and sorry for the trouble!

@dotdc
Copy link
Owner

dotdc commented Apr 25, 2024

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@dotdc dotdc added the released label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Namespace dashboard shows double resource usage
2 participants