Support OpenTelemetry metrics alongside kube-state-metrics - #546
Conversation
|
|
WalkthroughThis change adds Kube State Metrics and OpenTelemetry dialects, automatic dialect detection, configurable pod-owner resolution, recording-rule support, CLI and configuration fields, dialect-aware Prometheus queries, documentation, and tests. ChangesPrometheus dialect and owner resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant PrometheusMetricsService
participant Prometheus
participant MetricLoaders
CLI->>Config: Set dialect and owner-resolution options
Config->>PrometheusMetricsService: Provide Prometheus settings
PrometheusMetricsService->>Prometheus: Probe metric availability
Prometheus-->>PrometheusMetricsService: Return probe results
PrometheusMetricsService->>MetricLoaders: Pass selected dialect
MetricLoaders->>Prometheus: Query dialect-specific metrics
Prometheus-->>MetricLoaders: Return metric data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Cross-references to existing issues/PRs, for whoever reviews this: Overlaps with #420 — that PR changes the same I am happy to rebase on top of #420 in whichever order you prefer. Worth noting: #420 also drops the Partially answers #354 (complete list of required metrics) — the new README section tabulates every Kubernetes state metric KRR queries, in both dialects, with the OpenTelemetry receiver each comes from. Related but not closed by this PR:
If #512 or #323 (pydantic v2) lands first, |
20f2158 to
863ad99
Compare
|
Updated to fold #420 in, so the two PRs no longer collide over the same four lines. The Difference from #420: the Also expanded the README requirements list to name every metric KRR queries and the component that exposes it, which is what #354 asks for. 74 tests pass, including a new case asserting the cluster-summary matchers stay correct when |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
robusta_krr/core/models/config.py (1)
54-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
prometheus_workload_recording_ruleas a PromQL metric name.This value is interpolated straight into PromQL in
prometheus_metrics_service.pyat Line 477 and Line 290.Configis aBaseSettingsclass, so the value can also arrive from the environment. If the value is empty or contains PromQL syntax, the generated query is invalid. The probe path catches the exception and reports the metric as absent, so the misconfiguration stays silent.Add a validator that rejects values which are not valid metric or recording-rule names.
🛡️ Proposed validator
+ `@pd.validator`("prometheus_workload_recording_rule") + def validate_prometheus_workload_recording_rule(cls, v: str) -> str: + if not re.fullmatch(r"[a-zA-Z_:][a-zA-Z0-9_:]*", v): + raise ValueError("--prometheus-workload-recording-rule must be a valid PromQL metric name") + return vAdd
import reat the top of the file if it is not present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robusta_krr/core/models/config.py` around lines 54 - 57, Add validation for Config.prometheus_workload_recording_rule using a compiled regular expression that accepts only valid PromQL metric or recording-rule names and rejects empty or syntax-containing values, including environment-provided settings. Import re if needed and ensure invalid values raise the model’s standard validation error before they are interpolated by prometheus_metrics_service.py.robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (1)
34-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCouple the workload-type allowlist to the configured recording rule.
RECORDING_RULE_WORKLOAD_TYPEShardcodes the default rule coverage, while--prometheus-workload-recording-rulecan set another rule. Override a rule that covers additional workload types still forces owner-metric fallback, and a rule that omits one of the allowed types still makes KRR fail to find related pods for that kind. Expose the coveredworkload_typevalues as a setting, or derive them from the configured rule when autodetecting owner resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py` around lines 34 - 36, Update the recording-rule workload coverage used by the owner-resolution logic around RECORDING_RULE_WORKLOAD_TYPES so it is coupled to the configured --prometheus-workload-recording-rule instead of always using the hardcoded default set. Expose the covered workload_type values as configuration, or derive them when autodetecting owner resolution, and ensure custom rules both support additional kinds and fall back to owner metrics for omitted kinds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 165: Update the README description for --prometheus-owner-resolution auto
to state that it uses the owner chain when kube_pod_owner has data, otherwise
uses the recording rule when that has data, and falls back to the
kube-state-metrics strategy when neither has data.
- Line 164: Update the README description of --prometheus-owner-resolution
recording-rule to require the recording rule’s namespace label alongside
workload, workload_type, and pod, and document that the configured
--prometheus-cluster-label label must also be exposed when that option is used.
- Around line 144-147: Update the README’s top-level requirements near the
kube-state-metrics statement to clarify that kube-state-metrics is optional when
using the OpenTelemetry metrics dialect, while retaining cAdvisor as a required
dependency. Ensure the surrounding OpenTelemetry section and dialect guidance
remain consistent with this exception.
In
`@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py`:
- Around line 222-233: Update _metric_has_data to query the metric over a recent
lookback range rather than as an instant vector, while preserving the existing
cluster-label filter and boolean/error-handling behavior. Also update the probe
regex in tests/test_metric_dialects.py to match the new range-query shape.
In `@robusta_krr/core/models/metric_dialects.py`:
- Around line 109-125: Update OTEL_DIALECT and the query builders using
memory_resource_selector, cpu_resource_selector, pod_phase_running_selector, and
oom_reason_label so OTel metrics are queried only with labels they actually
expose. Add explicit metric-label mappings where supported, or gate/skip
selector clauses when the configured value is empty; preserve KSM_DIALECT
behavior.
---
Nitpick comments:
In
`@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py`:
- Around line 34-36: Update the recording-rule workload coverage used by the
owner-resolution logic around RECORDING_RULE_WORKLOAD_TYPES so it is coupled to
the configured --prometheus-workload-recording-rule instead of always using the
hardcoded default set. Expose the covered workload_type values as configuration,
or derive them when autodetecting owner resolution, and ensure custom rules both
support additional kinds and fall back to owner metrics for omitted kinds.
In `@robusta_krr/core/models/config.py`:
- Around line 54-57: Add validation for
Config.prometheus_workload_recording_rule using a compiled regular expression
that accepts only valid PromQL metric or recording-rule names and rejects empty
or syntax-containing values, including environment-provided settings. Import re
if needed and ensure invalid values raise the model’s standard validation error
before they are interpolated by prometheus_metrics_service.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a1e342a-a992-466e-b7d4-d35994e24956
📒 Files selected for processing (8)
README.mdrobusta_krr/core/integrations/prometheus/metrics/base.pyrobusta_krr/core/integrations/prometheus/metrics/memory.pyrobusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.pyrobusta_krr/core/models/config.pyrobusta_krr/core/models/metric_dialects.pyrobusta_krr/main.pytests/test_metric_dialects.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (1)
311-328: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winConstruct the metric loader outside the
tryblock.The
tryblock now covers theLoaderClass(...)call. If the constructor raises,databecomes{}and control reaches thewarning_on_no_databranch, wheremetric_loaderis still unbound. Python then raisesUnboundLocalError, which masks the original error.PrometheusMetric.__init__raisesValueErrorwhenpods_batch_size <= 0, so this path is reachable.🛠️ Proposed fix
dialect = await self.get_dialect() + metric_loader = LoaderClass(self.get_prometheus(), self.name(), self.executor, dialect) try: - metric_loader = LoaderClass(self.get_prometheus(), self.name(), self.executor, dialect) data = await metric_loader.load_data(object, period, step)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py` around lines 311 - 328, Move the MetricLoader construction in the method containing the shown history-loading logic outside the try block, leaving only load_data execution under exception handling. Ensure constructor failures are not converted into empty data that later references an unbound metric_loader, while preserving the existing logging and no-data behavior for load_data failures.
🧹 Nitpick comments (1)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (1)
288-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog a warning when neither owner source is present.
_detect_dialectwarns when no dialect probe matches, but_detect_owner_resolutionfalls back toKUBE_STATE_METRICSsilently. On a cluster withoutkube_pod_ownerand without the recording rule, everyload_podscall then returns an empty pod list, and the operator only sees "no metrics" warnings later. A warning here names the actual cause.♻️ Proposed change
if await self._metric_has_data(settings.prometheus_workload_recording_rule): return OwnerResolutionName.RECORDING_RULE + logger.warning( + "Could not detect a pod owner resolution strategy (neither kube_pod_owner nor %s returned data), " + "falling back to '%s'. Set --prometheus-owner-resolution to select one explicitly.", + settings.prometheus_workload_recording_rule, + OwnerResolutionName.KUBE_STATE_METRICS.value, + ) return OwnerResolutionName.KUBE_STATE_METRICS🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py` around lines 288 - 298, Update _detect_owner_resolution so that when both kube_pod_owner and settings.prometheus_workload_recording_rule lack data, it logs a warning identifying that neither owner metric source is available before returning the existing KUBE_STATE_METRICS fallback. Keep the configured and detected-source branches unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 141-143: Update the historical-pod fallback note in the README to
explicitly name the three metrics whose absence restricts recommendations to
currently running pods: kube_pod_container_resource_limits,
kube_pod_container_status_last_terminated_reason, and kube_job_owner.
---
Outside diff comments:
In
`@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py`:
- Around line 311-328: Move the MetricLoader construction in the method
containing the shown history-loading logic outside the try block, leaving only
load_data execution under exception handling. Ensure constructor failures are
not converted into empty data that later references an unbound metric_loader,
while preserving the existing logging and no-data behavior for load_data
failures.
---
Nitpick comments:
In
`@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py`:
- Around line 288-298: Update _detect_owner_resolution so that when both
kube_pod_owner and settings.prometheus_workload_recording_rule lack data, it
logs a warning identifying that neither owner metric source is available before
returning the existing KUBE_STATE_METRICS fallback. Keep the configured and
detected-source branches unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cfc61e96-9c8c-4d70-9716-1d173829ad49
📒 Files selected for processing (8)
README.mdrobusta_krr/core/integrations/prometheus/metrics/base.pyrobusta_krr/core/integrations/prometheus/metrics/memory.pyrobusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.pyrobusta_krr/core/models/config.pyrobusta_krr/core/models/metric_dialects.pyrobusta_krr/main.pytests/test_metric_dialects.py
🚧 Files skipped from review as they are similar to previous changes (5)
- robusta_krr/core/integrations/prometheus/metrics/base.py
- robusta_krr/core/models/config.py
- robusta_krr/core/integrations/prometheus/metrics/memory.py
- tests/test_metric_dialects.py
- robusta_krr/core/models/metric_dialects.py
863ad99 to
6c27c64
Compare
|
Went through the CodeRabbit review. Summary of what changed and what I pushed back on. Accepted — the label mapping ( Now: the requirement is documented with the Accepted — probe over a lookback window. Detection now uses Accepted — all four README findings. kube-state-metrics is now stated as replaceable (cAdvisor is not), the recording rule's required labels include Unrelated fix found while addressing the above: Also verified against a live OTel/Thanos stack ( One data point for #420 from that stack: 77 tests pass. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_metric_dialects.py (3)
388-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the cluster label in the recording-rule query.
README.mdLine 180 requires the recording rule to expose the label passed to--prometheus-cluster-label._load_related_pods_by_recording_ruleappends that label. No test covers it, so a regression that drops the cluster label from this path stays silent and returns pods from every cluster.Add
prometheus_label="cluster", prometheus_cluster_label="prod"to theconfigure(...)call and extend the expected query at Line 406.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metric_dialects.py` around lines 388 - 409, Update test_load_pods_with_recording_rule to configure prometheus_label as "cluster" and prometheus_cluster_label as "prod", then extend the expected recording-rule query assertion to require the cluster label filter. Keep the existing pod query and assertions unchanged.
309-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a concurrent-detection test.
test_detection_is_cachedcovers sequential calls only. The_dialect_lockexists to make two concurrent callers share one detection pass, and no test exercises that path.gather_dataruns per workload underasyncio.gather, so concurrent first calls are the normal case.♻️ Suggested additional test
`@pytest.mark.asyncio` async def test_concurrent_detection_probes_once(configure): service = FakePrometheusMetricsService(existing_metrics=(OTEL_PROBE,)) dialects = await asyncio.gather(*(service.get_dialect() for _ in range(5))) assert dialects == [OTEL_DIALECT] * 5 assert len(service.queries) == 2 # kube-state-metrics probe, then otel probeAdd the same shape for
get_owner_resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metric_dialects.py` around lines 309 - 317, Add concurrent-call tests alongside test_detection_is_cached for both get_dialect and get_owner_resolution, using asyncio.gather with multiple first callers. Assert every caller receives the expected result and that service.queries contains only the single expected detection probe sequence, validating _dialect_lock coordination.
51-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the global config after each test.
The fixture calls
Config.set_configand never restores the previous value. The last config this module sets stays installed for the rest of the session. Any later test module that readssettingsthen observesprometheus_metrics_dialect=kube-state-metricsandprometheus_cluster_label="prod"fromtest_cluster_summary_keeps_the_cluster_label. That makes results depend on collection order.Convert the fixture to
yieldand restore the prior config.♻️ Proposed refactor
`@pytest.fixture` def configure(): def _configure(**kwargs) -> None: Config.set_config( Config( ... ) ) _configure() - return _configure + yield _configure + Config._config = None # type: ignore[attr-defined]Confirm the attribute name that
Config.set_configwrites before you apply this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metric_dialects.py` around lines 51 - 69, Update the configure fixture to capture the existing global configuration attribute used by Config.set_config, yield the configuration helper for the test, then restore the captured value afterward through the same configuration mechanism. Confirm the exact attribute written by Config.set_config before implementing the teardown so later tests see the prior settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py`:
- Around line 310-321: Update _detect_owner_resolution so that when both
kube_pod_owner and settings.prometheus_workload_recording_rule have no data, it
emits the same diagnostic warning pattern used by _detect_dialect before
returning KUBE_STATE_METRICS. Preserve the existing source-priority behavior and
fallback return value.
In `@robusta_krr/main.py`:
- Around line 167-172: Update the prometheus_workload_recording_rule option help
text to list every required label used by _load_related_pods_by_recording_rule:
workload, workload_type, pod, namespace, and the cluster label returned by
get_prometheus_cluster_label(). Keep the existing recording-rule description and
ensure the help text matches the requirements documented in README.md.
---
Nitpick comments:
In `@tests/test_metric_dialects.py`:
- Around line 388-409: Update test_load_pods_with_recording_rule to configure
prometheus_label as "cluster" and prometheus_cluster_label as "prod", then
extend the expected recording-rule query assertion to require the cluster label
filter. Keep the existing pod query and assertions unchanged.
- Around line 309-317: Add concurrent-call tests alongside
test_detection_is_cached for both get_dialect and get_owner_resolution, using
asyncio.gather with multiple first callers. Assert every caller receives the
expected result and that service.queries contains only the single expected
detection probe sequence, validating _dialect_lock coordination.
- Around line 51-69: Update the configure fixture to capture the existing global
configuration attribute used by Config.set_config, yield the configuration
helper for the test, then restore the captured value afterward through the same
configuration mechanism. Confirm the exact attribute written by
Config.set_config before implementing the teardown so later tests see the prior
settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cd39a18-ff07-4cd6-bdbf-c0cd586c5e07
📒 Files selected for processing (8)
README.mdrobusta_krr/core/integrations/prometheus/metrics/base.pyrobusta_krr/core/integrations/prometheus/metrics/memory.pyrobusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.pyrobusta_krr/core/models/config.pyrobusta_krr/core/models/metric_dialects.pyrobusta_krr/main.pytests/test_metric_dialects.py
🚧 Files skipped from review as they are similar to previous changes (4)
- robusta_krr/core/integrations/prometheus/metrics/memory.py
- robusta_krr/core/integrations/prometheus/metrics/base.py
- robusta_krr/core/models/config.py
- robusta_krr/core/models/metric_dialects.py
KRR hardcodes kube-state-metrics / cAdvisor / node-exporter metric names.
Clusters whose Kubernetes state metrics come from an OpenTelemetry Collector
(k8s_cluster + hostmetrics receivers) expose the same information under
different names, so KRR finds nothing there.
Introduce a metric dialect for Kubernetes *state* metrics, selected with
`--prometheus-metrics-dialect {auto,kube-state-metrics,otel}`:
| KRR needs | kube-state-metrics | otel |
|-----------------|--------------------------------------------------|------------------------------------|
| node memory | kube_node_status_capacity{resource="memory"} | system_memory_limit_bytes |
| node CPU | kube_node_status_capacity{resource="cpu"} | system_cpu_logical_count |
| memory requests | kube_pod_container_resource_requests | k8s_container_memory_request_bytes |
| CPU requests | kube_pod_container_resource_requests | k8s_container_cpu_request |
| memory limits | kube_pod_container_resource_limits | k8s_container_memory_limit_bytes |
| pod phase | kube_pod_status_phase{phase="Running"} == 1 | k8s_pod_phase == 2 (phase enum) |
| OOMKills | kube_pod_container_status_last_terminated_reason | k8s_container_status_reason |
Container usage metrics (container_cpu_usage_seconds_total,
container_memory_working_set_bytes) are deliberately left out of the dialect:
they keep their cAdvisor names in every setup, so an OpenTelemetry-only
deployment still has to scrape cAdvisor (e.g. via the Collector's prometheus
receiver). This keeps the change to state metrics only.
The otel dialect requires the Collector to map the Kubernetes resource
attributes onto the namespace / pod / container labels, because that is how the
state metrics are joined with the cAdvisor usage metrics; a pipeline exporting
them verbatim as k8s_namespace_name etc. cannot work. Dialect detection probes
for the mapped labels, not only for the metric name, and logs which mapping is
missing instead of accepting such a pipeline silently. The README carries the
transform processor that does the mapping.
The node capacity metrics also change on the kube-state-metrics side, which
fixes robusta-dev#420: machine_memory_bytes and machine_cpu_cores are deprecated and are
missing on Azure Managed Prometheus, so the cluster summary now reads
kube_node_status_capacity, grouped by `node` instead of by `instance`.
Pod owner resolution is a separate axis, because the k8s_cluster receiver has
no equivalent of the kube_*_owner metrics. Added
`--prometheus-owner-resolution {auto,kube-state-metrics,recording-rule}`:
- kube-state-metrics: the existing kube_replicaset_owner /
kube_replicationcontroller_owner / kube_job_owner / kube_pod_owner chain,
including the per-kind handling of Rollout, DeploymentConfig, CronJob and
GroupedJob and the KRR_OWNER_BATCH_SIZE batching.
- recording-rule: one query against a pod owner recording rule
(namespace_workload_pod:kube_pod_owner:relabel by default, configurable via
--prometheus-workload-recording-rule). Saves a query per workload on large
clusters. It only covers Deployment, DaemonSet, StatefulSet, Job and
ReplicaSet, so other kinds fall back to the owner chain instead of silently
returning no pods.
Both `auto` modes probe kube-state-metrics first and are cached per service, so
an existing kube-state-metrics installation keeps sending the queries it sent
before, at the cost of one extra probe per cluster. Probes look back one hour
(KRR_DIALECT_PROBE_WINDOW) rather than reading an instant vector, so a scrape
gap cannot silently switch the dialect for a whole scan. When no dialect is
detected, KRR warns and falls back to kube-state-metrics.
Also stop reading the metric loader in the no-data warning of gather_data: the
loader is unbound when its constructor raised, which turned that path into a
NameError.
The README now lists every metric KRR queries and which component exposes it,
which covers robusta-dev#354, plus a table mapping each one to its OpenTelemetry name.
tests/test_metric_dialects.py pins the generated PromQL for both dialects,
using the pre-existing kube-state-metrics queries as golden values, and covers
detection precedence, the unmapped-label rejection, probe windowing, caching,
and the owner resolution fallbacks. Apart from the node capacity metrics, the
only change to a kube-state-metrics query is the quote style in the kube-system
request queries ('kube-system' -> "kube-system").
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6c27c64 to
a63a9ae
Compare
|
Both of the latest two findings accepted, pushed as Warn when neither owner source has data — correct, and it was asymmetric with
77 tests pass, black/flake8 clean under the repo configs. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (1)
221-223: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStrip only the leading separator.
replace(",", "")removes every comma in the string. The intent is to drop the leading,thatget_prometheus_cluster_labeladds. A cluster label value that contains a comma produces a corrupted matcher.♻️ Proposed change
def _single_cluster_label(self) -> str: # use this for queries with no labels. turn ', cluster="xxx"' to 'cluster="xxx"' - return self.get_prometheus_cluster_label().replace(",", "") + return self.get_prometheus_cluster_label().lstrip(", ")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py` around lines 221 - 223, Update _single_cluster_label to remove only the leading separator added by get_prometheus_cluster_label, rather than deleting every comma; preserve commas within the cluster label value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 162-168: Update the README node metrics documentation to clarify
how host_name is produced for the otel dialect: document the host_metrics,
resourcedetection, and Prometheus exporter mapping from host.name, or change the
table’s grouping label to the node label created by the shown resource mapping.
Ensure the documented label matches the actual configuration path.
In `@robusta_krr/main.py`:
- Around line 159-166: Update the help text for the prometheus_owner_resolution
option to state that auto prefers the owner chain, falls back to the recording
rule when that rule has data, and otherwise uses the kube-state-metrics
strategy, matching _detect_owner_resolution and the README wording.
In `@tests/test_metric_dialects.py`:
- Line 86: Align the documented Python version requirements with the runtime
constraint in pyproject.toml: update the README and any other Python 3.9 claims
to state Python 3.10 or greater, preserving the existing upper bound if
documented. Do not add py39 support unless the project is intentionally changing
its declared dependency constraints.
---
Nitpick comments:
In
`@robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py`:
- Around line 221-223: Update _single_cluster_label to remove only the leading
separator added by get_prometheus_cluster_label, rather than deleting every
comma; preserve commas within the cluster label value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f81a3429-1922-48f2-863e-bac390a46ab6
📒 Files selected for processing (8)
README.mdrobusta_krr/core/integrations/prometheus/metrics/base.pyrobusta_krr/core/integrations/prometheus/metrics/memory.pyrobusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.pyrobusta_krr/core/models/config.pyrobusta_krr/core/models/metric_dialects.pyrobusta_krr/main.pytests/test_metric_dialects.py
🚧 Files skipped from review as they are similar to previous changes (4)
- robusta_krr/core/integrations/prometheus/metrics/memory.py
- robusta_krr/core/integrations/prometheus/metrics/base.py
- robusta_krr/core/models/metric_dialects.py
- robusta_krr/core/models/config.py
| - set(attributes["node"], resource.attributes["k8s.node.name"]) where resource.attributes["k8s.node.name"] != "" | ||
| ``` | ||
|
|
||
| | What KRR needs | kube-state-metrics dialect | `otel` dialect | OpenTelemetry source | | ||
| | --- | --- | --- | --- | | ||
| | node memory | `kube_node_status_capacity{resource="memory"}` (by `node`) | `system_memory_limit_bytes` (by `host_name`) | `hostmetrics` receiver, `memory` scraper with `system.memory.limit` enabled | | ||
| | node CPU | `kube_node_status_capacity{resource="cpu"}` (by `node`) | `system_cpu_logical_count` (by `host_name`) | `hostmetrics` receiver, `cpu` scraper with `system.cpu.logical.count` enabled | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
OpenTelemetry Collector prometheus exporter resource_to_telemetry_conversion host_name label hostmetrics
💡 Result:
In the OpenTelemetry Collector, the Prometheus exporter handles resource attributes differently depending on your configuration [1][2]. By default, resource attributes are not attached to every metric as labels; instead, they are associated with a special metric called target_info [1][3]. To perform queries that filter or group by these attributes, you must perform a PromQL join operation on this target_info metric [1][3]. If you want resource attributes (like host.name) to appear directly as labels on your metrics, you have two primary options: 1. Enable resource_to_telemetry_conversion: You can set resource_to_telemetry_conversion.enabled: true in the prometheus exporter configuration [1][2]. When enabled, all resource attributes are automatically converted to metric labels [1][2]. Note that this can significantly increase label cardinality, which may lead to performance issues in your Prometheus instance [3][4]. 2. Use the Transform Processor (Recommended): To avoid high cardinality, it is generally recommended to use the transform processor to selectively copy only the specific resource attributes you need into the datapoint attributes [1][3][4]. The prometheus exporter will automatically convert these datapoint attributes into Prometheus labels [5][4]. Example configuration for the transform processor: processors: transform: metric_statements: - context: datapoint statements: - set(attributes["host_name"], resource.attributes["host.name"]) Regarding hostmetrics: The hostmetrics receiver does not automatically attach host-related resource attributes (like host.name) to the metrics it collects [6][7]. To ensure these attributes are present on your metrics, you should use the resourcedetection processor in your pipeline [7][8]. This processor detects system metadata and attaches the appropriate resource attributes to the telemetry data before it reaches the exporter [8]. Summary of steps to get host.name as a label: - Ensure the resourcedetection processor is included in your pipeline to populate the host.name resource attribute [8][9]. - Use the transform processor (as shown above) to copy host.name from the resource level to the datapoint attribute level [1][3]. - The prometheus exporter will then correctly translate this into a host_name label on the metric [5][4].
Citations:
- 1: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.153.0/exporter/prometheusexporter/README.md
- 2: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/prometheusexporter/README.md
- 3: https://github.com/ollygarden/opentelemetry-agent-skills/blob/main/skills/otel-collector/components/prometheus_exporter/advanced.md
- 4: https://grafana.com/docs/alloy/next/reference/components/otelcol/otelcol.exporter.prometheus
- 5: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/translator/prometheus
- 6: Not able to get resource metrics as labels in prometheus exporter open-telemetry/opentelemetry-collector-contrib#5075
- 7: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/refs/heads/main/receiver/hostmetricsreceiver/README.md
- 8: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/resourcedetectionprocessor/README.md
- 9: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/hostmetricsreceiver/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files named README.md:\n'
fd -a '^README\.md$' . | sed 's#^\./##' | sort | head -20
printf '\nTarget README section:\n'
if [ -f README.md ]; then
sed -n '130,185p' README.md | nl -ba -v130
fi
printf '\nSearch for host_name / node mappings and otel examples:\n'
rg -n 'host_name|resource_to_telemetry_conversion|set\(attributes\["node"\]|system_cpu_logical_count|system_memory_limit_bytes|container_memory_limit' README.md . --glob '!vendor/**' --glob '!node_modules/**' 2>/dev/null || true
printf '\nRelevant diff summary:\n'
git diff -- README.md | sed -n '1,220p' || trueRepository: robusta-dev/krr
Length of output: 343
🏁 Script executed:
#!/bin/bash
set -u
printf 'README section with line numbers:\n'
awk 'NR>=140 && NR<=175 { printf "%d: %s\n", NR, $0 }' README.md
printf '\nSearch for host_name / node mappings and otel exports:\n'
grep -RnI 'host_name\|resource_to_telemetry_conversion\|set(attributes\["node"\]\|system_cpu_logical_count\|system_memory_limit_bytes\|container_memory_limit' README.md docker README.md docker enforcer 2>/dev/null | head -200 || true
printf '\nRelevant README diff summary:\n'
git diff -- README.md | sed -n '1,260p' || trueRepository: robusta-dev/krr
Length of output: 5672
🌐 Web query:
OpenTelemetry Collector hostmetrics receiver host.name resource attribute host_name label prometheus exporter
💡 Result:
The hostmetrics receiver in the OpenTelemetry Collector does not automatically add resource attributes like host.name to the metrics it generates [1][2][3]. These host-related attributes are considered identity information, which is handled separately by the resourcedetection processor [2]. To include host.name in your pipeline and subsequently as a label in Prometheus, you should follow these steps: 1. Add the resourcedetection processor to your pipeline and configure it to use the system detector. This detector will automatically discover and attach host.name and os.type as resource attributes [2][4]. 2. Configure the prometheus exporter to promote these resource attributes into labels for each metric. By default, OTLP resource attributes are not included as metric labels in the exported Prometheus format. You must enable resource_to_telemetry_conversion in your exporter configuration [5]. Example configuration: processors: resourcedetection: detectors: ["system"] exporters: prometheus: resource_to_telemetry_conversion: enabled: true service: pipelines: metrics: receivers: [hostmetrics] processors: [resourcedetection] exporters: [prometheus] When resource_to_telemetry_conversion is enabled, the prometheus exporter will convert resource attributes into metric labels [3][5]. The OpenTelemetry attribute host.name will typically be exported as the label host_name in Prometheus, following standard translation conventions where dots are replaced by underscores [3].
Citations:
- 1: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/refs/heads/main/receiver/hostmetricsreceiver/README.md
- 2: https://hyperping.com/blog/opentelemetry-host-metrics-explained
- 3: https://www.dash0.com/guides/opentelemetry-host-metrics
- 4: https://github.com/Dynatrace/dynatrace-otel-collector/blob/main/config_examples/host-metrics.yaml
- 5: Gathered hostmetrics process shown in console but not as metric in prometheus open-telemetry/opentelemetry-collector-contrib#36496
Document how host_name reaches Prometheus, and clarify the node mapping.
The otel node rows group by host_name, but the example only maps resource.attributes["k8s.node.name"] into a node attribute. Show the host_metrics/resourcedetection/Prometheus exporter path that turns host.name into host_name, or replace the table value with the label produced by the shown mapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 162 - 168, Update the README node metrics
documentation to clarify how host_name is produced for the otel dialect:
document the host_metrics, resourcedetection, and Prometheus exporter mapping
from host.name, or change the table’s grouping label to the node label created
by the shown resource mapping. Ensure the documented label matches the actual
configuration path.
| prometheus_owner_resolution: OwnerResolutionName = typer.Option( | ||
| OwnerResolutionName.AUTO.value, | ||
| "--prometheus-owner-resolution", | ||
| help="How to find the pods of a workload: 'kube-state-metrics' walks the kube_*_owner chain, " | ||
| "'recording-rule' uses a single pod owner recording rule, 'auto' prefers the owner chain and " | ||
| "falls back to the recording rule.", | ||
| rich_help_panel="Prometheus Settings", | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete the auto description.
The help text says auto "prefers the owner chain and falls back to the recording rule". _detect_owner_resolution uses the recording rule only when the rule itself has data, and returns the kube-state-metrics strategy when neither source has data. README Line 181 already states this. Align the help text.
🛠️ Proposed change
help="How to find the pods of a workload: 'kube-state-metrics' walks the kube_*_owner chain, "
"'recording-rule' uses a single pod owner recording rule, 'auto' prefers the owner chain and "
- "falls back to the recording rule.",
+ "falls back to the recording rule when only the rule has data.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prometheus_owner_resolution: OwnerResolutionName = typer.Option( | |
| OwnerResolutionName.AUTO.value, | |
| "--prometheus-owner-resolution", | |
| help="How to find the pods of a workload: 'kube-state-metrics' walks the kube_*_owner chain, " | |
| "'recording-rule' uses a single pod owner recording rule, 'auto' prefers the owner chain and " | |
| "falls back to the recording rule.", | |
| rich_help_panel="Prometheus Settings", | |
| ), | |
| prometheus_owner_resolution: OwnerResolutionName = typer.Option( | |
| OwnerResolutionName.AUTO.value, | |
| "--prometheus-owner-resolution", | |
| help="How to find the pods of a workload: 'kube-state-metrics' walks the kube_*_owner chain, " | |
| "'recording-rule' uses a single pod owner recording rule, 'auto' prefers the owner chain and " | |
| "falls back to the recording rule when only the rule has data.", | |
| rich_help_panel="Prometheus Settings", | |
| ), |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 159-166: Do not perform function call typer.Option in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@robusta_krr/main.py` around lines 159 - 166, Update the help text for the
prometheus_owner_resolution option to state that auto prefers the owner chain,
falls back to the recording rule when that rule has data, and otherwise uses the
kube-state-metrics strategy, matching _detect_owner_resolution and the README
wording.
| it without the selector. | ||
| """ | ||
|
|
||
| def __init__(self, *, existing_metrics: tuple[str, ...] = (), responses: list | None = None) -> None: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Declared Python floor for the project.
fd -H -t f -d 2 'pyproject.toml|setup.py|setup.cfg|.python-version|.tool-versions|tox.ini' -x cat -n {}
# CI matrix.
fd -H -t f -e yml -e yaml . .github/workflows -x rg -n 'python-version' {}
# Other runtime-evaluated PEP 604 annotations in the repo.
rg -nP --type=py '^\s*(async\s+)?def .*:\s*[\w\[\], .]+\s*\|\s*None' -g '!**/.venv/**'Repository: robusta-dev/krr
Length of output: 2307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning Python versions =="
rg -n "Python|python|>=3|3\.9|3\.10|3\.11|3\.12|py39|py310|python-version" -S README.md pyproject.toml 2>/dev/null || true
echo
echo "== pytest_metric_dialects relevant section =="
cat -n tests/test_metric_dialects.py | sed -n '1,140p'
echo
echo "== all runtime-evaluated PEP 604 annotations =="
python3 - <<'PY'
import ast, pathlib
for path in pathlib.Path('.').rglob('*.py'):
if any(part in {'.git', '.venv', '__pycache__'} for part in path.parts):
continue
try:
text = path.read_text()
except UnicodeDecodeError:
continue
try:
tree = ast.parse(text)
except SyntaxError:
continue
for node in ast.walk(tree):
if isinstance(node, ast.BinOp) and isinstance(node.op, ast.BitOr):
if (ast.get_source_segment(text, node) or '').count('|') > 0 and ast.getdoc(node):
continue
print(path, getattr(node, 'lineno', '?'), ast.get_source_segment(text, node))
PYRepository: robusta-dev/krr
Length of output: 8123
Update the README Python floor.
pyproject.toml declares runtime support as python = ">=3.10,<=3.12.9", while the README says “Python 3.9 or greater.” If 3.10 is required, update the README and any 3.9 claims; otherwise add py39 support to the dependency constraints.
[maintainability_and_codec_quality]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_metric_dialects.py` at line 86, Align the documented Python
version requirements with the runtime constraint in pyproject.toml: update the
README and any other Python 3.9 claims to state Python 3.10 or greater,
preserving the existing upper bound if documented. Do not add py39 support
unless the project is intentionally changing its declared dependency
constraints.
2 participants
Problem
KRR hardcodes kube-state-metrics / cAdvisor / node-exporter metric names. Clusters whose Kubernetes state metrics come from an OpenTelemetry Collector (
k8s_cluster+hostmetricsreceivers) expose the same information under different names, so KRR finds no pods, no node capacity and no OOMKills there. Disabling kube-state-metrics is common in OTel-based observability stacks.What this adds
1. A metric dialect for Kubernetes state metrics —
--prometheus-metrics-dialect {auto,kube-state-metrics,otel}kube-state-metricsotelmachine_memory_bytes(byinstance)system_memory_limit_bytes(byhost_name)system.memory.limitmachine_cpu_cores(byinstance)system_cpu_logical_count(byhost_name)system.cpu.logical.countkube_pod_container_resource_requests{resource="memory"}k8s_container_memory_request_byteskube_pod_container_resource_requests{resource="cpu"}k8s_container_cpu_requestkube_pod_container_resource_limits{resource="memory"}k8s_container_memory_limit_byteskube_pod_status_phase{phase="Running"} == 1k8s_pod_phase == 2(phase enum)kube_pod_container_status_last_terminated_reason{reason="OOMKilled"}k8s_container_status_reason{k8s_container_status_reason="OOMKilled"}k8s.container.status.reason(off by default)Container usage metrics (
container_cpu_usage_seconds_total,container_memory_working_set_bytes) are deliberately not part of the dialect — they keep their cAdvisor names in every setup, so an OTel-only deployment still scrapes cAdvisor (e.g. with the Collector'sprometheusreceiver). This keeps the change scoped to state metrics.2. Pod owner resolution as a separate axis —
--prometheus-owner-resolution {auto,kube-state-metrics,recording-rule}The
k8s_clusterreceiver has no equivalent of thekube_*_ownermetrics, so this cannot be expressed as a name swap.kube-state-metrics— the existing owner chain, unchanged, including the per-kind handling of Rollout / DeploymentConfig / CronJob / GroupedJob andKRR_OWNER_BATCH_SIZEbatching.recording-rule— one query against a pod owner recording rule (namespace_workload_pod:kube_pod_owner:relabelby default, override with--prometheus-workload-recording-rule). Also saves a query per workload on large clusters. It only covers Deployment, DaemonSet, StatefulSet, Job and ReplicaSet, so other kinds fall back to the owner chain rather than silently returning no pods.Backwards compatibility
Both
automodes probe kube-state-metrics first and cache the result per service, so an existing installation keeps sending exactly the queries it sent before, at the cost of onecount()probe per cluster. If nothing is detected, KRR logs a warning and falls back to kube-state-metrics. The only intentional change to a kube-state-metrics query is the quote style in the kube-system request queries ('kube-system'→"kube-system").Tests
tests/test_metric_dialects.py(18 tests) pins the generated PromQL for both dialects, using the pre-existing kube-state-metrics queries as golden values, and covers detection precedence, caching, explicit selection, and the owner-resolution fallbacks. Full suite: 73 passed.🤖 Generated with Claude Code