You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "youjie23 (via GitHub)" <gi...@apache.org> on 2025/10/11 10:03:52 UTC
[PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
youjie23 opened a new pull request, #13539:
URL: https://github.com/apache/skywalking/pull/13539
- **Add** alarm recovery detection with a recovery-observation-period(default 0).
- **Store** the alarm recovery record with the same UUID as the related alarm record.
- **Notify** hooks using a recovery-text-template. The default template is '[Recovered]' + text-template, and the notification includes the recoveryTime.
Submodule PR:
- [skywalking-booster-ui#505](https://github.com/apache/skywalking-booster-ui/pull/505)
- [skywalking-query-protocol#153](https://github.com/apache/skywalking-query-protocol/pull/153)
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [x] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #13492.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3508505471
> Please update the alarm docs accordingly, about how recovery works, what are recovery notification APIs, and how to set those up
I have updated the corresponding description in the `backend-alarm.md` document. Please review it at your convenience.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3521153182
> Let's add the different cases in the UT and check if the alarm window status changes as expected:
>
> 1. `silencePeriod` and `recoveryObservationPeriod` are not set.
> 2. Only set `silencePeriod`.
> 3. Only set `recoveryObservationPeriod`.
> 4. `silencePeriod` > `recoveryObservationPeriod`.
> 5. `recoveryObservationPeriod` > `silencePeriod`.
>
> The status changes should include the `AlarmStateMachine` current status after each `match` or `misMatch`
I have added the unit tests to cover all the different cases you mentioned. The tests now verify the status changes of the AlarmStateMachineafter each match and misMatch.
The changes are in RunningRuleTest. Please review when you have time. Thanks!
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3497075145
Please update the alarm docs accordingly, about how recovery works, what are recovery notification APIs, and how to set those up
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2498222596
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RunningRule.java:
##########
@@ -447,6 +478,119 @@ 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;
+ 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);
+ }
+ }
+ silenceCountdown--;
+ switch (currentState) {
+ case NORMAL:
+ case SILENCED:
+ case OBSERVING_RECOVERY:
+ case RECOVERED:
+ if (silenceCountdown < 0) {
+ transitionTo(State.FIRING);
+ }
+ break;
+ case FIRING:
+ if (silenceCountdown >= 0) {
+ transitionTo(State.SILENCED);
+ }
+ break;
+ default:
+ break;
+ }
+ }
+
+ public void onMismatch() {
+ if (log.isTraceEnabled()) {
+ log.trace("RuleName:{} AlarmEntity {} {} {} onMismatch silenceCountdown:{} " +
+ "recoveryObservationCountdown:{} currentState:{}",
+ ruleName, entity.getName(), entity.getId0(), entity.getId1(), silenceCountdown,
+ recoveryObservationCountdown, currentState);
+ }
+ recoveryObservationCountdown--;
+ silenceCountdown--;
+ switch (currentState) {
+ case FIRING:
+ case SILENCED:
+ if (this.recoveryObservationCountdown < 0) {
+ transitionTo(State.RECOVERED);
+ } else {
+ transitionTo(State.OBSERVING_RECOVERY);
+ }
+ break;
+ case OBSERVING_RECOVERY:
+ if (recoveryObservationCountdown < 0) {
+ transitionTo(State.RECOVERED);
+ }
+ break;
+ case RECOVERED:
+ transitionTo(State.NORMAL);
+ break;
+ case NORMAL:
+ default:
+ break;
+ }
+ }
+
+ private void transitionTo(State newState) {
+ if (log.isTraceEnabled()) {
+ log.trace("RuleName:{} AlarmEntity {} {} {} transitionTo newState:{}",
+ ruleName, entity.getName(), entity.getId0(), entity.getId1(), newState);
+ }
+ this.currentState = newState;
+ switch (newState) {
+ case NORMAL:
+ resetCountdowns();
+ break;
+ case FIRING:
+ this.silenceCountdown = this.silencePeriod;
+ this.recoveryObservationCountdown = recoveryObservationPeriod;
+ break;
+ case SILENCED:
+ break;
+ case OBSERVING_RECOVERY:
+ this.recoveryObservationCountdown = this.recoveryObservationPeriod - 1;
+ //this.silenceCountdown = -1;
Review Comment:
Remove commented-out code. If this assignment is intentionally excluded, add a comment explaining why; otherwise, remove the commented line entirely.
```suggestion
```
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RunningRule.java:
##########
@@ -447,6 +478,119 @@ 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;
+ 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);
+ }
+ }
+ silenceCountdown--;
+ switch (currentState) {
+ case NORMAL:
+ case SILENCED:
+ case OBSERVING_RECOVERY:
+ case RECOVERED:
+ if (silenceCountdown < 0) {
+ transitionTo(State.FIRING);
+ }
+ break;
+ case FIRING:
+ if (silenceCountdown >= 0) {
+ transitionTo(State.SILENCED);
+ }
+ break;
+ default:
+ break;
+ }
+ }
+
+ public void onMismatch() {
+ if (log.isTraceEnabled()) {
+ log.trace("RuleName:{} AlarmEntity {} {} {} onMismatch silenceCountdown:{} " +
+ "recoveryObservationCountdown:{} currentState:{}",
+ ruleName, entity.getName(), entity.getId0(), entity.getId1(), silenceCountdown,
+ recoveryObservationCountdown, currentState);
+ }
+ recoveryObservationCountdown--;
+ silenceCountdown--;
+ switch (currentState) {
+ case FIRING:
+ case SILENCED:
+ if (this.recoveryObservationCountdown < 0) {
+ transitionTo(State.RECOVERED);
+ } else {
+ transitionTo(State.OBSERVING_RECOVERY);
+ }
+ break;
+ case OBSERVING_RECOVERY:
+ if (recoveryObservationCountdown < 0) {
+ transitionTo(State.RECOVERED);
+ }
+ break;
+ case RECOVERED:
+ transitionTo(State.NORMAL);
+ break;
+ case NORMAL:
+ default:
+ break;
+ }
+ }
+
+ private void transitionTo(State newState) {
+ if (log.isTraceEnabled()) {
+ log.trace("RuleName:{} AlarmEntity {} {} {} transitionTo newState:{}",
+ ruleName, entity.getName(), entity.getId0(), entity.getId1(), newState);
+ }
+ this.currentState = newState;
+ switch (newState) {
+ case NORMAL:
+ resetCountdowns();
+ break;
+ case FIRING:
+ this.silenceCountdown = this.silencePeriod;
+ this.recoveryObservationCountdown = recoveryObservationPeriod;
+ break;
+ case SILENCED:
+ break;
+ case OBSERVING_RECOVERY:
+ this.recoveryObservationCountdown = this.recoveryObservationPeriod - 1;
+ //this.silenceCountdown = -1;
+ break;
+ case RECOVERED:
+ this.recoveryObservationCountdown = this.recoveryObservationPeriod;
+ break;
+ }
+ }
+
+ private void resetCountdowns() {
+ //silenceCountdown = -1;
Review Comment:
Remove commented-out code. If this logic should not be included, delete the commented line rather than leaving it in the codebase.
```suggestion
```
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RunningRule.java:
##########
@@ -214,59 +219,60 @@ 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());
+ }
+ //return;
Review Comment:
Remove the commented-out `return` statement. The logic has changed to continue processing expired entities for potential recovery notifications, so this commented code is misleading.
```suggestion
```
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508549015
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
Thanks for the review.
Yes, I also agree that the upgrade should not cause any additional hassle for existing users. I have already addressed this in the latest commit: if neither recovery-text-template nor recovery-urls is configured, no recovery notification will be sent externally (though it will still be persisted to storage), and this will not affect the project's startup or upgrade process.
For example,the key logic in WebhookCallback is as follows: it checks the configured URLs using the getUrls method. For recovery notifications, it specifically uses setting.getRecoveryUrls(). If this list is empty (i.e., not configured), the loop for (final var url : urls)will not execute, thus no external notification is sent.
```
@Override
public void doAlarmCallback(List<AlarmMessage> alarmMessages, boolean isRecovery) throws Exception {
// ... existing setup code ...
List<String> urls = getUrls(setting, isRecovery);
if (setting == null || CollectionUtils.isEmpty(urls) || CollectionUtils.isEmpty(messages)) {
continue; // This is where it skips sending if URLs are empty
}
for (final var url : urls) {
// ... send message ...
}
}
private static List<String> getUrls(WebhookSettings setting, boolean isRecovery) {
return isRecovery ? setting.getRecoveryUrls() : setting.getUrls(); // Returns an empty list if not configured
}
```
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3538934897
> @youjie23 [apache/skywalking-booster-ui#505](https://github.com/apache/skywalking-booster-ui/pull/505) the UI has been merged, please sync the UI commit into this PR. Thanks.
Updated. Thanks。
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3418639248
> Another PR just passed all the tests and merged. I assume if there is anything wrong, it is in this change.
Thank you for the helpful feedback.
Fixed in the [skywalking-infra-e2e #133](https://github.com/apache/skywalking-infra-e2e/pull/133)
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3525745116
Would you please rerun the [CI / E2E test (Virtual MQ, test/e2e-v2/cases/virtual-mq/e2e.yaml) (pull_request)](https://github.com/apache/skywalking/actions/runs/19320780403/job/55265643639?pr=13539) ？ I haven't made any changes related to it in my recent commits. Thanks. @wu-sheng
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508549015
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
Yes, I also agree that the upgrade should not cause any additional hassle for existing users. I have already addressed this in the latest commit: if neither recovery-text-templatenor recovery-urlsis configured, no recovery notification will be sent externally (though it will still be persisted to storage), and this will not affect the project's startup or upgrade process.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508155907
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RunningRule.java:
##########
@@ -447,6 +477,117 @@ 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;
+ 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;
Review Comment:
Thank you. You've correctly summarized the logic. As per #13492, the current plan is to recover the alarm immediately when conditions aren't met, with the observation period being configurable (default: 0) to avoid flapping. Does this approach seem reasonable to you? We can adjust if needed.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496109893
Let's revert them and rerun.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2431069554
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/BanyanDBAlarmQueryDAO.java:
##########
@@ -88,12 +93,35 @@ public void apply(StreamQuery query) {
AlarmRecord alarmRecord = builder.storage2Entity(
new BanyanDBConverter.StorageToStream(AlarmRecord.INDEX_NAME, rowEntity)
);
- AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord);
+ Long recoveryTime = getAlarmRecoveryTime(alarmRecord.getUuid(), duration);
+ AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord, recoveryTime);
Review Comment:
It appears that the e2e test job on the GitHub Actions workflow was blocked and then got canceled. I'm not entirely sure if this is an issue on my end. I've sampled all the alarm e2e tests and some other tests that were not marked as completed; they all seemed to have passed verification. Is there anything I need to do on my side to allow them to run to completion?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3454411577
I recommend you focusing on your new scenarios, and e2e tooling PRs first. Once they are passed, we could take care of others later.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 merged PR #13539:
URL: https://github.com/apache/skywalking/pull/13539
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3509651129
I will cut the release of 10.3 today. Let's make this as a part of key of 10.4
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508549015
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
Yes, I also agree that the upgrade should not cause any additional hassle for existing users. I have already addressed this in the latest commit: if neither recovery-text-templatenor recovery-urls is configured, no recovery notification will be sent externally (though it will still be persisted to storage), and this will not affect the project's startup or upgrade process.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3509315855
@wankai123 Let's propose a set of UTs to check alarm status changes, e.g. from alarming -> silence -> recovery.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3404941105
> Please fix CI.
It appears that the e2e test job on the GitHub Actions workflow was blocked and then got canceled. I've sampled all the alarm e2e tests and some other tests that were not marked as completed; they all seemed to have passed verification. Could you please spare a moment to guide me on what I need to do to get them to run to completion?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2422858087
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/BanyanDBAlarmQueryDAO.java:
##########
@@ -88,12 +93,35 @@ public void apply(StreamQuery query) {
AlarmRecord alarmRecord = builder.storage2Entity(
new BanyanDBConverter.StorageToStream(AlarmRecord.INDEX_NAME, rowEntity)
);
- AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord);
+ Long recoveryTime = getAlarmRecoveryTime(alarmRecord.getUuid(), duration);
+ AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord, recoveryTime);
Review Comment:
I have concerns about the way you are doing this. Querying status from a list usually results a bad performance.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496631090
> Please fix them. Review this still takes some time.
Fixed.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3487567156
> Logs are available in this page, https://github.com/apache/skywalking/actions/runs/18618177572. You need to scroll down to the very botton of the page.
Thank you for help.
I pulled the latest SkyWalking code and the corresponding e2e version code locally but was unable to reproduce the verification error observed in GHA. According to [runs](https://github.com/apache/skywalking/actions/runs/18958284652?pr=13539), the failures are all related to MySQL/PostgreSQL cases (some of which are unrelated to alarm). Based on the uploaded OAP and other logs, it appears that certain JDBC queries for specific tables were not initiated during the execution of the cases. After reviewing some potential code paths, I would like to inquire if I can add some debug-level logs to aid in troubleshooting the issue. Correspondingly, I have also submitted a [PR ](https://github.com/apache/skywalking-infra-e2e/pull/136)in the skywalking-e2e repository.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2430968385
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/BanyanDBAlarmQueryDAO.java:
##########
@@ -88,12 +93,35 @@ public void apply(StreamQuery query) {
AlarmRecord alarmRecord = builder.storage2Entity(
new BanyanDBConverter.StorageToStream(AlarmRecord.INDEX_NAME, rowEntity)
);
- AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord);
+ Long recoveryTime = getAlarmRecoveryTime(alarmRecord.getUuid(), duration);
+ AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord, recoveryTime);
Review Comment:
Thank you for the helpful feedback. I've pushed new commits to address the points you raised. Please take another look when you have a moment, and let me know if anything else needs adjustment.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2525397168
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
@youjie23 how many tests were failed in this PR if you don’t change the `times` to -1?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2532326366
##########
docs/en/changes/changes.md:
##########
@@ -5,6 +5,7 @@
#### OAP Server
* KubernetesCoordinator: make self instance return real pod IP address instead of `127.0.0.1`.
+* Enhance the alarm kernel with recovered status notification capability
#### UI
Review Comment:
```suggestion
#### UI
* Fix the missing icon in new native trace view.
```
According to https://github.com/apache/skywalking-booster-ui/compare/30927258d66934278a401b2defa0d9592e7d1974...6eaf7fe26da704cf54d1371ac489b3c8f458fbb8, this submodule update includes two commits.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3419239919
They are not cancelled this tine, but failed.
please take a look.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3393190569
Apologies for the oversight. While merging the latest master code, the @BanyanDB.Groupannotation in the AlarmRecoveryRecordclass was accidentally missed, which caused the e2e test failure @wankai123 @wu-sheng
I will fix it immediately and re-run the tests.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496146777
> Let's revert them and rerun.
Reverted.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496038651
Could you revert the recent changes of adding logs? Those tables should exist as they are initialized.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508410251
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
Please the make the codes to support recovery period as -1 as no recovery rules. And support no `recovery-text-template` in hooks as no need to send recovery notifications.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2525580840
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
There are a total of 36 failed[ GHA cases ](https://github.com/apache/skywalking/actions/runs/18618177572), but some are related to the ES container startup, so we cannot determine the exact number.
I can roll back this change for all cases except the alarm-related ones. Then, we can rerun the GHA with the latest code, and I will adjust the configurations for the failed cases accordingly.
Does that sound acceptable to everyone? @wu-sheng @kezhenxu94 @wankai123
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2532330448
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -430,15 +501,37 @@ the sliding window will be destroyed and re-created, causing the Alarm of this s
### Keys with data types of alerting rule configuration file
-| Alerting element | Configuration property key | Type | Description |
-|----------------------|----------------------------|----------------|--------------------|
-| Expression | expression | string | MQE expression |
-| Include names | include-names | string array | |
-| Exclude names | exclude-names | string array | |
-| Include names regex | include-names-regex | string | Java regex Pattern |
-| Exclude names regex | exclude-names-regex | string | Java regex Pattern |
-| Tags | tags | key-value pair | |
-| Period | Period | int | |
-| Silence period | silence-period | int | |
-| Message | message | string | |
-| Hooks | hooks | string array | |
+| Alerting element | Configuration property key | Type | Description |
+| --------------------------- | --------------------------- | -------------- | ------------------ |
+| Expression | expression | string | MQE expression |
+| Include names | include-names | string array | |
+| Exclude names | exclude-names | string array | |
+| Include names regex | include-names-regex | string | Java regex Pattern |
+| Exclude names regex | exclude-names-regex | string | Java regex Pattern |
+| Tags | tags | key-value pair | |
+| Period | period | int | |
+| Silence period | silence-period | int | |
+| Recovery observation period | recovery-observation-period | int | |
+| Message | message | string | |
+| Hooks | hooks | string array | |
+
+## Alarm state transition
+The overall alarm state transition after the introduction of alarm restoration detection and notification since version 10.4.0 is as follows:
+```mermaid
Review Comment:
Recommended English term for triggering event.
- Expression evaluated true
- Expression true, but in silence period
- Expression evaluated false
- Expression false, in recovery window
And from the preview page, you can see, these two labels are hard to tell from which transition.
<img width="1897" height="1379" alt="Image" src="https://github.com/user-attachments/assets/2cf2f7b6-b746-4ecb-a22f-435117c89f6f" />
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2501473417
##########
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());
+ }
}
Optional<AlarmMessage> alarmMessageOptional = window.checkAlarm();
- if (alarmMessageOptional.isPresent()) {
- AlarmMessage alarmMessage = alarmMessageOptional.get();
- alarmMessage.setScopeId(alarmEntity.getScopeId());
- alarmMessage.setScope(alarmEntity.getScope());
- alarmMessage.setName(alarmEntity.getName());
- alarmMessage.setId0(alarmEntity.getId0());
- alarmMessage.setId1(alarmEntity.getId1());
- alarmMessage.setRuleName(this.ruleName);
- alarmMessage.setAlarmMessage(formatter.format(alarmEntity));
- alarmMessage.setStartTime(System.currentTimeMillis());
- alarmMessage.setPeriod(this.period);
- alarmMessage.setTags(this.tags);
- alarmMessage.setHooks(this.hooks);
- alarmMessage.setExpression(expression);
- alarmMessage.setMqeMetricsSnapshot(window.mqeMetricsSnapshot);
- alarmMessageList.add(alarmMessage);
- }
+ alarmMessageOptional.ifPresent(alarmMessageList::add);
});
expiredEntityList.forEach(windows::remove);
return alarmMessageList;
}
+ public enum State {
+ NORMAL,
+ FIRING,
+ SILENCED,
+ OBSERVING_RECOVERY,
+ RECOVERED
+ }
+
/**
* A metrics window, based on AlarmRule#period. This window slides with time, just keeps the recent N(period)
* buckets.
*/
public class Window {
+
@Getter
private LocalDateTime endTime;
@Getter
private final int additionalPeriod;
@Getter
private final int size;
@Getter
- private int silenceCountdown;
+ private final int period;
+ @Getter
+ private final AlarmStateMachine stateMachine;
private LinkedList<Map<String, Metrics>> values;
private ReentrantLock lock = new ReentrantLock();
+ private AlarmMessage lastAlarmMessage;
@Getter
private JsonObject mqeMetricsSnapshot;
private AlarmEntity entity;
- public Window(AlarmEntity entity, int period, int additionalPeriod) {
+ public Window(AlarmEntity entity, int period, int silencePeriod, int recoveryObservationPeriod,
+ int additionalPeriod) {
this.entity = entity;
this.additionalPeriod = additionalPeriod;
- this.size = period + additionalPeriod;
- // -1 means silence countdown is not running.
- silenceCountdown = -1;
- init();
+ this.size = period + additionalPeriod + Math.max(silencePeriod, recoveryObservationPeriod);
Review Comment:
Could you explain why you have to change the window size?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3453928808
Logs are available in this page, https://github.com/apache/skywalking/actions/runs/18618177572. You need to scroll down to the very botton of the page.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508596579
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
> Please the make the codes to support recovery period as -1 as no recovery rules. And support no `recovery-text-template` in hooks as no need to send recovery notifications.
Sorry. I didn't see this message when I submitted my last reply. I have already implemented the latter behavior (supporting no recovery-text-template in hooks). I'm not entirely sure if the first part (supporting recovery period as -1) is still required. The state transitions and separate storage should not introduce additional side effects, as operations like table creation are automatically handled during the startup process.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3404866353
Please fix CI.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2532087682
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
> I am fine to keep endless retry as they were to be like this. We could try to change that to limited numbers in another PR.
Noted. I've updated the references to the UI module in the latest commit. Once this PR is completed, I will create a new PR to follow up on the matter you mentioned. Thanks.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3540126365
Most others seem good to me. Please fix docs, and we could merge this now.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2422859717
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/stream/BanyanDBAlarmQueryDAO.java:
##########
@@ -88,12 +93,35 @@ public void apply(StreamQuery query) {
AlarmRecord alarmRecord = builder.storage2Entity(
new BanyanDBConverter.StorageToStream(AlarmRecord.INDEX_NAME, rowEntity)
);
- AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord);
+ Long recoveryTime = getAlarmRecoveryTime(alarmRecord.getUuid(), duration);
+ AlarmMessage alarmMessage = buildAlarmMessage(alarmRecord, recoveryTime);
Review Comment:
You should at least get the alarm list first. Then use the UUID list to retrieve the recovery list.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 closed pull request #13539: feat:Enhance the alarm kernel with recovered status notification capability for alarm rules
URL: https://github.com/apache/skywalking/pull/13539
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496627561
> fix
Fixed.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3405799362
Are you setting the recovery quickly enough? They are running for over one hour, and be cancelled due to preset timeout
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3509381380
> Also, with #13570 is going to be merged, this new status should be reflected into query APIs.
Please hold off on the CI pipeline due to compilation errors after merging master. I'm looking into the errors and will update everyone once it's fixed.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508408117
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
About the default value, we could change the default rules into 0, and are considered as immediately recovery. But for ppl don't have this config(previous versions' users), we are better to support `-1` as default value for config absent, which could provide a more consistent behaviour.
After all, you will send new notifications. The old confiiguation files don't have `recovery-text-template` or relative url, you should take care of them as normal cases. Otherwise, they are going to fail to boot and upgrade, then have to change all rules manually.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508596579
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
> Please the make the codes to support recovery period as -1 as no recovery rules. And support no `recovery-text-template` in hooks as no need to send recovery notifications.
Sorry. I didn't see this message when I submitted my last reply. I have already implemented the latter behavior (supporting no recovery-text-templatein hooks). I'm not entirely sure if the first part (supporting recovery period as -1) is still required. The state transitions and separate storage should not introduce additional side effects, as operations like table creation are automatically handled during the startup process.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496583590
Please fix them. Review this still takes some time.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3495990914
What did you change to make CI passed?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2520498811
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
Why change so many irrelevant e2e files, and set it to `-1`?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3530535955
@youjie23 https://github.com/apache/skywalking-booster-ui/pull/505 the UI has been merged, please sync the UI commit into this PR. Thanks.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3447575039
> They are not cancelled this tine, but failed. please take a look.
1. My analysis revealed that a subset of the problems were introduced as a side effect of the resolution for [skywalking-infra-e2e#133](https://github.com/apache/skywalking-infra-e2e/pull/133). In accordance with the consensus reached in discussion [skywalking-infra-e2e#134](https://github.com/apache/skywalking-infra-e2e/pull/134), I have modified the e2e.yaml to align its runtime behavior with the previous state.
1. Regarding other issues, for instance, the consumer container startup failure documented in [[E2E test (Storage ES 7.16.3, test/e2e-v2/cases/storage/es/e2e.yaml, ES_VERSION=7.16.3)](https://github.com/apache/skywalking/actions/runs/18618177572/job/53095660470#logs)], My attempts to reproduce the issue locally have been unsuccessful. I would like to highlight that no modifications were made to the code related to this component. At this stage, I am uncertain about the feasibility of accessing the relevant Docker logs. I would be grateful for any advice you might have on the recommended course of action.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496497026
> > Let's revert them and rerun.
After the revert, all e2e cases are now passing. Copilot reported 3 issues regarding the removal of commented-out code lines. Should I create a new commit to address these now, or would you prefer to review first?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3496097799
> What did you change to make CI passed?
I only added some debug-level logs to investigate why and when this collection of tables might be empty (which I believe was the cause of the previous CI failures), without modifying any other java code.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3393192172
Take your time.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3406418874
Another PR just passed all the tests and merged. I assume if there is anything wrong, it is in this change.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508549015
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
Thanks for the review.
Yes, I also agree that the upgrade should not cause any additional hassle for existing users. I have already addressed this in the latest commit: if neither recovery-text-templatenor recovery-urls is configured, no recovery notification will be sent externally (though it will still be persisted to storage), and this will not affect the project's startup or upgrade process.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3509623146
Let's add the different cases in the UT and check if the alarm window status changes as expected:
1. `silencePeriod` and `recoveryObservationPeriod` are not set.
2. Only set `silencePeriod`.
3. Only set `recoveryObservationPeriod`.
4. `silencePeriod` > `recoveryObservationPeriod`.
5. `recoveryObservationPeriod` > `silencePeriod`.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2525580840
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
There are a total of 36 failed[ GHA cases ](https://github.com/apache/skywalking/actions/runs/18618177572), but some are related to the ES container startup, so we cannot determine the exact number.
I can roll back this change for all cases except the alarm-related ones. Then, we can rerun the GHA with the latest code, and I will adjust the configurations for the failed cases accordingly.
How does that sound? Does that sound acceptable to everyone? @wu-sheng @kezhenxu94 @wankai123
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2525580840
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
There are a total of 36 failed[ GHA cases ](https://github.com/apache/skywalking/actions/runs/18618177572), but some are related to the ES container startup, so we cannot determine the exact number.
We can roll back this change for all cases except the alarm-related ones. Then, we can rerun the GHA with the latest code, and I will adjust the configurations for the failed cases accordingly.
Does that sound acceptable to everyone? @wu-sheng @kezhenxu94 @wankai123
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3524672768
The others LGTM
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2522661422
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
@kezhenxu94 could you clarify here? For keeping default behaviors consistently, it should mean nothing changed, right?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3540159923
> Most others seem good to me. Please fix docs, and we could merge this now.
Updated. Please review when you have time. Thanks.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508144732
##########
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());
+ }
}
Optional<AlarmMessage> alarmMessageOptional = window.checkAlarm();
- if (alarmMessageOptional.isPresent()) {
- AlarmMessage alarmMessage = alarmMessageOptional.get();
- alarmMessage.setScopeId(alarmEntity.getScopeId());
- alarmMessage.setScope(alarmEntity.getScope());
- alarmMessage.setName(alarmEntity.getName());
- alarmMessage.setId0(alarmEntity.getId0());
- alarmMessage.setId1(alarmEntity.getId1());
- alarmMessage.setRuleName(this.ruleName);
- alarmMessage.setAlarmMessage(formatter.format(alarmEntity));
- alarmMessage.setStartTime(System.currentTimeMillis());
- alarmMessage.setPeriod(this.period);
- alarmMessage.setTags(this.tags);
- alarmMessage.setHooks(this.hooks);
- alarmMessage.setExpression(expression);
- alarmMessage.setMqeMetricsSnapshot(window.mqeMetricsSnapshot);
- alarmMessageList.add(alarmMessage);
- }
+ alarmMessageOptional.ifPresent(alarmMessageList::add);
});
expiredEntityList.forEach(windows::remove);
return alarmMessageList;
}
+ public enum State {
+ NORMAL,
+ FIRING,
+ SILENCED,
+ OBSERVING_RECOVERY,
+ RECOVERED
+ }
+
/**
* A metrics window, based on AlarmRule#period. This window slides with time, just keeps the recent N(period)
* buckets.
*/
public class Window {
+
@Getter
private LocalDateTime endTime;
@Getter
private final int additionalPeriod;
@Getter
private final int size;
@Getter
- private int silenceCountdown;
+ private final int period;
+ @Getter
+ private final AlarmStateMachine stateMachine;
private LinkedList<Map<String, Metrics>> values;
private ReentrantLock lock = new ReentrantLock();
+ private AlarmMessage lastAlarmMessage;
@Getter
private JsonObject mqeMetricsSnapshot;
private AlarmEntity entity;
- public Window(AlarmEntity entity, int period, int additionalPeriod) {
+ public Window(AlarmEntity entity, int period, int silencePeriod, int recoveryObservationPeriod,
+ int additionalPeriod) {
this.entity = entity;
this.additionalPeriod = additionalPeriod;
- this.size = period + additionalPeriod;
- // -1 means silence countdown is not running.
- silenceCountdown = -1;
- init();
+ this.size = period + additionalPeriod + Math.max(silencePeriod, recoveryObservationPeriod);
Review Comment:
Thanks for the review. This change is no longer needed with the latest logic and I've removed it in a new commit.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2501504120
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/RunningRule.java:
##########
@@ -447,6 +477,117 @@ 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;
+ 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;
Review Comment:
If `recoveryObservationPeriod` is not set, according to the following logic, I think the alarm will trigger the recovery immediately if not match? We need a compatible logical if the user didn't config the recovery settings.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2508549015
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -39,6 +39,9 @@ The metrics names in the expression could be found in the [list of all potential
If the hook name is not specified, the global hook will be used.
- **Silence period**. After the alarm is triggered at Time-N (TN), there will be silence during the **TN -> TN + period**.
By default, it works in the same manner as **period**. The same Alarm (having the same ID in the same metrics name) may only be triggered once within a period.
+- **Recovery observation period**. Defines the number of consecutive periods that the alarm condition must remain false before the alarm is considered recovered. When the alarm condition becomes false, the system enters an observation period. If the condition remains false for the specified number of periods, a recovery notification is sent. If the condition becomes true again during the observation period, the alarm returns to the FIRING state.
+The default value is 0, which means immediate recovery notification when the condition becomes false.
Review Comment:
Thanks for the review.
Yes, I also agree that the upgrade should not cause any additional hassle for existing users. I have already addressed this in the latest commit: if neither recovery-text-templatenor recovery-urls is configured, no recovery notification will be sent externally (though it will still be persisted to storage), and this will not affect the project's startup or upgrade process.
For example,the key logic in WebhookCallback is as follows: it checks the configured URLs using the getUrls method. For recovery notifications, it specifically uses setting.getRecoveryUrls(). If this list is empty (i.e., not configured), the loop for (final var url : urls)will not execute, thus no external notification is sent.
```
@Override
public void doAlarmCallback(List<AlarmMessage> alarmMessages, boolean isRecovery) throws Exception {
// ... existing setup code ...
List<String> urls = getUrls(setting, isRecovery);
if (setting == null || CollectionUtils.isEmpty(urls) || CollectionUtils.isEmpty(messages)) {
continue; // This is where it skips sending if URLs are empty
}
for (final var url : urls) {
// ... send message ...
}
}
private static List<String> getUrls(WebhookSettings setting, boolean isRecovery) {
return isRecovery ? setting.getRecoveryUrls() : setting.getUrls(); // Returns an empty list if not configured
}
```
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3509317414
Also, with https://github.com/apache/skywalking/pull/13570 is going to be merged, this new status should be reflected into query APIs.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#issuecomment-3405834897
> Are you setting the recovery quickly enough? They are running for over one hour, and be cancelled due to preset timeout
It seems unrelated to the test cases. I observed that some test cases had been verified successfully before the 18-minute mark, but the test did not continue running."
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2525386350
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
We need to modify the `times` in all places in this PR, as I suggested https://github.com/apache/skywalking-infra-e2e/pull/134#issuecomment-3436596334
It was a bug that `times` didn’t take effect in e2e, but in this repo, if setting `times` to 10 was intended, which means after 10 times the trigger should stop, then the tests should passed without changes, otherwise, setting times to 10 was wrong and should be updated to -1 like this PR did
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2531962213
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
I am fine to keep endless retry as they were to be like this.
We could try to change that to limited numbers in another PR.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2520905352
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
The reason for updating these e2e files is that in previous versions, the e2e test did not stop the HTTP action when expected. It continued running until the entire test ended. This issue has been fixed in Pull Requests [#132](https://github.com/apache/skywalking-infra-e2e/pull/132) and [#134](https://github.com/apache/skywalking-infra-e2e/pull/132) . Therefore, to maintain the original behavior, we need to set this value to -1 in the e2e.yaml configuration.
We can explore different ways to implement this if you have suggestions. Thanks. @wankai123
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat:Enhance the alarm kernel with recovered status notification capability for alarm rules [skywalking]
Posted by "youjie23 (via GitHub)" <gi...@apache.org>.
youjie23 commented on code in PR #13539:
URL: https://github.com/apache/skywalking/pull/13539#discussion_r2520905352
##########
test/e2e-v2/cases/apisix/otel-collector/e2e.yaml:
##########
@@ -31,7 +31,7 @@ setup:
trigger:
action: http
interval: 3s
- times: 10
+ times: -1
Review Comment:
The reason for updating these e2e files is that in previous versions, the e2e test did not stop the HTTP action when expected. It continued running until the entire test ended. This issue has been fixed in Pull Requests [#132](https://github.com/apache/skywalking-infra-e2e/pull/132) and [#134](https://github.com/apache/skywalking-infra-e2e/pull/132) . Therefore, to maintain the original behavior, we need to set this value to -1 in the e2e.yaml configuration.
We can explore different ways to implement this if you have suggestions. Thanks.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org