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: [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]
