Fix the bug where cri-o doesn't emit any metrics when all is set.#9719
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced uses of Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server as "Metric Server"
participant Config as "StatsConfig"
participant Pop as "PopulateMetricDescriptors"
Client->>Server: Request metric descriptors
Server->>Config: call EnabledPodMetrics()
Note over Config: return internal includedPodMetrics\n(single "all" → AvailableMetrics)
Config-->>Server: includedKeys
Server->>Pop: PopulateMetricDescriptors(includedKeys)
Note over Pop: init descriptorsMap with always-on metrics\niterate includedKeys to set descriptorsMap[k] (no early return)
Pop-->>Server: descriptorsMap
Server->>Server: flatten descriptorsMap to slice
Server-->>Client: MetricDescriptors[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
2221-2238: Exclude "all" from the expanded internal list.
includedPodMetricsis documented as not containing"all", butAvailableMetricsincludes it. This can surface as “unknown metric: all” warnings or break descriptor population if"all"is not handled downstream.🛠️ Suggested fix
if len(c.IncludedPodMetrics) == 1 && c.IncludedPodMetrics[0] == AllMetrics { - c.includedPodMetrics = AvailableMetrics + c.includedPodMetrics = make([]string, 0, len(AvailableMetrics)-1) + for _, m := range AvailableMetrics { + if m == AllMetrics { + continue + } + c.includedPodMetrics = append(c.includedPodMetrics, m) + } return nil }
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9719 +/- ##
==========================================
+ Coverage 64.45% 67.46% +3.01%
==========================================
Files 210 210
Lines 29109 29116 +7
==========================================
+ Hits 18761 19644 +883
+ Misses 8657 7784 -873
+ Partials 1691 1688 -3 🚀 New features to boost your workflow:
|
|
@cri-o/cri-o-maintainers PTAL |
|
|
||
| func (c *StatsConfig) Validate() error { | ||
| if len(c.IncludedPodMetrics) == 1 && c.IncludedPodMetrics[0] == AllMetrics { | ||
| c.includedPodMetrics = AvailableMetrics |
There was a problem hiding this comment.
Should we filter out AllMetrics here?
There was a problem hiding this comment.
I removed "all" from AvailableMetrics. It's only used for this Validate function anyway.
f8eb19a to
d3f63c3
Compare
| // Do NOT use this field directly. Use Metrics() instead. | ||
| IncludedPodMetrics []string `toml:"included_pod_metrics"` |
There was a problem hiding this comment.
I think we should depreciate it to make linters aware.
saschagrunert
left a comment
There was a problem hiding this comment.
Just one non-blocking nit, otherwise LGTM
/hold
Feel free to unhold if you think we should not deprecate the config field.
| return nil | ||
| } | ||
|
|
||
| func (c *StatsConfig) Metrics() []string { |
There was a problem hiding this comment.
Maybe EnabledPodMetrics or something? I wonder if Metrics is too generic, there is a whole metrics table in the config
There was a problem hiding this comment.
I was looking for a better naming. Thanks!
d3f63c3 to
20267b5
Compare
| return nil | ||
| } | ||
|
|
||
| func (c *StatsConfig) EnabledMetrics() []string { |
There was a problem hiding this comment.
I still feel including pod is good here, cri-o has server metrics it exposes which are different than these
There was a problem hiding this comment.
It slipped my eyes. Thanks.
20267b5 to
d761ade
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
2223-2247: Usefmt.Errorfinstead oferrors.New()on line 2232 to align with Go coding guidelines and codebase patterns.The validation logic correctly handles:
- Single "all" element → expands to
AvailableMetrics- "all" mixed with other metrics → returns error (clear guidance)
- Individual metrics → validates against allowed list
However, line 2232 must be changed from
errors.New()tofmt.Errorf()to comply with the coding guidelines and match the codebase pattern (used consistently throughout config.go). WhenIncludedPodMetricsis empty/nil, validation passes and no metrics are collected—this is the intended default behavior.
|
/retest |
|
/unhold @cri-o/cri-o-maintainers PTAL |
|
@cri-o/cri-o-maintainers PTAL |
1 similar comment
|
@cri-o/cri-o-maintainers PTAL |
|
/retest |
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
d761ade to
020b308
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/config/config.go (1)
2271-2275: Shared slice reference to package-levelAvailableMetrics— consider a defensive copy.
c.includedPodMetrics = AvailableMetricsmakes the internal field point to the same backing array as the package-level global. If any consumer ever mutates the returned slice (e.g., viaappendor element assignment), it would silently corruptAvailableMetricsfor all subsequent callers.Suggested fix
if len(c.IncludedPodMetrics) == 1 && c.IncludedPodMetrics[0] == AllMetrics { - c.includedPodMetrics = AvailableMetrics + c.includedPodMetrics = slices.Clone(AvailableMetrics) return nil }
|
sorry rebase removed lgtm, PTAL again @cri-o/cri-o-maintainers |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bitoku, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/cherry-pick release-1.35 |
|
@bitoku: new pull request created: #9781 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes a bug where CRI-O didn't return all metrics when "all" is set.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Bug Fixes
Changes