feat: add kpi that tracks CR count per configured cluster - #1054
Merged
Conversation
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 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 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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.
![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
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 (2)
internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go (2)
143-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject GVK values with an additional separator.
strings.SplitNacceptsapps/v1/DeploymentList/extraand storesDeploymentList/extraas the kind. Reject this value duringInitwith the other malformed GVK values.Proposed fix
- parts := strings.SplitN(s, "/", 3) - if len(parts) != 3 { + parts := strings.Split(s, "/") + if len(parts) != 3 || parts[1] == "" || parts[2] == "" { return schema.GroupVersionKind{}, fmt.Errorf("expected group/version/Kind, got: %s", s) } - if parts[1] == "" || parts[2] == "" { - return schema.GroupVersionKind{}, fmt.Errorf("expected group/version/Kind, got: %s", s) - }🤖 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 143 - 149, Update the GVK parsing logic in Init around strings.SplitN so values containing an additional "/" after the kind are rejected as malformed. Validate the original input or parsed kind for extra separators while preserving the existing validation and error behavior for missing group, version, or kind.
125-136: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefine duplicate routing-label handling.
ConfiguredRouteLabelsexposes every configured label set, including duplicate sets for the same GVK, andAddRemoteonly logs label duplication.ListMetadataPerCluster()then emits each remote cluster separately using those same routing labels, so identical label sets create overlapping Prometheus series for the samemulticluster_object_count_kpi. Reject duplicate GVK-specific label sets during configuration, or add a stable cluster identity label that cannot be duplicated.🤖 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 125 - 136, Prevent duplicate GVK-specific routing-label sets from producing overlapping metrics in the multicluster_object_count_kpi. Update the configuration/registration flow around ConfiguredRouteLabels and AddRemote to reject duplicate label sets for the same GVK rather than only logging them, and ensure ListMetadataPerCluster emits metrics only for validated, uniquely identified configurations.
🧹 Nitpick comments (1)
internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi_test.go (1)
65-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the production schema-construction path.
initWithReaderduplicates the label-union and descriptor logic fromInit. The union test can pass ifInitregresses. Extract the shared schema construction into a helper used by bothInitand this test, with route labels keyed by GVK.🤖 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_test.go` around lines 65 - 93, Extract the label-union and Prometheus descriptor construction from production Init into a shared schema-construction helper, using route labels keyed by GVK. Update Init and initWithReader to call this helper so the test exercises the production schema path while retaining the existing GVK descriptor setup.
🤖 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 78-98: Validate routing label keys after toSnakeCase in the Init
logic before constructing the prometheus descriptor, rejecting any key that
collides with fixed labels group, version, kind, or is_home, or with another
normalized routing key. Ensure the KPI initialization fails rather than creating
sharedDesc when such a collision is detected.
---
Outside diff comments:
In `@internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi.go`:
- Around line 143-149: Update the GVK parsing logic in Init around
strings.SplitN so values containing an additional "/" after the kind are
rejected as malformed. Validate the original input or parsed kind for extra
separators while preserving the existing validation and error behavior for
missing group, version, or kind.
- Around line 125-136: Prevent duplicate GVK-specific routing-label sets from
producing overlapping metrics in the multicluster_object_count_kpi. Update the
configuration/registration flow around ConfiguredRouteLabels and AddRemote to
reject duplicate label sets for the same GVK rather than only logging them, and
ensure ListMetadataPerCluster emits metrics only for validated, uniquely
identified configurations.
---
Nitpick comments:
In
`@internal/knowledge/kpis/plugins/deployment/multicluster_object_count_kpi_test.go`:
- Around line 65-93: Extract the label-union and Prometheus descriptor
construction from production Init into a shared schema-construction helper,
using route labels keyed by GVK. Update Init and initWithReader to call this
helper so the test exercises the production schema path while retaining the
existing GVK descriptor setup.
🪄 Autofix
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: a70b80eb-d647-4066-93e1-d4dce23c33f5
📒 Files selected for processing (4)
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.gopkg/multicluster/client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- helm/bundles/cortex-nova/templates/kpis_kvm.yaml
- pkg/multicluster/client.go
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
Contributor
Test Coverage ReportTest Coverage 📊: 70.7% |
PhilippMatthes
approved these changes
Aug 5, 2026
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