feat: notify Metrics when a controller's event processor starts#3485
feat: notify Metrics when a controller's event processor starts#3485jiangzho wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new lifecycle callback to the SDK’s monitoring API to signal when a controller’s event processor actually begins accepting events, enabling measurement of operator/controller startup latency (including deferred-start scenarios such as leader election).
Changes:
- Introduces
Metrics.eventProcessingStarted(Controller)and wires it throughControllerandAggregatedMetrics. - Adds unit tests covering notification behavior around inline vs deferred event processor start.
- Implements the callback in Micrometer metrics (V1 and V2) as a per-controller gauge set to JVM uptime at the moment processing starts.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java | Emits the new metrics callback when event processing starts (inline and deferred). |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java | Adds the eventProcessingStarted default method and Javadoc contract. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/AggregatedMetrics.java | Forwards the new callback to all aggregated metrics instances. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/ControllerTest.java | Adds coverage for the new callback behavior. |
| micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetricsV2.java | Implements callback as a per-controller gauge. |
| micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java | Implements callback as a per-controller gauge (legacy naming style). |
| @Test | ||
| void notifiesMetricsWhenEventProcessorStarts() { | ||
| final var client = MockKubernetesClient.client(Secret.class); | ||
| final var metrics = mock(Metrics.class); | ||
| final var configurationService = | ||
| ConfigurationService.newOverriddenConfigurationService( | ||
| new BaseConfigurationService(), o -> o.withMetrics(metrics)); | ||
| final var configuration = | ||
| MockControllerConfiguration.forResource(Secret.class, configurationService); | ||
| final var controller = new Controller<Secret>(reconciler, configuration, client); | ||
|
|
||
| // Inline start (event processing started synchronously, e.g. no leader election). | ||
| controller.start(true); | ||
| verify(metrics, times(1)).eventProcessingStarted(controller); | ||
|
|
||
| // Deferred start (event processing started later, e.g. after acquiring leadership). | ||
| controller.startEventProcessing(); | ||
| verify(metrics, times(2)).eventProcessingStarted(controller); | ||
| } |
Add Metrics.eventProcessingStarted(Controller), invoked in Controller whenever the event processor begins accepting events (both on inline start and after a deferred start, e.g. once leader election succeeds), so implementations can measure operator startup latency. Wire it through AggregatedMetrics so composed Metrics instances all receive the callback, and implement it in MicrometerMetrics/V2 as a per-controller gauge recording JVM uptime at the moment processing starts.
55ebfd9 to
b55c9bd
Compare
|
Hi @jiangzho , thank you for the PR, looks good to me for covering initial controller startup. I was just thinking if we should also cover dynamic namespace changes: When we change dynamically watched namespaces, we stop the event processing, and if all the nem namespaces synced in terms of the underlying informer (and/or namespaces are removed) we start the processing again. Would it make sense also to monitor this? |
| final var tags = new ArrayList<Tag>(); | ||
| addControllerNameTag(name, tags); | ||
| registry.gauge( | ||
| PROCESSING_STARTED_LATENCY_GAUGE, tags, ManagementFactory.getRuntimeMXBean().getUptime()); |
There was a problem hiding this comment.
Was wondering also if JMX is the best strategy to use here in general. But probably would would be ok for now. Since it measures JVM startup time; might a difference between startup and when Operator.start() is called. However I don't have strong opinion if we should measure one or the other. So this is fine by me.
We have been observing logs like
While this is expected during startup, it would be beneficial for us to measure the startup delay
Add Metrics.eventProcessingStarted(Controller), invoked in Controller
whenever the event processor begins accepting events (both on inline
start and after a deferred start, e.g. once leader election succeeds),
so implementations can measure operator startup latency.
Wire it through AggregatedMetrics so composed Metrics instances all
receive the callback, and implement it in MicrometerMetrics/V2 as a
per-controller gauge recording JVM uptime at the moment processing
starts.
Add Metrics.eventProcessingStarted(Controller), invoked in Controller
whenever the event processor begins accepting events (both on inline
start and after a deferred start, e.g. once leader election succeeds),
so implementations can measure operator startup latency.
Wire it through AggregatedMetrics so composed Metrics instances all
receive the callback, and implement it in MicrometerMetrics/V2 as a
per-controller gauge recording JVM uptime at the moment processing
starts.