Add per-event-type topology change metrics for controller#185
Add per-event-type topology change metrics for controller#185thestreak101 wants to merge 2 commits into
Conversation
Surfaces a per-ClusterEventType ReceivedCounter / ProcessedCounter MBean under ClusterStatus:eventName=TopologyChangeEvent for the five event types the rebalancer treats as topology-affecting (IdealStateChange, InstanceConfigChange, ResourceConfigChange, LiveInstanceChange, ClusterConfigChange). Mirrors the topology classification already used by ResourceChangeDetector so metric semantics line up with rebalancer behavior. Received is incremented in pushToEventQueues before the cluster event queue's dedup, processed at the end of handleEvent on successful non-task pipeline runs. The gap between the two reflects coalescing under load -- the signal PRR-30 reviewers asked for to correlate controller load with downstream rebalance throughput. Non-topology event types are filtered at the ClusterStatusMonitor boundary so callers stay simple. All five MBeans are registered eagerly in active() so dashboards (and OTel scrapers) see a stable schema before the first event of a given type arrives. Tests: - TestTopologyChangeEventMonitor: MBean registration, increment, filter, enum coverage lock. - TestTopologyChangeEventMetric: real controller + ZK integration test asserting counters advance on participant join, resource add, and instance disable through the full ZK -> CallbackHandler -> pipeline path; also verifies non-topology MBeans are never registered. ACTIONITEM-18683
thestreak101
requested review from
LZD-PratyushBhatt,
PranaviAncha,
arkmish,
kabragaurav,
laxman-ch,
ngngwr and
proud-parselmouth
as code owners
LZD-PratyushBhatt
left a comment
LZD-PratyushBhatt
left a comment
There was a problem hiding this comment.
Added few comments
| public void incrementReceived() { | ||
| _receivedCounter.updateValue(_receivedCounter.getValue() + 1); | ||
| } | ||
|
|
||
| public void incrementProcessed() { | ||
| _processedCounter.updateValue(_processedCounter.getValue() + 1); | ||
| } |
There was a problem hiding this comment.
pushToEventQueues runs on independent ZK callback threads (one per listener:
onLiveInstanceChange, onIdealStateChange, onInstanceConfigChange,
onResourceConfigChange, onClusterConfigChange, plus forceRebalance from the
scheduled rebalance task). Concurrent increments for the same event type
lose updates.
Back both counters with AtomicLong, expose via a SimpleDynamicMetric
subclass whose getValue() reads from the atomic.
| "ReceivedCounter", icReceivedBefore + 1); | ||
| awaitCounterAtLeast(server, clusterName, ClusterEventType.InstanceConfigChange, | ||
| "ProcessedCounter", icProcessedBefore + 1); | ||
|
|
There was a problem hiding this comment.
can we add test for a ResourceConfig write for ResourceConfigChange, a cluster-config toggle for ClusterConfigChange. each followed by awaitCounterAtLeast on both counters.
| // Count topology-change events received from ZK before they get coalesced in the | ||
| // cluster event queue. Counted once per logical event (not per pipeline fan-out into | ||
| // DEFAULT/TASK queues) so the metric reflects ZK-driven churn, not internal queueing. | ||
| if (_isMonitoring && _clusterStatusMonitor != null) { |
There was a problem hiding this comment.
nit. _clusterStatusMonitor is final, initialized in the ctor at line 697. null check is dead. line 977 omits it. drop && _clusterStatusMonitor != null.
| _clusterStatusMonitor | ||
| .updateClusterEventDuration(ClusterEventMonitor.PhaseName.TotalProcessed.name(), | ||
| _lastPipelineEndTimestamp - startTime); | ||
| // Count topology-change events that completed the resource pipeline successfully. |
There was a problem hiding this comment.
nit: "completed the resource pipeline" is inaccurate as this fires for the management pipeline too
| public static final String EVENT_NAME_KEY = "eventName"; | ||
| public static final String EVENT_TYPE_KEY = "eventType"; | ||
|
|
||
| private static final String SENSOR_DN_KEY = "TopologyChangeEvent"; |
There was a problem hiding this comment.
nit: EVENT_NAME and SENSOR_DN_KEY are both "TopologyChangeEvent". one constant.
| long getReceivedCounter() { | ||
| return _receivedCounter.getValue(); | ||
| } | ||
|
|
||
| long getProcessedCounter() { | ||
| return _processedCounter.getValue(); | ||
| } |
| // Paired with the received counter incremented in pushToEventQueues; the gap | ||
| // between the two reflects coalescing in the cluster event queue under load. | ||
| if (!rebalanceFail) { | ||
| _clusterStatusMonitor.incrementTopologyChangeEventProcessed(event.getEventType()); |
There was a problem hiding this comment.
processed is also incremented for management pipeline events.
When _inManagementMode=true, topology events only go to
_managementModeEventQueue (line 1317). Management registry (lines 667-673)
only has a pipeline for LiveInstanceChange. For the other 4 types,
getPipelinesForEvent returns emptyList, the for-loop is a no-op,
rebalanceFail stays false, and we still bump processed here.
this will have controller stuck in management mode shows received == processed,
looks healthy on dashboards. Exactly what PRR-30 was trying to detect.
Fix:
if (!rebalanceFail && dataProvider instanceof ResourceControllerDataProvider) {
_clusterStatusMonitor.incrementTopologyChangeEventProcessed(event.getEventType());
}Also add an integration case: force management mode, fire an
IdealStateChange, assert processed does NOT move while received does.
Concurrency fix: pushToEventQueues runs on independent ZK callback threads (one per controller listener), so the Long-backed SimpleDynamicMetric increments were racy and lost updates. Switch both counters to an AtomicLong-backed SimpleDynamicMetric subclass so incrementAndGet() makes them safe under concurrent writers. Management-mode gate: in management mode, topology events go to the management mode event queue. The management registry only has a pipeline for LiveInstanceChange; the other four topology types get an empty pipeline list, the for-loop in handleEvent runs zero times, and rebalanceFail stays false -- so the previous gate would falsely credit those events as "processed". Restrict the increment to ResourceControllerDataProvider runs (extracted as a package-private shouldCountTopologyEventAsProcessed predicate so the gate logic is directly unit-testable). Without this fix a controller stuck in management mode would show received==processed -- exactly the false-healthy signal the metric was meant to surface. Test coverage expanded: TestTopologyChangeEventMetric now exercises ResourceConfigChange (resource-config write) and ClusterConfigChange (cluster-config update) end-to-end through the controller, in addition to the existing IdealStateChange / LiveInstanceChange / InstanceConfigChange cases. New TestGenericHelixControllerTopologyMetricGate unit-tests the predicate across resource success, resource failure, management, task-framework, and unknown-data-provider paths. Nits: drop dead getReceivedCounter / getProcessedCounter accessors, collapse duplicated EVENT_NAME / SENSOR_DN_KEY string into one constant, remove dead "_clusterStatusMonitor != null" check in pushToEventQueues (field is final, initialized in the controller constructor). ACTIONITEM-18683
|
Thanks @LZD-PratyushBhatt — addressed all 7 comments in a091201. Substantive
Nits
Test results: 17/17 pass across the 5 affected test classes ( On the management-mode integration test I went with a focused unit test on the extracted predicate rather than a full ZK-freeze integration scenario for two reasons:
Happy to add the heavier ZK-freeze integration case if you'd still prefer it — let me know. On CI: the PR_CI failure is a pre-existing |
2 participants
Issues
Description
Adds per-
ClusterEventTypeReceivedCounter/ProcessedCounterMBean attributes underClusterStatus:eventName=TopologyChangeEventfor the five event types the rebalancer treats as topology-affecting:IdealStateChangeInstanceConfigChangeResourceConfigChangeLiveInstanceChangeClusterConfigChangeThe set mirrors the topology classification already used by
ResourceChangeDetector.determinePropertyMapByType, so metric semantics line up with how the rebalancer itself defines a topology change.Receivedis incremented inGenericHelixController.pushToEventQueuesbefore the cluster event queue's dedup.Processedis incremented at the end ofhandleEventon successful non-task pipeline runs. The gap between the two reflects coalescing under load -- the signal needed to correlate controller load with rebalance throughput.Non-topology event types (
MessageChange,CurrentStateChange, etc.) are filtered at theClusterStatusMonitorboundary so callers stay simple. All five MBeans are registered eagerly inactive()so dashboards and OTel scrapers see a stable schema before the first event of a given type arrives.MBean shape
Files changed
controller/stages/ClusterEventType.java-- addsisTopologyChange()predicate andtopologyChangeEventTypes()set as the single source of truth for the filter.monitoring/mbeans/TopologyChangeEventMonitor.java(new) -- per-event-type MBean built on the existingDynamicMBeanProviderpattern (sibling ofClusterEventMonitor).monitoring/mbeans/ClusterStatusMonitor.java-- map of monitors, eager registration inactive(), unregister inreset(), publicincrementTopologyChangeEvent{Received,Processed}methods that no-op on non-topology types.controller/GenericHelixController.java-- two hook lines (Received inpushToEventQueues, Processed at end ofhandleEvent).Tests
The following tests are written for this issue:
TestTopologyChangeEventMonitor(unit, 3 tests):testEagerRegistrationAndCounters-- all 5 MBeans registered atactive(), counters start at zero, increments route to the matching event type, non-topology types are silently dropped, MBeans unregistered onreset().testConstructorRejectsNonTopologyType-- monitor construction rejects non-topologyClusterEventTypevalues.testTopologyChangeEventTypesCoverage-- locks the topology set in tests so future enum additions are a conscious choice.TestTopologyChangeEventMetric(integration, 1 test): drives a real controller + ZK + 2 participants and asserts that:LiveInstanceChangeandIdealStateChangeReceived/Processed counters move on cluster startup.IdealStateChangecounters advance past baseline when a new resource is added.InstanceConfigChangecounters advance past baseline when an instance is disabled.MessageChangeand other non-topology types never get a topology MBean registered.The following is the result of running
mvn -pl helix-core testagainst the new and adjacent monitor tests:Tests included:
TestTopologyChangeEventMonitor(3),TestTopologyChangeEventMetric(1),TestClusterStatusMonitor(7),TestClusterEventStatusMonitor(1). No regressions in adjacent monitor tests.Changes that Break Backward Compatibility (Optional)
None. Purely additive: a new MBean class, new public methods on
ClusterStatusMonitor, two new lines inGenericHelixController. No existing attributes, methods, or behaviors are changed.Documentation (Optional)
None for this PR. Internal follow-up will land an
OtelJmxAdaptor.SENSOR_DIMENSION_NAMESentry so OTel labels readCluster.Name/Event.Name/Event.Typeinstead of the raw lowercase ObjectName keys; metric works without it but with inconsistent label casing.Commits
Single commit, follows the standard guidelines (subject in imperative mood under 50 chars, body wraps at 72, explains what and why).
Code Quality
Diff follows the existing
helix-style.xmlconventions. New code matches the established_fieldNameunderscore convention used throughout the surroundingClusterStatusMonitor/ClusterEventMonitor/ monitor MBean classes.