metrics: add default_runtime#9870
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haircommander 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 |
|
assisted by claude |
📝 WalkthroughWalkthroughAdds a Prometheus gauge metric and collector to track CRI-O's configured default runtime, sets that metric at daemon startup and on SIGHUP reload, registers the metric conditionally with the collectors list, adds a Bats test to validate the metric, and documents the new collector. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as cmd/crio/main
participant Server as server (SIGHUP/reload)
participant Metrics as server/metrics
participant PromReg as PrometheusRegistry
participant Scraper as MetricsScraper
Main->>Metrics: MetricDefaultRuntimeSet(DefaultRuntime)
Metrics->>PromReg: Reset()
Metrics->>PromReg: GetMetricWithLabelValues(runtime=DefaultRuntime) / Set(1)
Server->>Metrics: on SIGHUP -> MetricDefaultRuntimeSet(NewDefault)
Metrics->>PromReg: Reset()
Metrics->>PromReg: GetMetricWithLabelValues(runtime=NewDefault) / Set(1)
Scraper->>PromReg: GET /metrics
PromReg-->>Scraper: container_runtime_crio_default_runtime{runtime="..."} 1
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9870 +/- ##
==========================================
- Coverage 67.57% 67.53% -0.04%
==========================================
Files 212 214 +2
Lines 29373 29628 +255
==========================================
+ Hits 19848 20010 +162
- Misses 7827 7909 +82
- Partials 1698 1709 +11 🚀 New features to boost your workflow:
|
5a5ede5 to
48f1369
Compare
|
/retest |
|
/cherry-pick release-1.35 |
|
@haircommander: once the present PR merges, I will cherry-pick it on top of 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. |
|
/override ci/prow/e2e-aws-ovn |
|
@haircommander: Overrode contexts on behalf of haircommander: ci/prow/e2e-aws-ovn 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. |
bitoku
left a comment
There was a problem hiding this comment.
It looks like reloading config is not considered (+its integration test)
48f1369 to
4fe04af
Compare
|
great point, updated |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/server.go (2)
635-635: Guard the reload metric write behindEnableMetrics.This runs on every successful reload and still touches global metrics state even when metrics are disabled. Consider gating it to avoid unnecessary singleton interaction.
Proposed change
- metrics.Instance().MetricDefaultRuntimeSet(s.config.DefaultRuntime) + if s.config.EnableMetrics { + metrics.Instance().MetricDefaultRuntimeSet(s.config.DefaultRuntime) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/server.go` at line 635, The reload handler currently always calls metrics.Instance().MetricDefaultRuntimeSet(s.config.DefaultRuntime), touching global metrics even when metrics are disabled; wrap that call in a guard checking the server config's metrics toggle (e.g., s.config.EnableMetrics or equivalent) so the metric update runs only when metrics are enabled, keeping the metrics singleton untouched otherwise and preserving current behavior when enabled.
635-635: Please mirror this metric feature in downstream repos.Since this line adds runtime-metric behavior on reload, make sure cri-o.io and packaging repos are updated so docs/defaults stay aligned.
Based on learnings, "When adding/changing features, update related repositories: cri-o.io website and packaging repositories".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/server.go` at line 635, This change calls metrics.Instance().MetricDefaultRuntimeSet(s.config.DefaultRuntime) to record the default runtime on reload; mirror this metric behavior in downstream repos by updating cri-o.io and packaging to set and document the same default-runtime metric on config reloads, ensure documentation pages and defaults in packaging templates reflect s.config.DefaultRuntime semantics and examples, and add any necessary metric initialization code or config hooks in those repositories equivalent to MetricDefaultRuntimeSet so docs, packaging defaults, and runtime behavior stay aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/server.go`:
- Line 635: The reload handler currently always calls
metrics.Instance().MetricDefaultRuntimeSet(s.config.DefaultRuntime), touching
global metrics even when metrics are disabled; wrap that call in a guard
checking the server config's metrics toggle (e.g., s.config.EnableMetrics or
equivalent) so the metric update runs only when metrics are enabled, keeping the
metrics singleton untouched otherwise and preserving current behavior when
enabled.
- Line 635: This change calls
metrics.Instance().MetricDefaultRuntimeSet(s.config.DefaultRuntime) to record
the default runtime on reload; mirror this metric behavior in downstream repos
by updating cri-o.io and packaging to set and document the same default-runtime
metric on config reloads, ensure documentation pages and defaults in packaging
templates reflect s.config.DefaultRuntime semantics and examples, and add any
necessary metric initialization code or config hooks in those repositories
equivalent to MetricDefaultRuntimeSet so docs, packaging defaults, and runtime
behavior stay aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60176a72-bf68-42f4-8592-4d0f7af87cdd
📒 Files selected for processing (7)
cmd/crio/main.godocs/crio.8.mdserver/metrics/collectors/collectors.goserver/metrics/metrics.goserver/server.gotest/metrics.batstest/updateunified/updateunified
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/crio/main.go
- docs/crio.8.md
- server/metrics/collectors/collectors.go
- server/metrics/metrics.go
- test/metrics.bats
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@haircommander The shellcheck failed. |
we consistently need to find which runtime a user is using, and have talked about adding this for a while. Now's the time! Signed-off-by: Peter Hunt <pehunt@redhat.com>
4fe04af to
50bad6d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/metrics/collectors/collectors.go (1)
70-71: Prefer a rationale-focused comment forDefaultRuntimeThe constant is good, but the new comment explains what the key is rather than why this collector exists. Consider rewording it to capture operational intent (for example, exposing configured runtime for observability/alerting).
As per coding guidelines, "Add comments explaining 'why' not 'what' in Go code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/metrics/collectors/collectors.go` around lines 70 - 71, The comment for the constant DefaultRuntime currently restates what the key is; update it to explain why this collector exists (operational intent) — e.g., state that DefaultRuntime exposes the container runtime configured in CRI-O so operators and alerting/observability systems can detect runtime changes or mismatches; locate the comment above the DefaultRuntime Collector constant and replace the “what” description with a concise “why” rationale focused on observability/alerting/operational use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/metrics/collectors/collectors.go`:
- Around line 70-71: The comment for the constant DefaultRuntime currently
restates what the key is; update it to explain why this collector exists
(operational intent) — e.g., state that DefaultRuntime exposes the container
runtime configured in CRI-O so operators and alerting/observability systems can
detect runtime changes or mismatches; locate the comment above the
DefaultRuntime Collector constant and replace the “what” description with a
concise “why” rationale focused on observability/alerting/operational use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df07bcca-ea54-438a-bb38-693c8367c70b
📒 Files selected for processing (7)
cmd/crio/main.godocs/crio.8.mdserver/metrics/collectors/collectors.goserver/metrics/metrics.goserver/server.gotest/metrics.batstest/updateunified/updateunified
✅ Files skipped from review due to trivial changes (2)
- docs/crio.8.md
- cmd/crio/main.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/server.go
- server/metrics/metrics.go
- test/metrics.bats
|
/retest |
|
/lgtm |
|
@haircommander: new pull request created: #9897 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. |
we consistently need to find which runtime a user is using, and have talked about adding this for a while. Now's the time!
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
New Features
Tests
Documentation