kezhenxu94 commented on code in PR #11137:
URL: https://github.com/apache/skywalking/pull/11137#discussion_r1275663263
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -223,85 +271,95 @@ message KeyStringValuePair {
}
```
-## Slack Chat Hook
+### Slack Chat Hook
Follow the [Getting Started with Incoming Webhooks
guide](https://api.slack.com/messaging/webhooks) and create new Webhooks.
The alarm message will be sent through HTTP post by `application/json` content
type if you have configured Slack Incoming Webhooks as follows:
```yml
slackHooks:
- textTemplate: |-
+ default:
+ isGlobal: true
+ textTemplate: |-
{
"type": "section",
"text": {
"type": "mrkdwn",
"text": ":alarm_clock: *Apache Skywalking Alarm* \n **%s**."
}
}
Review Comment:
```suggestion
{
"type": "section",
"text": {
"type": "mrkdwn",
"text": ":alarm_clock: *Apache Skywalking Alarm* \n **%s**."
}
}
```
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -223,85 +271,95 @@ message KeyStringValuePair {
}
```
-## Slack Chat Hook
+### Slack Chat Hook
Follow the [Getting Started with Incoming Webhooks
guide](https://api.slack.com/messaging/webhooks) and create new Webhooks.
The alarm message will be sent through HTTP post by `application/json` content
type if you have configured Slack Incoming Webhooks as follows:
```yml
slackHooks:
- textTemplate: |-
+ default:
+ isGlobal: true
+ textTemplate: |-
{
"type": "section",
"text": {
"type": "mrkdwn",
"text": ":alarm_clock: *Apache Skywalking Alarm* \n **%s**."
}
}
- webhooks:
+ webhooks:
- https://hooks.slack.com/services/x/y/z
```
-## WeChat Hook
+### WeChat Hook
Note that only the WeChat Company Edition (WeCom) supports WebHooks. To use
the WeChat WebHook, follow the [Wechat Webhooks
guide](https://work.weixin.qq.com/help?doc_id=13376).
The alarm message will be sent through HTTP post by `application/json` content
type after you have set up Wechat Webhooks as follows:
```yml
wechatHooks:
- textTemplate: |-
+ default:
+ isGlobal: true
+ textTemplate: |-
{
"msgtype": "text",
"text": {
"content": "Apache SkyWalking Alarm: \n %s."
}
}
- webhooks:
+ webhooks:
- https://qyapi.weixin.qq.com/cgi-bin/webhook/send?key=dummy_key
```
-## DingTalk Hook
+### DingTalk Hook
Follow the [Dingtalk Webhooks
guide](https://ding-doc.dingtalk.com/doc#/serverapi2/qf2nxq/uKPlK) and create
new Webhooks.
You can configure an optional secret for an individual webhook URL for
security purposes.
The alarm message will be sent through HTTP post by `application/json` content
type if you have configured DingTalk Webhooks as follows:
```yml
dingtalkHooks:
- textTemplate: |-
+ default:
+ isGlobal: true
+ textTemplate: |-
{
"msgtype": "text",
"text": {
"content": "Apache SkyWalking Alarm: \n %s."
}
}
Review Comment:
```suggestion
{
"msgtype": "text",
"text": {
"content": "Apache SkyWalking Alarm: \n %s."
}
}
```
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/webhook/WebhookCallback.java:
##########
@@ -39,13 +41,24 @@ public class WebhookCallback extends HttpAlarmCallback {
private final Gson gson = new Gson();
@Override
- public void doAlarm(List<AlarmMessage> alarmMessage) throws IOException,
InterruptedException {
- if (alarmRulesWatcher.getWebHooks().isEmpty()) {
+ public void doAlarm(List<AlarmMessage> alarmMessages) throws IOException,
InterruptedException {
+ Map<String, WebhookSettings> settingsMap =
alarmRulesWatcher.getWebHooks();
+ if (settingsMap == null || settingsMap.isEmpty()) {
return;
}
- for (String url : alarmRulesWatcher.getWebHooks()) {
- post(URI.create(url), gson.toJson(alarmMessage), Map.of());
+ Map<String, List<AlarmMessage>> groupedMessages =
groupMessagesByHook(alarmMessages);
+ for (Map.Entry<String, List<AlarmMessage>> entry :
groupedMessages.entrySet()) {
+ var hookName = entry.getKey();
+ var messages = entry.getValue();
+ var setting = settingsMap.get(hookName);
Review Comment:
The `hookName` from the `groupMessagesByHook(alarmMessages);` is something
like `slackHooks.custom1`, but the keys in `settingsMap` is something like
`custom1`, how can you get the setting with `settingsMap.get(hookName);`? Did
you process the hook name somewhere I missed?
And the tests only covers global webhooks, please test the rule-level web
hooks
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/AlarmHookSettings.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.core.alarm.provider;
+
+import lombok.Getter;
+import org.apache.skywalking.oap.server.core.Const;
+
+public abstract class AlarmHookSettings {
+ private final String name;
+ @Getter
+ private final AlarmHooksType type;
+ @Getter
+ private final boolean isGlobal;
+
+ public AlarmHookSettings(String name, AlarmHooksType type, boolean
isGlobal) {
+ this.name = name;
+ this.type = type;
+ this.isGlobal = isGlobal;
+ }
Review Comment:
Replace with `@RequiredArgsConstructor`
##########
docs/en/setup/backend/backend-alarm.md:
##########
@@ -223,85 +271,95 @@ message KeyStringValuePair {
}
```
-## Slack Chat Hook
+### Slack Chat Hook
Follow the [Getting Started with Incoming Webhooks
guide](https://api.slack.com/messaging/webhooks) and create new Webhooks.
The alarm message will be sent through HTTP post by `application/json` content
type if you have configured Slack Incoming Webhooks as follows:
```yml
slackHooks:
- textTemplate: |-
+ default:
+ isGlobal: true
+ textTemplate: |-
{
"type": "section",
"text": {
"type": "mrkdwn",
"text": ":alarm_clock: *Apache Skywalking Alarm* \n **%s**."
}
}
- webhooks:
+ webhooks:
- https://hooks.slack.com/services/x/y/z
```
-## WeChat Hook
+### WeChat Hook
Note that only the WeChat Company Edition (WeCom) supports WebHooks. To use
the WeChat WebHook, follow the [Wechat Webhooks
guide](https://work.weixin.qq.com/help?doc_id=13376).
The alarm message will be sent through HTTP post by `application/json` content
type after you have set up Wechat Webhooks as follows:
```yml
wechatHooks:
- textTemplate: |-
+ default:
+ isGlobal: true
+ textTemplate: |-
{
"msgtype": "text",
"text": {
"content": "Apache SkyWalking Alarm: \n %s."
}
}
Review Comment:
```suggestion
{
"msgtype": "text",
"text": {
"content": "Apache SkyWalking Alarm: \n %s."
}
}
```
##########
oap-server/server-starter/src/main/resources/alarm-settings.yml:
##########
@@ -41,81 +41,128 @@ rules:
tags:
level: WARNING
-webhooks:
-# - http://127.0.0.1/notify/
-# - http://127.0.0.1/go-wechat/
-
-gRPCHook:
-# target_host: 127.0.0.1
-# target_port: 9888
-
-slackHooks:
- textTemplate: |-
- {
- "type": "section",
- "text": {
- "type": "mrkdwn",
- "text": ":alarm_clock: *Apache Skywalking Alarm* \n **%s**."
- }
- }
- webhooks:
-# - https://hooks.slack.com/services/x/y/zssss
-
-wechatHooks:
- textTemplate: |-
- {
- "msgtype": "text",
- "text": {
- "content": "Apache SkyWalking Alarm: \n %s."
- }
- }
- webhooks:
-# - https://qyapi.weixin.qq.com/cgi-bin/webhook/send?key=dummy_key
-
-dingtalkHooks:
- textTemplate: |-
- {
- "msgtype": "text",
- "text": {
- "content": "Apache SkyWalking Alarm: \n %s."
- }
- }
- webhooks:
-# - url: https://oapi.dingtalk.com/robot/send?access_token=dummy_token
-# secret: dummysecret
-
-feishuHooks:
- textTemplate: |-
- {
- "msg_type": "text",
- # at someone with feishu_user_ids
- # "ats": "feishu_user_id_1,feishu_user_id_2",
- "content": {
- "text": "Apache SkyWalking Alarm: \n %s."
- }
- }
- webhooks:
-# - url: https://open.feishu.cn/open-apis/bot/v2/hook/dummy_token
-# secret: dummysecret
-
-welinkHooks:
- textTemplate: "Apache SkyWalking Alarm: \n %s."
- webhooks:
-# # you may find your own client_id and client_secret in your app, below
are dummy, need to change.
-# - client_id: "dummy_client_id"
-# client_secret: dummy_secret_key
-# access_token_url:
https://open.welink.huaweicloud.com/api/auth/v2/tickets
-# message_url:
https://open.welink.huaweicloud.com/api/welinkim/v1/im-service/chat/group-chat
-# # if you send to multi group at a time, separate group_ids with commas,
e.g. "123xx","456xx"
-# group_ids: "dummy_group_id"
-# # make a name you like for the robot, it will display in group
-# robot_name: robot
+ service_cpm_rule:
+ metrics-name: service_cpm
+ # [Optional] Default, match all services in this metrics
+ threshold: 1
+ op: ">"
+ period: 10
+ count: 1
+ tags:
+ level: WARNING
-pagerDutyHooks:
- textTemplate: "Apache SkyWalking Alarm: \n %s."
- integrationKeys:
-# # you can find your integration key(s) on the Events API V2 integration
page for your PagerDuty service(s).
-# # (you may need to create an Events API V2 integration for your PagerDuty
service if you don't have one yet)
-# # below are dummy keys that should be replaced with your own integration
keys.
-# - dummy_key
-# - dummy_key2
+hooks:
Review Comment:
Why do we want to add a section level `hooks` here? The following sections
all have a `**hooks` suffix already and this looks pretty verbose
##########
oap-server/server-alarm-plugin/src/main/java/org/apache/skywalking/oap/server/core/alarm/provider/discord/DiscordHookCallback.java:
##########
@@ -43,17 +44,28 @@ public class DiscordHookCallback extends HttpAlarmCallback {
*/
@Override
public void doAlarm(List<AlarmMessage> alarmMessages) throws Exception {
- final var discordSettings = alarmRulesWatcher.getDiscordSettings();
- if (discordSettings == null ||
discordSettings.getWebhooks().isEmpty()) {
+ Map<String, DiscordSettings> settingsMap =
alarmRulesWatcher.getDiscordSettings();
+ if (settingsMap == null || settingsMap.isEmpty()) {
return;
}
- for (final var webHookUrl : discordSettings.getWebhooks()) {
- for (final var alarmMessage : alarmMessages) {
- final var content = String.format(
- discordSettings.getTextTemplate(),
+
+ Map<String, List<AlarmMessage>> groupedMessages =
groupMessagesByHook(alarmMessages);
+ for (Map.Entry<String, List<AlarmMessage>> entry :
groupedMessages.entrySet()) {
+ var hookName = entry.getKey();
+ var messages = entry.getValue();
+ var setting = settingsMap.get(hookName);
+ if (setting == null ||
CollectionUtils.isEmpty(setting.getWebhooks()) || CollectionUtils.isEmpty(
+ messages)) {
+ continue;
+ }
Review Comment:
You can't simply skip if `settings == null`, `settings` being null has high
possibility that the users configure a wrong name that doesn't exist at all
(e.g. typo), you should check the hook names exist when reading the configs and
bail out as a startup exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
