Repository navigation
Add reconciliation metrics and Kubernetes events - #52
Conversation
- add ReconcileObserver, reporting every reconciliation pass (phase, outcome, duration) and named steps within one; passes cancelled mid-way are reported as abandoned - add PrometheusMetrics behind a new `prometheus` feature - add EventRecorder, publishing events.k8s.io/v1 events that aggregate identical repeats into one event's series and truncate overlong notes - publish a warning event when reconciling a resource fails, via the new Context::failure_event hook - add Controller::with_name/with_observer/with_event_recorder, and TraceMetadata::step/set_outcome/publish_event - export Error, with Error::display_chain, so error_action can be overridden outside this crate Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Event publishing has a critical concurrency flaw, with additional error rendering, test coverage, and CI gaps.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds controller-wide reconciliation metrics and Kubernetes event recording, including optional Prometheus support.
Changes:
- Adds reconciliation phases, outcomes, observers, and step timing.
- Adds aggregated Kubernetes events and failure hooks.
- Updates controller integration, tests, exports, and documentation.
| File | Description and findings |
|---|---|
src/test_util.rs |
Adds mock API-server utilities. Nit (1 vote): Fix the “Creates respond” documentation wording. |
src/prometheus.rs |
Implements Prometheus metrics collection. No direct issue identified. |
src/observe.rs |
Defines observation APIs. Nits (1 vote each): Fix feature-gated documentation link and inaccurate ReconcileRecord and finish_with behavior descriptions. |
src/lib.rs |
Exports and documents new APIs. Nit (1 vote): Default-feature documentation links to feature-gated PrometheusMetrics. |
src/events.rs |
Implements event recording and aggregation. Critical (4 votes): Concurrent publishes can lose counts, create duplicate series, or corrupt cached state; serialize transitions per SeriesKey and test concurrency. |
src/controller.rs |
Integrates instrumentation and failure events. Moderate (2 votes): display_chain can suppress real causes. Moderate (1 vote): Finalizer phases and PATCH failures lack tests. Nit (1 vote): Cleanup documentation incorrectly claims success_action behavior matches apply. |
README.md |
Documents observability features. No issue identified. |
CHANGELOG.md |
Records the unreleased functionality. No issue identified. |
Cargo.toml |
Adds Prometheus and test dependencies. Moderate (1 vote): CI does not compile or test the optional Prometheus feature. |
Cargo.lock |
Updates dependency resolution. No issue identified. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if let Some(key) = &key | ||
| && let Some((namespace, name, count)) = self.repeat_of(key, &event) | ||
| { | ||
| match self.patch_series(&namespace, &name, count).await { |
| let message = err.to_string(); | ||
| // Error types commonly include their source's message in their | ||
| // own (as this one does), which a naive join would repeat. | ||
| if !out.contains(&message) { |
- serialize event publishes per series key, so concurrent identical events can't each create an event, patch to the same count, or update a series that was replaced mid-request - only skip a cause in Error::display_chain when the message so far ends with it, rather than merely contains it - reduce the default histogram buckets to 10ms..30s - derive PrometheusMetrics' desc/collect from one exhaustive list of its collectors, so a new metric can't be left out of either - add the conditions module for maintaining standard status conditions: set keeps lastTransitionTime unless the status changes and reports whether anything changed; find_observed ignores conditions determined from an older generation Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alex-hunt-materialize
left a comment
There was a problem hiding this comment.
Overall, this looks good.
| /// A single recorder is typically shared (via [`Arc`]) by every controller in | ||
| /// a process, and by the reconcilers that publish events of their own. | ||
| pub struct EventRecorder { | ||
| client: Client, | ||
| reporter: Reporter, | ||
| series: Mutex<HashMap<SeriesKey, Slot>>, | ||
| } |
There was a problem hiding this comment.
If the recorder is is shared by every controller, how do we know which one published the event? It's pretty common to have multiple controllers reconciling the same object.
There was a problem hiding this comment.
the Reporter has a reportingController and reportingInstance
There was a problem hiding this comment.
Maybe I'm just missing something, but don't we also only have one reporter?
I think the term "controller" might be causing some confusion between us. I mean the individual instances of things implementing the traits in this repo, of which there can be multiple in a single pod. These controllers don't even need to point at different resource types. We could have three controllers in a single pod all pointing at Materialize CRs, for example. In that case, how would we know which one generated the event if they all share the same EventRecorder (and therefore the same Reporter)?
There was a problem hiding this comment.
oh, traditionally you'd want a separate reporter per controller implementation. Bad comment.
| struct SeriesKey { | ||
| /// The controller whose failure events these are, or `None` for events | ||
| /// published by reconcilers or other callers. | ||
| origin: Option<Arc<str>>, |
There was a problem hiding this comment.
It is a bit unclear to me why this would ever not be Some. Shouldn't we always have access to the name of the controller to be able to set this?
There was a problem hiding this comment.
mostly because standard kubernetes allows it to be None. EventRecorder::publish can be called outside of the controller (like a failed to refresh a cache on a timer).
- document giving each controller its own EventRecorder, whose reporter names it, since the reporter is all that identifies which controller published an event; with that, failure series no longer need keying by controller name - document that Reporter::controller must be a qualified name - bound each publish with a timeout (5s by default, configurable with EventRecorder::with_timeout), so an unresponsive API server can't hold up a failed reconciliation for the client's full read timeout - return PublishError from EventRecorder::publish, to report timeouts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### Motivation Part of [CLO-188](https://linear.app/materializeinc/issue/CLO-188). This reimplements #39030 on top of k8s-controller 0.13.0, which now provides reconciliation metrics and Kubernetes events itself (MaterializeInc/k8s-controller#52). The metric names, steps and lifecycle events are the same; orchestratord no longer carries its own reconciliation wrapper and event publisher. ### Description - **Dependency:** bumps `k8s-controller` to 0.13.0 with its `prometheus` feature. - **Metrics:** registers k8s-controller's Prometheus metrics under the `orchestratord` prefix and reports every controller's passes to them: - `orchestratord_reconciliations_total{controller, phase, outcome}` - `orchestratord_reconciliation_duration_seconds{controller, phase}` - `orchestratord_reconciliation_steps_total{controller, step, outcome}` - `orchestratord_reconciliation_step_duration_seconds{controller, step}` - **Steps:** the Materialize, Balancer and Console reconcilers mark named steps through the `TraceMetadata` each pass receives, with the same step names as #39030. - **Failure events:** each controller gets its own `EventRecorder`, so the event's reporting controller identifies it: `orchestratord.materialize.cloud/materialize`, `/balancer` or `/console`, with the pod name as the instance. A failed pass publishes a `ReconcileFailed` warning event on the resource, with the error and its causes as the note. - **Lifecycle events:** the Materialize controller publishes each `UpToDate` transition as an event once the status is written. The condition's reason and message become the event's. It's a warning when the environment is not up to date, except while waiting for approval. - **RBAC:** the operator's ClusterRole gains `create` and `patch` on `events.k8s.io` events. - **Metrics catalog:** `mz-metrics-catalog` only saw `metric!` invocations, so it now also recognizes `PrometheusMetrics::new("<namespace>")`. It documents those metrics by constructing them and reading back their descriptions, as it already does for tokio's runtime metrics. It now depends on `k8s-controller` and `k8s-openapi`, which makes the catalog lint slower to build. Differences from #39030 that reviewers may notice: - The failure event reason is k8s-controller's `ReconcileFailed`, not `ReconciliationFailed`. - The phase label is `phase` rather than `event_type`. - Each controller reports as itself instead of all sharing `orchestratord.materialize.cloud`. - An identical lifecycle event repeated within 10 minutes increments the existing event's count instead of creating a new event. The messages include the generation number, so this only applies when a transition really repeats. ### Verification - `mz-metrics-catalog`: a test that `PrometheusMetrics` constructions are documented with the right names and labels. - `mz-orchestratord`: a test of which lifecycle transitions are published as warnings. - Helm: the ClusterRole test asserts the events permission. - `test/orchestratord`: - A new `failure-events` workflow, with a nightly step. It drives an environment through two different failures, a missing backend secret and then an invalid license key, and checks that the `ReconcileFailed` event's note follows the current cause. - `manually-promote` now checks that each rollout phase was published as an event. - Deployments expected to fail (`post_run_check` with `expect_fail`) now also check for a `ReconcileFailed` event. ### Release notes This release will publish Kubernetes events on Materialize, Balancer and Console resources when the operator fails to reconcile them, and on each Materialize rollout phase, so `kubectl describe` shows why a resource isn't progressing. The operator's ClusterRole now includes `create` and `patch` on `events.k8s.io` events. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>


What
Adds controller-agnostic building blocks for observing and reporting on reconciliation:
Controller::with_observer(Arc<dyn ReconcileObserver>)reports each pass with its controller name, resource,Phase(init/apply/cleanup/delete),Outcome(completed/waiting/skipped/failed/abandoned) and duration.metadata.step("...")on theTraceMetadatathey already receive.metadata.set_outcome(...)overrides a pass's outcome.abandoned.PrometheusMetrics, behind a new optionalprometheusfeature, implements the trait. It's aCollector, so it works with a plainprometheus::Registryormz_ore'sregister_collector. Default histogram buckets run from 10ms to 30s ([0.01, 0.05, 0.25, 1, 5, 30]);with_bucketsoverrides them.events::EventRecorderpublishesevents.k8s.io/v1events on behalf of one controller.Reporternames it (a qualified name such asexample.com/widgets). The reporter is the only thing in an event that shows which controller published it, even when several controllers reconcile the same kind of resource.series.count; any change (including to the note) starts a new event. Notes over 1 KiB are truncated. A series whose event the API server has already expired is recreated.Controller::with_event_recorderpublishes a warning event whenever a pass fails. The event comes from the newContext::failure_eventhook, which has a default implementation and can be overridden to reword or suppress it.metadata.publish_event(...). Code outside a reconcile (for example, a background task) can publish on the controller's behalf withEventRecorder::publish.EventRecorder::with_timeoutoverrides it), returningPublishError::Timeout.conditionsmodule maintains standardmetav1.Conditions, following the same rules as apimachinery'smeta.SetStatusCondition.conditions::setapplies aDesiredCondition. It keepslastTransitionTimeunless the status changes, and returnsUnchanged/Updated/Transitioned, which tells the caller whether the status needs writing.conditions::find_observedignores conditions determined from an older generation of the resource.Controller::with_namesets the controller label. It defaults toFINALIZER_NAME, or failing that the resource kind.Erroris now exported, along withError::display_chain. Before this,Context::error_actioncouldn't be overridden outside the crate.Why
Downstream controllers (orchestratord in materialize, and the cloud environment/region controllers and stack-deployer) have no metrics for reconciliation, and no way for a stuck object to explain itself in
kubectl describe. Materialize's orchestratord has a prototype of the metrics and events as aContextwrapper (CLO-188). Building them into the crate:Every downstream CRD already uses
metav1.Condition, but each repo updates conditions by hand, and several getlastTransitionTimewrong (resetting it when only the message changes, or stamping it on every write).conditionsgives them one correct implementation to move to.Reviewer notes
#[doc(hidden)]Context::_reconcile, which became a private function.publish_eventorEventRecorder::publish; otherwise a reconciler that publishes the same event every pass while it waits would create a new event each time. This is why a recorder must not be shared between controllers: one controller's success would reset another's failure series.Ok(Some(Action::requeue(..)))counts aswaiting;Noneandawait_change()count ascompleted. A controller that requeues as a periodic resync after converging would show as waiting unless it callsset_outcome.createandpatchonevents.k8s.io/events. This is documented in theeventsmodule.src/test_util.rs). The async tests run on a small helper runtime instead of#[tokio::test], because tokio'smacrosfeature pulls in a secondsynversion that cargo-deny rejects.🤖 Generated with Claude Code