You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@hertzbeat.apache.org by "love12yadav (via GitHub)" <gi...@apache.org> on 2026/01/05 06:49:12 UTC
[PR] Fix: correct NoticeConfigService label and time filtering logic #3852 [hertzbeat]
love12yadav opened a new pull request, #3945:
URL: https://github.com/apache/hertzbeat/pull/3945
## Problem
NoticeConfigService filtering logic for labels and time periods was incorrect.
- Single alert labels were not fully matched.
- Time filter for periodStart/periodEnd could skip alerts incorrectly.
## Solution
- Fix label matching for single alerts.
- Correct time filter logic.
- Keep existing cache handling unchanged.
#3852
--
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
Re: [PR] Fix: correct NoticeConfigService label and time filtering logic #3852 [hertzbeat]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3945:
URL: https://github.com/apache/hertzbeat/pull/3945#discussion_r2661828056
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/NoticeConfigServiceImpl.java:
##########
@@ -213,43 +213,58 @@ public List<NoticeRule> getReceiverFilterRule(GroupAlert alert) {
// The temporary rule is to forward all, and then implement more matching rules: alarm status selection, monitoring type selection, etc.
return rules.stream()
- .filter(rule -> {
- if (!rule.isFilterAll()) {
- // filter labels
- if (rule.getLabels() != null && !rule.getLabels().isEmpty()) {
- boolean labelMatch = rule.getLabels().entrySet().stream().allMatch(labelItem -> {
- if (!alert.getCommonLabels().containsKey(labelItem.getKey())) {
- return false;
- }
- String alertLabelValue = alert.getCommonLabels().get(labelItem.getKey());
- return Objects.equals(labelItem.getValue(), alertLabelValue);
- });
- if (!labelMatch) {
- return false;
+ .filter(rule -> {
+ if (!rule.isFilterAll()) {
+ if (rule.getLabels() != null && !rule.getLabels().isEmpty()) {
+
+ boolean labelMatch = rule.getLabels().entrySet().stream().allMatch(labelItem -> {
+
+ // 1. Check common labels
+ if (alert.getCommonLabels() != null &&
+ Objects.equals(alert.getCommonLabels().get(labelItem.getKey()), labelItem.getValue())) {
+ return true;
}
- }
- }
-
- LocalDateTime nowDate = LocalDateTime.now();
- // filter day
- int currentDayOfWeek = nowDate.toLocalDate().getDayOfWeek().getValue();
- if (rule.getDays() != null && !rule.getDays().isEmpty()) {
- boolean dayMatch = rule.getDays().stream().anyMatch(item -> item == currentDayOfWeek);
- if (!dayMatch) {
+
+ // 2. Check group labels
+ if (alert.getGroupLabels() != null &&
+ Objects.equals(alert.getGroupLabels().get(labelItem.getKey()), labelItem.getValue())) {
+ return true;
+ }
+
+ // 3. Check single alert labels (MOST IMPORTANT)
+ return alert.getAlerts() != null && alert.getAlerts().stream().anyMatch(singleAlert ->
+ singleAlert.getLabels() != null &&
+ Objects.equals(singleAlert.getLabels().get(labelItem.getKey()), labelItem.getValue())
+ );
+ });
+
+ if (!labelMatch) {
return false;
}
}
- // filter time
- LocalTime nowTime = nowDate.toLocalTime();
- boolean startMatch = rule.getPeriodStart() == null
- || nowTime.isAfter(rule.getPeriodStart().toLocalTime())
- || (rule.getPeriodEnd() != null && rule.getPeriodStart().isAfter(rule.getPeriodEnd())
- && nowTime.isBefore(rule.getPeriodStart().toLocalTime()));
- boolean endMatch = rule.getPeriodEnd() == null
- || nowTime.isBefore(rule.getPeriodEnd().toLocalTime());
- return startMatch && endMatch;
- })
- .collect(Collectors.toList());
+ }
+
+
+ LocalDateTime nowDate = LocalDateTime.now();
+ // filter day
+ int currentDayOfWeek = nowDate.toLocalDate().getDayOfWeek().getValue();
+ if (rule.getDays() != null && !rule.getDays().isEmpty()) {
+ boolean dayMatch = rule.getDays().stream().anyMatch(item -> item == currentDayOfWeek);
+ if (!dayMatch) {
+ return false;
+ }
+ }
+ // filter time
+ LocalTime nowTime = nowDate.toLocalTime();
+ boolean startMatch = rule.getPeriodStart() == null
+ || nowTime.isAfter(rule.getPeriodStart().toLocalTime())
+ || (rule.getPeriodEnd() != null && rule.getPeriodStart().isAfter(rule.getPeriodEnd())
+ && nowTime.isBefore(rule.getPeriodStart().toLocalTime()));
+ boolean endMatch = rule.getPeriodEnd() == null
+ || nowTime.isBefore(rule.getPeriodEnd().toLocalTime());
+ return startMatch && endMatch;
Review Comment:
The time filtering logic doesn't correctly handle overnight periods. When periodStart > periodEnd (e.g., 22:00 to 06:00), the logic should match times that are either:
- After periodStart (e.g., 22:00-23:59), OR
- Before periodEnd (e.g., 00:00-06:00)
The current implementation separates startMatch and endMatch with AND logic, which fails for overnight periods. For example, with a period of 22:00 to 06:00:
- At 23:00: startMatch=true (after 22:00), but endMatch=false (not before 06:00), result: false (incorrect, should be true)
- At 05:00: startMatch would depend on the third condition, endMatch=true, but the overall logic is flawed
The logic should be rewritten to handle regular and overnight periods separately.
```suggestion
LocalTime startTime = rule.getPeriodStart() != null ? rule.getPeriodStart().toLocalTime() : null;
LocalTime endTime = rule.getPeriodEnd() != null ? rule.getPeriodEnd().toLocalTime() : null;
boolean timeMatch;
if (startTime == null && endTime == null) {
// no time restriction
timeMatch = true;
} else if (startTime != null && endTime == null) {
// only start bound: now >= start
timeMatch = !nowTime.isBefore(startTime);
} else if (startTime == null) {
// only end bound: now <= end
timeMatch = !nowTime.isAfter(endTime);
} else if (!startTime.isAfter(endTime)) {
// normal period (same day): start <= now <= end
timeMatch = !nowTime.isBefore(startTime) && !nowTime.isAfter(endTime);
} else {
// overnight period (crosses midnight): now >= start OR now <= end
timeMatch = !nowTime.isBefore(startTime) || !nowTime.isAfter(endTime);
}
return timeMatch;
```
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/NoticeConfigServiceImpl.java:
##########
@@ -213,43 +213,58 @@ public List<NoticeRule> getReceiverFilterRule(GroupAlert alert) {
// The temporary rule is to forward all, and then implement more matching rules: alarm status selection, monitoring type selection, etc.
return rules.stream()
- .filter(rule -> {
- if (!rule.isFilterAll()) {
- // filter labels
- if (rule.getLabels() != null && !rule.getLabels().isEmpty()) {
- boolean labelMatch = rule.getLabels().entrySet().stream().allMatch(labelItem -> {
- if (!alert.getCommonLabels().containsKey(labelItem.getKey())) {
- return false;
- }
- String alertLabelValue = alert.getCommonLabels().get(labelItem.getKey());
- return Objects.equals(labelItem.getValue(), alertLabelValue);
- });
- if (!labelMatch) {
- return false;
+ .filter(rule -> {
+ if (!rule.isFilterAll()) {
+ if (rule.getLabels() != null && !rule.getLabels().isEmpty()) {
+
+ boolean labelMatch = rule.getLabels().entrySet().stream().allMatch(labelItem -> {
+
+ // 1. Check common labels
+ if (alert.getCommonLabels() != null &&
+ Objects.equals(alert.getCommonLabels().get(labelItem.getKey()), labelItem.getValue())) {
+ return true;
}
- }
- }
-
- LocalDateTime nowDate = LocalDateTime.now();
- // filter day
- int currentDayOfWeek = nowDate.toLocalDate().getDayOfWeek().getValue();
- if (rule.getDays() != null && !rule.getDays().isEmpty()) {
- boolean dayMatch = rule.getDays().stream().anyMatch(item -> item == currentDayOfWeek);
- if (!dayMatch) {
+
+ // 2. Check group labels
+ if (alert.getGroupLabels() != null &&
+ Objects.equals(alert.getGroupLabels().get(labelItem.getKey()), labelItem.getValue())) {
+ return true;
+ }
+
+ // 3. Check single alert labels (MOST IMPORTANT)
+ return alert.getAlerts() != null && alert.getAlerts().stream().anyMatch(singleAlert ->
+ singleAlert.getLabels() != null &&
+ Objects.equals(singleAlert.getLabels().get(labelItem.getKey()), labelItem.getValue())
+ );
+ });
+
+ if (!labelMatch) {
return false;
}
}
- // filter time
- LocalTime nowTime = nowDate.toLocalTime();
- boolean startMatch = rule.getPeriodStart() == null
- || nowTime.isAfter(rule.getPeriodStart().toLocalTime())
- || (rule.getPeriodEnd() != null && rule.getPeriodStart().isAfter(rule.getPeriodEnd())
- && nowTime.isBefore(rule.getPeriodStart().toLocalTime()));
- boolean endMatch = rule.getPeriodEnd() == null
- || nowTime.isBefore(rule.getPeriodEnd().toLocalTime());
- return startMatch && endMatch;
- })
- .collect(Collectors.toList());
+ }
+
+
+ LocalDateTime nowDate = LocalDateTime.now();
+ // filter day
+ int currentDayOfWeek = nowDate.toLocalDate().getDayOfWeek().getValue();
+ if (rule.getDays() != null && !rule.getDays().isEmpty()) {
+ boolean dayMatch = rule.getDays().stream().anyMatch(item -> item == currentDayOfWeek);
+ if (!dayMatch) {
+ return false;
+ }
+ }
+ // filter time
+ LocalTime nowTime = nowDate.toLocalTime();
+ boolean startMatch = rule.getPeriodStart() == null
+ || nowTime.isAfter(rule.getPeriodStart().toLocalTime())
+ || (rule.getPeriodEnd() != null && rule.getPeriodStart().isAfter(rule.getPeriodEnd())
+ && nowTime.isBefore(rule.getPeriodStart().toLocalTime()));
+ boolean endMatch = rule.getPeriodEnd() == null
+ || nowTime.isBefore(rule.getPeriodEnd().toLocalTime());
+ return startMatch && endMatch;
+ })
+ .collect(Collectors.toList());
Review Comment:
The getReceiverFilterRule method now includes significantly more complex label matching logic (checking commonLabels, groupLabels, and individual alert labels) and corrected time filtering. However, there are no test cases covering these filtering scenarios. Consider adding tests for:
- Label matching with different combinations (commonLabels, groupLabels, singleAlert labels)
- Time period filtering including overnight periods (e.g., 22:00 to 06:00)
- Day of week filtering
- Combined filters (labels + time + days)
--
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
Re: [PR] Fix: correct NoticeConfigService label and time filtering logic #3852 [hertzbeat]
Posted by "love12yadav (via GitHub)" <gi...@apache.org>.
love12yadav closed pull request #3945: Fix: correct NoticeConfigService label and time filtering logic #3852
URL: https://github.com/apache/hertzbeat/pull/3945
--
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