Custom Kube State Metric: Component Level Metric - #13554
Draft
martysp21 wants to merge 1 commit into
Draft
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: martysp21 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 |
Contributor
Kustomize Render DiffComparing
Total: 2 components, +58 -10 lines 📋 Full diff available in the workflow summary and as a downloadable artifact. |
PR Summary by QodoExpose component-level PipelineRun metrics via custom kube-state-metrics
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Duplicate series labelsets
|
![qodo-for-redhat-appstudio[bot]](https://avatars.githubusercontent.com/u/89927105?s=60&v=4){kind=small}
Comment on lines
+82
to
+85
| labelsFromPath: | ||
| application: [metadata, labels, appstudio.openshift.io/application] | ||
| component: [metadata, labels, appstudio.openshift.io/component] | ||
| pipeline_type: [metadata, labels, pipelines.appstudio.openshift.io/type] |
There was a problem hiding this comment.
1. Duplicate series labelsets 🐞 Bug ≡ Correctness
konflux_pipelinerun_info is emitted per PipelineRun but omits any unique per-run label, so multiple PipelineRuns with the same application/component/type can produce identical label sets and collide as the same Prometheus series. It also exports metadata.generation (not an info-style constant), contradicting the help text and making sum()-based usage incorrect.
Agent Prompt
## Issue description
`konflux_pipelinerun_info` is defined under a `PipelineRun` resource with an `each` stanza (per object), but its labels do not include a unique identifier (e.g., `metadata.name` or `metadata.uid`). That means multiple PipelineRuns can map to the exact same `{application,component,pipeline_type,event_type}` labelset.
This can cause collisions/duplicate-series in the scrape and makes the exported value misleading because it uses `metadata.generation` while the help text claims an info-style `~1` gauge.
## Issue Context
The config template in this repo explicitly models metrics as per-object via `each:`. For a “component-level” view, it’s safer to export a uniquely-keyed per-PipelineRun series and do component grouping in PromQL/recording rules, rather than trying to collapse uniqueness at exposition time.
## Fix Focus Areas
- components/monitoring/custom-kube-state-metrics/staging/custom-resource-state-config.yaml[80-92]
### Suggested change
- Add a unique label to `konflux_pipelinerun_info`, e.g.:
- `pipelinerun: [metadata, name]` (or `pipelinerun_uid: [metadata, uid]`) to guarantee uniqueness.
- Make the value truly “info-style” by switching to `each.type: Info` if supported by your kube-state-metrics deployment, or adjust the help text to match the actual semantics (and avoid describing it as `~1` if you keep `generation`).
- Document/expect usage via `count by (application, component, pipeline_type, event_type) (konflux_pipelinerun_info)` (or a recording rule) for the component-level rollup.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+85
to
+86
| pipeline_type: [metadata, labels, pipelines.appstudio.openshift.io/type] | ||
| event_type: [metadata, labels, pipelinesascode.tekton.dev/event-type] |
There was a problem hiding this comment.
2. Pipeline_type label dropped 🐞 Bug ◔ Observability
The new konflux_pipelinerun_info adds a pipeline_type label, but the staging remote-write LabelKeep allowlist regex does not include pipeline_type, so it will be stripped during remote-write/federation. This breaks the metric’s intended grouping dimension and can collapse distinct series downstream.
Agent Prompt
## Issue description
`konflux_pipelinerun_info` introduces a new Prometheus label named `pipeline_type`, but the staging federation/remote-write LabelKeep allowlist doesn’t include `pipeline_type`. As a result, remote-written data will lose this dimension.
## Issue Context
The allowlist is implemented as a `LabelKeep` regex; any label name not matched is dropped. The allowlist already keeps `event_type` but not `pipeline_type`.
## Fix Focus Areas
- components/monitoring/prometheus/staging/base/federation/writeRelabelConfigs.yaml[31-51]
### Suggested change
- Add `pipeline_type` to the allowlist regex (and do the same in production if this metric will be promoted), or rename the emitted label to an already-allowlisted label name if that’s the established convention.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2
to
+4
| - op: add | ||
| path: /spec/endpoints/0/metricRelabelings | ||
| value: |
There was a problem hiding this comment.
3. Endpoint index patch fragile 🐞 Bug ⚙ Maintainability
The ServiceMonitor patch targets /spec/endpoints/0, so a future reorder/insert of endpoints in the base ServiceMonitor can silently apply these relabelings to the wrong endpoint. This is a maintainability reliability risk because the patch encodes array position rather than asserting/matching the intended endpoint (port: metrics).
Agent Prompt
## Issue description
The JSON6902 patch hard-codes `/spec/endpoints/0`, which assumes the `metrics` endpoint remains at index 0 forever. If the base ServiceMonitor changes endpoint ordering, the patch may apply to the wrong endpoint.
## Issue Context
The base ServiceMonitor currently defines two endpoints (`metrics` then `telemetry`). The patch should at least fail fast if that ordering changes, or be rewritten to target the endpoint more robustly.
## Fix Focus Areas
- components/monitoring/custom-kube-state-metrics/staging/servicemonitor-patch.yaml[1-12]
- components/monitoring/custom-kube-state-metrics/base/servicemonitor.yaml[10-22]
### Suggested change
- Add a `test` op before the `add` op to assert you are patching the intended endpoint, e.g.:
- `op: test`, `path: /spec/endpoints/0/port`, `value: metrics`
- (Optional) Consider a patch approach that matches on the endpoint identity instead of array index if feasible in your kustomize setup.
Note: a `test` op won’t make the patch resilient to reordering, but it will prevent silent misapplication by failing the build/apply when the base changes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13554 +/- ##
=======================================
Coverage 60.38% 60.38%
=======================================
Files 24 24
Lines 1628 1628
=======================================
Hits 983 983
Misses 563 563
Partials 82 82
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
martysp21
marked this pull request as draft
August 17, 2026 12:19
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
1 participant
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.
Add a new metric exposing component level metrics.
This is untracked POC only.