Add dynamic labels to pipeline step event metrics - #1108
Merged
Conversation
Signed-off-by: Philipp Matthes <p.matthes@sap.com>
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
July 31, 2026 11:48
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds structured pipeline events with dynamic Prometheus labels. Image-property failures include the scheduling intent. The monitor collects these events, and the alert groups failures by intent. ChangesDynamic pipeline event observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FilterImageProperties
participant FilterWeigherPipelineStepResult
participant pipelineStepEventCollector
participant Prometheus
FilterImageProperties->>FilterWeigherPipelineStepResult: append event with intent label
FilterWeigherPipelineStepResult->>pipelineStepEventCollector: record event
pipelineStepEventCollector->>Prometheus: emit labeled counter
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
internal/scheduling/lib/filter_weigher_pipeline_monitor.go (1)
142-153: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a nil guard for
stepEventCollectorbefore callingCollect.
Collectcallsm.stepEventCollector.Collect(ch)without checking fornil.FilterWeigherPipelineStepMonitor.RunWrappedguards the same field withif s.stepEventCollector != nilbefore use. If aFilterWeigherPipelineMonitoris ever constructed without going throughNewPipelineMonitor()(for example, in a test or a future refactor),stepEventCollectorstaysnil, and callingCollecton it panics onLock(), sincesync.Mutexis the first embedded field.Match the guard already used in
RunWrappedfor consistency and defensive safety.🛡️ Proposed guard
m.requestCounter.Collect(ch) - m.stepEventCollector.Collect(ch) + if m.stepEventCollector != nil { + m.stepEventCollector.Collect(ch) + } }🤖 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/scheduling/lib/filter_weigher_pipeline_monitor.go` around lines 142 - 153, Update FilterWeigherPipelineMonitor.Collect to call stepEventCollector.Collect(ch) only when m.stepEventCollector is non-nil, matching the existing guard in FilterWeigherPipelineStepMonitor.RunWrapped; leave the other metric collection calls unchanged.
🧹 Nitpick comments (1)
internal/scheduling/lib/filter_weigher_pipeline_step_monitor.go (1)
277-303: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDocument the cardinality expectation for dynamic event labels.
countsandeventsare never evicted; the collector lives for the process lifetime. Today's caller only attachesintent, which has a small, bounded value set, so this is safe in practice. However, the type's doc comment onFilterWeigherPipelineStepEvent.Labelsonly requires "valid Prometheus label names" and does not warn against high-cardinality values (e.g., host name, project ID, request ID). A future step using this shared API with an unbounded-value label would grow these maps and the exported series count without bound for the life of the process.Add a doc comment on
pipelineStepEventCollector(and onFilterWeigherPipelineStepEvent.Labelsinfilter_weigher_pipeline_step_result.go) stating that label values must come from a small, bounded set, since entries are never evicted.🤖 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/scheduling/lib/filter_weigher_pipeline_step_monitor.go` around lines 277 - 303, Add documentation to pipelineStepEventCollector and FilterWeigherPipelineStepEvent.Labels stating that dynamic label values must come from a small, bounded set because collector entries and exported series are retained for the process lifetime and never evicted. Keep the existing implementation 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 `@internal/scheduling/lib/filter_weigher_pipeline_step_monitor.go`:
- Around line 304-349: Update pipelineStepEventCollector.Record to reject or
skip dynamic labels named pipeline, step, or event before constructing the key
and storing labels. Preserve valid labels and ensure Collect never receives
duplicate names when building the prometheus.Desc.
---
Outside diff comments:
In `@internal/scheduling/lib/filter_weigher_pipeline_monitor.go`:
- Around line 142-153: Update FilterWeigherPipelineMonitor.Collect to call
stepEventCollector.Collect(ch) only when m.stepEventCollector is non-nil,
matching the existing guard in FilterWeigherPipelineStepMonitor.RunWrapped;
leave the other metric collection calls unchanged.
---
Nitpick comments:
In `@internal/scheduling/lib/filter_weigher_pipeline_step_monitor.go`:
- Around line 277-303: Add documentation to pipelineStepEventCollector and
FilterWeigherPipelineStepEvent.Labels stating that dynamic label values must
come from a small, bounded set because collector entries and exported series are
retained for the process lifetime and never evicted. Keep the existing
implementation 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 67911338-ef5c-4ef4-98cc-e61eeb460383
📒 Files selected for processing (7)
helm/bundles/cortex-nova/templates/alerts.yamlinternal/scheduling/lib/filter_weigher_pipeline_monitor.gointernal/scheduling/lib/filter_weigher_pipeline_step_monitor.gointernal/scheduling/lib/filter_weigher_pipeline_step_monitor_test.gointernal/scheduling/lib/filter_weigher_pipeline_step_result.gointernal/scheduling/nova/plugins/filters/filter_image_properties.gointernal/scheduling/nova/plugins/filters/filter_image_properties_test.go
Signed-off-by: Philipp Matthes <p.matthes@sap.com>
Contributor
Test Coverage ReportTest Coverage 📊: 70.4% |
This was referenced Aug 10, 2026
Merged
PhilippMatthes
pushed a commit
that referenced
this pull request
Aug 10, 2026
Pipeline steps can now report named events with dynamic Prometheus labels via FilterWeigherPipelineStepEvent (introduced in PR #1108), but the docs did not mention this mechanism. This adds a brief subsection under Pipelines so developers writing new filters or weighers know how to emit step events and what metric they produce. Assisted-by: Claude Code:claude-sonnet-4-20250514 [Bash] [Read] Signed-off-by: cortex-ai-agents[bot] <279748396+cortex-ai-agents[bot]@users.noreply.github.com> Co-authored-by: cortex-ai-agents[bot] <279748396+cortex-ai-agents[bot]@users.noreply.github.com>
SoWieMarkus
added a commit
that referenced
this pull request
Aug 10, 2026
## Release cortex v0.3.6 ### New features - **Implement in-flight reservations controller** — adds a new controller that tracks reservations currently being fulfilled by monitoring VM creation state via Nova API, transitioning reservations through their lifecycle stages and cleaning up stale in-flight reservations ([#957](#957)) - **Add KPI that tracks CR count per configured cluster** — introduces the `MulticlusterObjectCountKPI` plugin which counts custom resource objects per cluster and exposes them as Prometheus metrics ([#1054](#1054)) - **Follow VM live migration in CR reservation reconciler** — the committed resource reservation controller now detects when a VM has been live-migrated to a different host and updates the reservation's target host accordingly ([#1048](#1048)) ### Bug fixes - **Subtract reservation CPU blocks when counting placeable slots** — the capacity accounting now correctly deducts reserved CPU blocks from available capacity before calculating how many new instances can be placed ([#1118](#1118)) ### Non-breaking changes - Skip non-candidate hypervisors in nova filters and weighers ([#1117](#1117)) - Add dynamic labels to pipeline step event metrics ([#1108](#1108)) - Update `go.xyrillian.de/gg` to v1.13.2 ([#1111](#1111), [#1122](#1122)) - Update `github.com/sapcc/go-bits` ([#1124](#1124)) - Update `kube-prometheus-stack` to v88.1.5 ([#1112](#1112), [#1119](#1119), [#1121](#1121)) - Update `debian:trixie-slim` Docker digest ([#1120](#1120)) ### Chart versions | Chart | Old | New | |-------|-----|-----| | cortex | 0.3.5 | 0.3.6 | | cortex-shim | 0.1.11 | 0.1.12 | | cortex-postgres | 0.6.11 | 0.6.12 | | cortex-nova | 0.0.85 | 0.0.86 | | cortex-cinder | 0.0.85 | 0.0.86 | | cortex-manila | 0.0.85 | 0.0.86 | | cortex-crds | 0.0.85 | 0.0.86 | | cortex-ironcore | 0.0.85 | 0.0.86 | | cortex-pods | 0.0.85 | 0.0.86 | | cortex-placement-shim | 0.1.11 | 0.1.12 |
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
3 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.
Scheduler pipeline steps can now attach arbitrary key-value labels to the events they report. This allows alerts to pinpoint specific dimensions such as the scheduling intent that triggered an event. The image properties filter uses this to label hypervisor-type-undetermined events with the request intent, and the corresponding alert now includes the intent in its description.
Assisted-by: thalamus/moonshotai/Kimi-K2.7-Code [Bash] [Read]