Copilot commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2518246794
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/grpc/GRPCCallback.java:
##########
@@ -51,7 +54,6 @@ public class GRPCCallback implements AlarmCallback {
public GRPCCallback(AlarmRulesWatcher alarmRulesWatcher) {
this.alarmRulesWatcher = alarmRulesWatcher;
- this.alarmSettingMap = new HashMap<>();
this.alarmServiceStubMap = new HashMap<>();
this.grpcClientMap = new HashMap<>();
Review Comment:
The field `alarmSettingMap` is not initialized in the constructor before
being used. It should be initialized as `this.alarmSettingMap = new
HashMap<>();` before the conditional block that uses `alarmSettingMap`.
```suggestion
this.grpcClientMap = new HashMap<>();
this.alarmSettingMap = new HashMap<>();
```
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RunningRule.java:
##########
@@ -214,59 +219,59 @@ public List<AlarmMessage> check() {
windows.forEach((alarmEntity, window) -> {
if (window.isExpired()) {
expiredEntityList.add(alarmEntity);
- return;
+ if (log.isTraceEnabled()) {
+ log.trace("RuleName:{} AlarmEntity {} {} {} expired",
ruleName, alarmEntity.getName(),
+ alarmEntity.getId0(), alarmEntity.getId1());
+ }
Review Comment:
[nitpick] The expired entities are being logged but then removed from the
window. The removal happens after the forEach completes. Consider adding a
return statement after logging to skip further processing of expired entities
in the same iteration.
```suggestion
}
return;
```
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RunningRule.java:
##########
@@ -447,6 +475,115 @@ private void init() {
values.add(null);
}
}
+
+ public class AlarmStateMachine {
+ @Getter
+ private int silenceCountdown;
+ @Getter
+ private int recoveryObservationCountdown;
+ private final int silencePeriod;
+ private final int recoveryObservationPeriod;
+ @Getter
+ private State currentState;
+
+ public AlarmStateMachine(int silencePeriod, int
recoveryObservationPeriod) {
+ this.currentState = State.NORMAL;
+ this.silencePeriod = silencePeriod;
+ this.recoveryObservationPeriod = recoveryObservationPeriod;
+ this.silenceCountdown = -1;
+ this.recoveryObservationCountdown = recoveryObservationPeriod;
+ }
+
+ public void onMatch() {
+ if (log.isTraceEnabled()) {
+ if (log.isTraceEnabled()) {
+ log.trace("RuleName:{} AlarmEntity {} {} {} onMatch
silenceCountdown:{} currentState:{}",
+ ruleName, entity.getName(), entity.getId0(),
entity.getId1(), silenceCountdown, currentState);
+ }
Review Comment:
Duplicate `if (log.isTraceEnabled())` check on lines 498 and 499. Remove the
inner duplicate check.
```suggestion
log.trace("RuleName:{} AlarmEntity {} {} {} onMatch
silenceCountdown:{} currentState:{}",
ruleName, entity.getName(), entity.getId0(),
entity.getId1(), silenceCountdown, currentState);
```
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -226,11 +246,13 @@ See the following example:
"scopeId": 1,
"scope": "SERVICE",
"name": "serviceA",
+ "uuid": "uuid1",
"id0": "12",
"id1": "",
- "ruleName": "service_resp_time_rule",
+ "ruleName": "service_resp_time_rule",
"alarmMessage": "alarmMessage xxxx",
"startTime": 1560524171000,
+ "recoveryTime": 15596606810000,
Review Comment:
The example recovery timestamp 15596606810000 appears to be in the future
(approximately year 2464). This should be a realistic timestamp that comes
after the startTime value of 1560524171000.
```suggestion
"recoveryTime": 1560524271000,
```
--
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]
