-
Notifications
You must be signed in to change notification settings - Fork 997
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] Support Collector Alarm #2693
base: master
Are you sure you want to change the base?
Conversation
alertService.editAlertStatus(CommonConstants.ALERT_STATUS_CODE_SOLVED, alertIds); | ||
|
||
// Recovery notifications are generated only after an alarm has occurred | ||
alertService.addAlert(restoreAlert); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 hi, how about use alarmCommonReduce.reduceAndSendAlarm(restoreAlert)
here? We send it for subsequent alarm convergence, alarm silencing, and alarm storage processing. But it needs to be tested, because before it was monitoring related alarms, I don’t know whether it is compatible with collector alarm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I will try to reuse AlarmCommonReduce
.times(1) | ||
.build(); | ||
this.offlineAlertMap.put(identity, alert); | ||
this.alertService.addAlert(alert); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same +1
…zbeat into feat-collector-alarm
if (alert.getTags() != null && alert.getTags().containsKey(CommonConstants.TAG_COLLECTOR_NAME)){ | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi, here has a question that why ignore to send notify the collector alarm message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AlertNotifyHandler
andMonitor
are heavily coupled and the changes are large. Therefore, send notify
is skipped for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it👍 It is recommended that we can merge after version 1.6.1 and then see how to solve the coupling problem between monitor and alarm.
When will this PR plan be merged? |
pls fix the conflicts |
What's changed?
#2652
Checklist
Add or update API