Re: [PR] [alerter]bugfix: preserve group alert recovery events [hertzbeat]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #4168:
URL: https://github.com/apache/hertzbeat/pull/4168#discussion_r3522709352


##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/reduce/AlarmGroupReduce.java:
##########
@@ -294,13 +294,17 @@ private void sendGroupAlert(GroupAlertCache cache) {
             AlertGroupConverge ruleConfig = groupDefines.get(cache.getGroupDefineName());
             long repeatInterval = ruleConfig.getRepeatInterval() != null
                     ? ruleConfig.getRepeatInterval() * MS_PER_SECOND : DEFAULT_REPEAT_INTERVAL;

Review Comment:
   `sendGroupAlert` assumes `groupDefines` still contains `cache.getGroupDefineName()`. If group rules are refreshed/removed while caches exist, `ruleConfig` can be null and `ruleConfig.getRepeatInterval()` will throw NPE. `shouldSendGroup(...)` already handles `ruleConfig == null`, so `sendGroupAlert(...)` should too and fall back to defaults.



##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/impl/DbAlertStoreHandlerImpl.java:
##########
@@ -127,9 +127,26 @@ public GroupAlert store(GroupAlert groupAlert) {
             }
             // Save alert group
             groupAlert.setAlertFingerprints(alertFingerprints.stream().toList());
+            refreshGroupStatus(groupAlert);
             GroupAlert savedGroupAlert = groupAlertDao.save(groupAlert);
             savedGroupAlert.setAlerts(groupAlert.getAlerts());
             return savedGroupAlert;
         }
     }
+
+    private void refreshGroupStatus(GroupAlert groupAlert) {
+        List<String> alertFingerprints = groupAlert.getAlertFingerprints();
+        if (alertFingerprints == null || alertFingerprints.isEmpty()) {
+            return;
+        }
+        List<SingleAlert> alerts = singleAlertDao.findSingleAlertsByFingerprintIn(alertFingerprints);
+        if (alerts == null) {
+            return;
+        }
+        boolean hasFiringAlert = alerts.stream()
+                .anyMatch(alert -> CommonConstants.ALERT_STATUS_FIRING.equals(alert.getStatus()));

Review Comment:
   `refreshGroupStatus` can change the group `status` based on alerts loaded from the DB, but the outgoing `GroupAlert` still carries only the transient `alerts` list from the current push. This can produce inconsistent payloads (e.g., `status=firing` while `alerts` contains only a resolved member), and templates/plugins rely on `alert.getAlerts()` for content.



##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/notice/impl/DbAlertStoreHandlerImpl.java:
##########
@@ -127,9 +127,26 @@ public GroupAlert store(GroupAlert groupAlert) {
             }
             // Save alert group
             groupAlert.setAlertFingerprints(alertFingerprints.stream().toList());
+            refreshGroupStatus(groupAlert);
             GroupAlert savedGroupAlert = groupAlertDao.save(groupAlert);

Review Comment:
   `refreshGroupStatus(groupAlert)` performs a DB lookup over the full `alertFingerprints` set on every store call. Since this is only needed to correct the *resolved* misclassification case (when the incoming payload says `resolved` but historical members are still firing), you can skip the DB query when the incoming group is already `firing` to reduce load.



-- 
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: notifications-unsubscribe@hertzbeat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@hertzbeat.apache.org
For additional commands, e-mail: notifications-help@hertzbeat.apache.org