feat: add kpi that tracks CR count per configured cluster#1054
Open
SoWieMarkus wants to merge 6 commits into
Open
feat: add kpi that tracks CR count per configured cluster#1054SoWieMarkus wants to merge 6 commits into
SoWieMarkus wants to merge 6 commits into
Conversation
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
📝 WalkthroughWalkthroughAdds a multicluster metadata listing API and a Prometheus KPI that counts configured Kubernetes objects per home or remote cluster, with routing labels and Helm configuration for four Nova GVKs. ChangesMulticluster object count
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MulticlusterObjectCountKPI
participant multicluster.Client
participant ClusterAPI
participant Prometheus
MulticlusterObjectCountKPI->>multicluster.Client: ListMetadataPerCluster(GVK)
multicluster.Client->>ClusterAPI: List partial object metadata
ClusterAPI-->>multicluster.Client: Per-cluster metadata items
multicluster.Client-->>MulticlusterObjectCountKPI: ClusterObjectMetadata
MulticlusterObjectCountKPI->>Prometheus: Emit object-count gauge
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
SoWieMarkus
force-pushed
the
feat/cr-count
branch
from
July 16, 2026 08:19
32bc4b0 to
adeea16
Compare
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
… label mapping Signed-off-by: Markus Wieland <markus.wieland@sap.com>
SoWieMarkus
marked this pull request as ready for review
July 16, 2026 08:40
SoWieMarkus
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
July 16, 2026 08:40
PhilippMatthes
left a comment
PhilippMatthes
left a comment
Member
There was a problem hiding this comment.
Looks good! Just some minor comments.
Contributor
Test Coverage ReportTest Coverage 📊: 70.3% |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/multicluster/client.go (1)
476-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a lowercase error string.
Line 476 starts with
GVK; change it togvkto satisfy linting. As per coding guidelines, “Error messages should always be lowercase to conform to linting rules.”🤖 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 `@pkg/multicluster/client.go` at line 476, Update the error message returned by the GVK lookup path to begin with lowercase “gvk” instead of “GVK”. Preserve the existing formatting and error behavior.Source: Coding guidelines
🤖 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 `@internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go`:
- Around line 43-44: Update the MulticlusterObjectCountKPI description in
internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go
lines 43-44 to mention counts for configured home and remote clusters. Update
the corresponding description in
helm/bundles/cortex-nova/templates/kpis_kvm.yaml lines 59-61 to state that
home-cluster metrics use is_home=true with empty routing-label values.
- Around line 138-142: Update the GVK parsing logic around strings.SplitN to
reject empty version or kind segments during Init, while preserving the existing
group/version/kind format validation and error behavior. Validate parts[1] and
parts[2] before constructing schema.GroupVersionKind so inputs such as
apps//DeploymentList and /v1/ fail immediately.
- Around line 68-95: The metric family currently creates descriptors with
different routing-label schemas per GVK. Refactor descriptor construction to
first compute one stable union of snake_case routing-label keys across all
configured GVKs, then use that shared schema for every
cortex_multicluster_object_count descriptor and emit empty values when a cluster
lacks a key. Add a registry test covering two GVKs with differing routing-label
sets and verifying collection succeeds.
---
Nitpick comments:
In `@pkg/multicluster/client.go`:
- Line 476: Update the error message returned by the GVK lookup path to begin
with lowercase “gvk” instead of “GVK”. Preserve the existing formatting and
error behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18689abe-4edd-4348-8fe0-576a97bc2046
📒 Files selected for processing (6)
helm/bundles/cortex-nova/templates/kpis_kvm.yamlinternal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.gointernal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi_test.gointernal/knowledge/kpis/supported_kpis.gopkg/multicluster/client.gopkg/multicluster/client_test.go
Comment on lines
+43
to
+44
| // MulticlusterObjectCountKPI reports the number of objects of each configured | ||
| // GVK per remote cluster, labelled by the cluster's routing labels. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document configured home and remote clusters consistently. Collect emits home-cluster metrics with is_home=true, so both descriptions should include that behavior.
internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go#L43-L44: describe counts for configured home and remote clusters.helm/bundles/cortex-nova/templates/kpis_kvm.yaml#L59-L61: state that home-cluster metrics are emitted withis_home=trueand empty routing-label values.
📍 Affects 2 files
internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go#L43-L44(this comment)helm/bundles/cortex-nova/templates/kpis_kvm.yaml#L59-L61
🤖 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 `@internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go`
around lines 43 - 44, Update the MulticlusterObjectCountKPI description in
internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go
lines 43-44 to mention counts for configured home and remote clusters. Update
the corresponding description in
helm/bundles/cortex-nova/templates/kpis_kvm.yaml lines 59-61 to state that
home-cluster metrics use is_home=true with empty routing-label values.
Comment on lines
+68
to
+95
| routeLabels := mcl.ConfiguredRouteLabels(gvk) | ||
| // Each remote cluster is registered with a routing label map (e.g. | ||
| // {"availabilityZone": "eu-de-1a"}). We collect the union of all key names | ||
| // across every remote cluster and snake_case them to produce Prometheus label | ||
| // names (availabilityZone → availability_zone). These become variable labels | ||
| // on the descriptor so each cluster gets its own time series. | ||
| keySet := map[string]bool{} | ||
| var labelKeys []string | ||
| for _, lm := range routeLabels { | ||
| for k := range lm { | ||
| snake := toSnakeCase(k) | ||
| if !keySet[snake] { | ||
| keySet[snake] = true | ||
| labelKeys = append(labelKeys, snake) | ||
| } | ||
| } | ||
| } | ||
| // Fixed labels: group/version/kind identify the resource type; is_home | ||
| // distinguishes the home cluster (no routing labels) from remote clusters. | ||
| // The routing label keys follow (e.g. availability_zone). | ||
| varLabels := append([]string{"group", "version", "kind", "is_home"}, labelKeys...) | ||
| desc := prometheus.NewDesc( | ||
| "cortex_multicluster_object_count", | ||
| "Number of objects of a given GVK per cluster", | ||
| varLabels, | ||
| nil, | ||
| ) | ||
| k.descs = append(k.descs, gvkDesc{gvk: gvk, labelKeys: labelKeys, desc: desc}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For github.com/prometheus/client_golang v1.23.2, does prometheus.Registry.Register reject descriptors with the same fully qualified metric name but different variable-label names?
💡 Result:
Yes, in prometheus/client_golang v1.23.2, the prometheus.Registry.Register method does reject descriptors that share the same fully-qualified metric name but have different variable-label names (or different help strings) [1]. The Prometheus registry enforces consistency for all metrics sharing the same fully-qualified name [1][2]. Specifically, when registering a collector, the registry performs internal consistency checks: 1. It checks if the metric name has been seen before [1]. 2. If it has, it compares the current descriptor's dimension hash (which represents the set of label names and help string) against the previously registered descriptor's dimension hash [1]. 3. If the dimension hashes do not match, the registry returns an error stating: "a previously registered descriptor with the same fully-qualified name as %s has different label names or a different help string" [1]. This design ensures that all metrics under a single name maintain a consistent structure, which is a fundamental requirement of the Prometheus data model [3][2]. While recent versions of the client library have refined these checks—for instance, by allowing inconsistent label dimensions in specific scenarios or by detecting collisions with "magic" names more effectively—the core requirement for structural consistency (same help string and same label names) for metrics sharing the same fully-qualified name remains enforced [1][4].
Citations:
- 1: https://github.com/prometheus/client_golang/blob/main/prometheus/registry.go
- 2: https://github.com/prometheus/client_golang/blob/master/prometheus/desc.go
- 3: https://github.com/prometheus/client_golang/blob/master/prometheus/doc.go
- 4: https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate target file =="
git ls-files | rg 'internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi\.go$' || true
echo "== outline target file =="
ast-grep outline internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go --view expanded || true
echo "== relevant file sections =="
sed -n '1,180p' internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go | nl
echo "== search metric name and route labels usages =="
rg -n "cortex_multicluster_object_count|ConfiguredRouteLabels|labelKeys|ToMetrics|Collect|Describe" internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go internal/knowledge/kpis/plugins/deployment || true
echo "== module prometheus dependency =="
sed -n '/github.com\/prometheus\/client_golang/p' go.mod go.sum | head -20 || trueRepository: cobaltcore-dev/cortex
Length of output: 1569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant file contents with line numbers =="
awk '{printf "%5d\t%s\n", NR, $0}' internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go
echo "== metric name and label schema usage =="
rg -n "cortex_multicluster_object_count|ConfiguredRouteLabels|labelKeys|ToMetrics|Collect|Describe|register" internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go internal/knowledge/kpis/plugins/deployment || true
echo "== module prometheus dependency =="
if [ -f go.mod ]; then sed -n '/github.com\/prometheus\/client_golang/p' go.mod; fi
if [ -f go.sum ]; then rg 'github.com/prometheus/client_golang ' go.sum | head -20; fi
echo "== repository files under deployment plugins =="
git ls-files 'internal/knowledge/kpis/plugins/deployment/*' | sortRepository: cobaltcore-dev/cortex
Length of output: 17977
Use one label schema for the metric family.
labelKeys is computed per GVK, but every descriptor uses cortex_multicluster_object_count. If configured GVKs expose different routing-label keys, Prometheus rejects the collector because one metric name ends up with incompatible variable labels. Build one stable union of routing-label keys across all configured GVKs, emit empty values where a cluster lacks a key, and add a registry test covering two differing label sets.
🤖 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 `@internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go`
around lines 68 - 95, The metric family currently creates descriptors with
different routing-label schemas per GVK. Refactor descriptor construction to
first compute one stable union of snake_case routing-label keys across all
configured GVKs, then use that shared schema for every
cortex_multicluster_object_count descriptor and emit empty values when a cluster
lacks a key. Add a registry test covering two GVKs with differing routing-label
sets and verifying collection succeeds.
Comment on lines
+138
to
+142
| parts := strings.SplitN(s, "/", 3) | ||
| if len(parts) != 3 { | ||
| return schema.GroupVersionKind{}, fmt.Errorf("expected group/version/Kind, got: %s", s) | ||
| } | ||
| return schema.GroupVersionKind{Group: parts[0], Version: parts[1], Kind: parts[2]}, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject empty GVK segments.
apps//DeploymentList and /v1/ pass the slash-count check, then fail later during collection. Require non-empty version and kind during Init.
Proposed fix
- if len(parts) != 3 {
+ if len(parts) != 3 || parts[1] == "" || parts[2] == "" {📝 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.
Suggested change
| parts := strings.SplitN(s, "/", 3) | |
| if len(parts) != 3 { | |
| return schema.GroupVersionKind{}, fmt.Errorf("expected group/version/Kind, got: %s", s) | |
| } | |
| return schema.GroupVersionKind{Group: parts[0], Version: parts[1], Kind: parts[2]}, nil | |
| parts := strings.SplitN(s, "/", 3) | |
| if len(parts) != 3 || parts[1] == "" || parts[2] == "" { | |
| return schema.GroupVersionKind{}, fmt.Errorf("expected group/version/Kind, got: %s", s) | |
| } | |
| return schema.GroupVersionKind{Group: parts[0], Version: parts[1], Kind: parts[2]}, nil |
🤖 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 `@internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go`
around lines 138 - 142, Update the GVK parsing logic around strings.SplitN to
reject empty version or kind segments during Init, while preserving the existing
group/version/kind format validation and error behavior. Validate parts[1] and
parts[2] before constructing schema.GroupVersionKind so inputs such as
apps//DeploymentList and /v1/ fail immediately.
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
2 participants
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.
CountPerGVKmethod to multicluster client