RohanExploit commented on issue #2402: URL: https://github.com/apache/hertzbeat/issues/2402#issuecomment-5341660634
I dug into this on current master (`67c861a`). The bug still reproduces, but the cause suggested in 2024 no longer matches the code, so here is what I found. `MonitorServiceImpl.deleteMonitors` does cancel the job now: ```java collectorMonitorBindDao.deleteCollectorMonitorBindsByMonitorId(monitor.getId()); collectJobScheduling.cancelAsyncCollectJob(monitor.getJobId()); applicationContext.publishEvent(new MonitorDeletedEvent(applicationContext, monitor.getId())); ``` So collection really does stop. The repeated alerts are not new collections, they are one alert that can never leave the firing state: 1. Any alert already firing for that monitor lives in `AlarmCacheManager.firingAlertMap` and in `single_alert` with status `FIRING`. 2. The only thing that clears it is `handleRecoveredAlert(...)` in `MetricsRealTimeAlertCalculator`, which is driven by incoming `MetricsData`. 3. The job is cancelled, so no `MetricsData` ever arrives for that monitor again, and the alert can never recover. 4. `AlarmGroupReduce` re-sends firing groups on a fixed timer (`scheduleAtFixedRate(this::dispatchCheckAndSendGroups, 10000, CHECK_INTERVAL)` at line 130), throttled per group by `repeatInterval` (default 4h). That is the "重复发送" part, and it is timer driven, so it keeps going with no further input. 5. `AlarmCacheManager`'s constructor reloads every `FIRING` row from `singleAlertDao.querySingleAlertsByStatus(...)`, so a restart does not clear it either. The alert shows only the raw ID because the monitor row is gone, so nothing can resolve it to a name. There is already an event meant for exactly this cleanup, but nothing consumes it. Grepping the whole tree for `MonitorDeletedEvent` returns only four hits: the class itself, the import, and the two `publishEvent` calls (`MonitorServiceImpl` lines 504 and 713). There is no `@EventListener` for it anywhere. One thing that makes the fix a design decision rather than a one liner: the alert fingerprint labels do not carry the monitor id. `MetricsRealTimeAlertCalculator` builds them as ```java commonFingerPrints.put(CommonConstants.LABEL_DEFINE_ID, String.valueOf(define.getId())); commonFingerPrints.put(CommonConstants.LABEL_ALERT_NAME, define.getName()); commonFingerPrints.put(CommonConstants.LABEL_INSTANCE_NAME, instanceName); commonFingerPrints.put(CommonConstants.LABEL_INSTANCE, instanceHost); ``` `LABEL_INSTANCE` is the monitor's `instance` field (host:port), not its id. The monitor id is only put into `commonContext` under `__instance__` for expression evaluation, and never reaches the stored labels. So a listener cannot currently select "all firing alerts for monitor 123" without either matching on host, which is ambiguous when two monitors share a host, or adding a monitor id label. Two directions I can see: * Add a monitor id label to the fingerprint in the calculator, then add a `MonitorDeletedEvent` listener that resolves the matching firing alerts in both `AlarmCacheManager` and `single_alert`. Cleanest to target, but it changes fingerprints, so existing firing alerts would need a migration or would be orphaned once. * Keep fingerprints as they are and have the listener resolve by the deleted monitor's `instance` plus `instancename` labels. Smaller blast radius, but ambiguous when several monitors share a host. I would rather not guess which tradeoff you want. If a maintainer picks a direction I am happy to implement it with tests. @tomsun28 since you triaged this originally. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
