add podCount metric queries in resource_optimization_local_monitoring_norecordingrules.yaml - #2017
Open
mbvreddy wants to merge 1 commit into
Open
add podCount metric queries in resource_optimization_local_monitoring_norecordingrules.yaml#2017mbvreddy wants to merge 1 commit into
mbvreddy wants to merge 1 commit into
Conversation
…_norecordingrules.yaml Signed-off-by: Bhaktavatsal Reddy <bhamaram@in.ibm.com>
Contributor
Reviewer's GuideAdds a new podCount metric to the resource_optimization_local_monitoring_norecordingrules performance profile, using Prometheus queries based on ready containers to capture current, average, max, and min ready pod counts over a measurement window. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="manifests/autotune/performance-profiles/resource_optimization_local_monitoring_norecordingrules.yaml" line_range="458" />
<code_context>
- function: sum
query: 'sum by(container, namespace, runtime, vendor, version)(jvm_info_total{namespace="$NAMESPACE$", container="$CONTAINER_NAME$"})'
+
+ # Pod Count (based on READY containers, not total pods)
+ - name: podCount
+ datasource: prometheus
</code_context>
<issue_to_address>
**issue:** Queries currently count ready containers, not pods, which conflicts with the "Pod Count" metric description.
`kube_pod_container_status_ready` is container-scoped, so this query returns ready containers, not ready pods. Either change the query to aggregate at the pod level (e.g., using `kube_pod_status_phase` / `kube_pod_status_ready` or grouping by pod) or update the metric name/description to clearly indicate it’s counting containers, not pods, to avoid misleading consumers.
</issue_to_address>
### Comment 2
<location path="manifests/autotune/performance-profiles/resource_optimization_local_monitoring_norecordingrules.yaml" line_range="465" />
<code_context>
+ kubernetes_object: "container"
+
+ aggregation_functions:
+ # Current ready pods
+ - function: sum
+ query: 'sum(kube_pod_container_status_ready{namespace="$NAMESPACE$", container="$CONTAINER_NAME$"})'
</code_context>
<issue_to_address>
**issue (bug_risk):** Missing filter on `condition="true"` for `kube_pod_container_status_ready` may overcount non-ready states.
`kube_pod_container_status_ready` exposes one series per `condition` value (e.g. `true`/`false`). Without `condition="true"`, the sum counts all conditions and won’t represent only ready containers. Please add `condition="true"` to the selector to match the intended "ready" semantics.
</issue_to_address>
### Comment 3
<location path="manifests/autotune/performance-profiles/resource_optimization_local_monitoring_norecordingrules.yaml" line_range="469" />
<code_context>
+ - function: sum
+ query: 'sum(kube_pod_container_status_ready{namespace="$NAMESPACE$", container="$CONTAINER_NAME$"})'
+
+ # Avg ready pods over time
+ - function: avg
+ query: 'avg_over_time(sum(kube_pod_container_status_ready{namespace="$NAMESPACE$", container="$CONTAINER_NAME$"})[$MEASUREMENT_DURATION_IN_MIN$m:])'
</code_context>
<issue_to_address>
**issue (bug_risk):** Use of subquery syntax inside `avg_over_time` with `[$MEASUREMENT_DURATION_IN_MIN$m:]` is likely invalid and/or semantically off.
This uses `avg_over_time(sum(metric){...}[$MEASUREMENT_DURATION_IN_MIN$m:])`, which creates a subquery (`[...] :`) and passes its result to a range function. `avg_over_time` expects a plain range vector, and the trailing colon without a step is invalid PromQL. To express "average ready containers over the last N minutes", use a simple range selector, e.g.:
- `avg_over_time(kube_pod_container_status_ready{...}[$MEASUREMENT_DURATION_IN_MIN$m])`, or
- `avg_over_time(sum(kube_pod_container_status_ready{...})[$MEASUREMENT_DURATION_IN_MIN$m])` if you want the average of the aggregated series.
`max_over_time` and `min_over_time` below should be updated similarly (remove the colon and use a plain `[...]` range).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - function: sum | ||
| query: 'sum by(container, namespace, runtime, vendor, version)(jvm_info_total{namespace="$NAMESPACE$", container="$CONTAINER_NAME$"})' | ||
|
|
||
| # Pod Count (based on READY containers, not total pods) |
Contributor
There was a problem hiding this comment.
issue: Queries currently count ready containers, not pods, which conflicts with the "Pod Count" metric description.
kube_pod_container_status_ready is container-scoped, so this query returns ready containers, not ready pods. Either change the query to aggregate at the pod level (e.g., using kube_pod_status_phase / kube_pod_status_ready or grouping by pod) or update the metric name/description to clearly indicate it’s counting containers, not pods, to avoid misleading consumers.
| kubernetes_object: "container" | ||
|
|
||
| aggregation_functions: | ||
| # Current ready pods |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Missing filter on condition="true" for kube_pod_container_status_ready may overcount non-ready states.
kube_pod_container_status_ready exposes one series per condition value (e.g. true/false). Without condition="true", the sum counts all conditions and won’t represent only ready containers. Please add condition="true" to the selector to match the intended "ready" semantics.
| - function: sum | ||
| query: 'sum(kube_pod_container_status_ready{namespace="$NAMESPACE$", container="$CONTAINER_NAME$"})' | ||
|
|
||
| # Avg ready pods over time |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Use of subquery syntax inside avg_over_time with [$MEASUREMENT_DURATION_IN_MIN$m:] is likely invalid and/or semantically off.
This uses avg_over_time(sum(metric){...}[$MEASUREMENT_DURATION_IN_MIN$m:]), which creates a subquery ([...] :) and passes its result to a range function. avg_over_time expects a plain range vector, and the trailing colon without a step is invalid PromQL. To express "average ready containers over the last N minutes", use a simple range selector, e.g.:
avg_over_time(kube_pod_container_status_ready{...}[$MEASUREMENT_DURATION_IN_MIN$m]), oravg_over_time(sum(kube_pod_container_status_ready{...})[$MEASUREMENT_DURATION_IN_MIN$m])if you want the average of the aggregated series.
max_over_time and min_over_time below should be updated similarly (remove the colon and use a plain [...] range).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR address issue #2016. As part of this, podCount metric queries are added to resource_optimization_local_monitoring_norecordingrules.yaml file which was missed in earlier PR #1932
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Add pod readiness-based podCount metric to the resource optimization local monitoring performance profile.
New Features:
Enhancements: