Skip to content
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

[Task] <Extern Alarm Manage API add Aliyun API> #1320

Closed
2 tasks done
SurryChen opened this issue Nov 7, 2023 · 20 comments · Fixed by #1325 or #1326
Closed
2 tasks done

[Task] <Extern Alarm Manage API add Aliyun API> #1320

SurryChen opened this issue Nov 7, 2023 · 20 comments · Fixed by #1325 or #1326
Assignees
Milestone

Comments

@SurryChen
Copy link
Contributor

SurryChen commented Nov 7, 2023

Description

Extern Alarm Manage API add Aliyun API and we will optimize the encoding of this section.

Task List

  • optimize the encoding of how to add Extern Alarm Manage API
  • add Alibaba Cloud API based on this encoding
@SurryChen SurryChen self-assigned this Nov 7, 2023
@SurryChen SurryChen added this to the v1.4.3 milestone Nov 7, 2023
@tomsun28 tomsun28 added the good first issue Good for newcomers label Nov 7, 2023
@SurryChen
Copy link
Contributor Author

接入第三方告警API的代码重构

一、现存编码的问题

1、Controller层通用性比较差

如下图

image

假设我们需要进行增加一个阿里云的API,我们需要写一个跟这个腾讯云API差不多的方法,调用方法的结构也是大同小异,不同之处只有alertConvertTenCloudService.convert(alertReport)这一处,这样的写法有些赘余,需要进行优化。

2、Service层的功能不明确

如下图

image

Service是进行业务处理的,但是在这个Service类的方法只提供了一个String对应的API的BO实体的功能,并不是那么符合Service层的定义,需要进行优化。

二、优化的方案

1、关于问题1,提出以下解决方案

我们的目标是提高接口的复用性,那么首先第一个步就是,我们需要使用一个接口方法来承接不同第三方API的请求,所以我们需要进行对来源的判断。

方案 描述 灵活性 用户体验 开发成本
方案一 提供一个泛化接口,通过调用信息进行判断来源,比如说,HTTP协议里面的Origin字段,或者请求的消息内容中包含对应的类似字段 强依赖于提供方的请求去判断来源,灵活性比较欠缺 体验良好,无需根据对应的云服务去做API的区分 开发成本不大,但是需要去找更多请求信息,同时未必能实现
方案二 提供一个接口,使用RESTAPI中的占位符URL,可以获取到请求来源 灵活,来源判断只依赖于具体使用时的设置,代码层面上也比较灵活,无需重复定义类似的API 体验一般,因为无法自动识别云端进行适配,还需要用户去看文档了解应该使用哪一个API 开发成本低,只需要增加一下类型判断即可
方案二优 方案一优 方案二优

综合考虑,暂选择方案二。

2、关于问题2,提出以下解决方案

由于该Service功能不明显,所以以下方案提出的基础在于,取消该Service,并考虑将该转换能力放置在何处。

方案 描述 合理性 开发成本
方案一 将转换能力独立出来,作为convert包,该包的能力为将request请求包装为BO 对于结构来说,比较合理,因为转换是一个独立的能力,独立出一个单独的包,可以使用一些类的功能更加清晰,但是考虑到目前项目结构下,并没有类似的包,如果此时独立,会导致转换能力散落在项目多个包下,不便于管理 如果不考虑将历史转换能力进行收敛,那么开发成本较小
方案二 由于最终结果是将一个String对象转换为一个BO对象,所以我们可以给BO增加对应的构造器方法 构造器可以承担该部分的功能,同时将生成对象的能力或者说转换对象的能力收敛到BO中,在没有将转换能力独立出来的时候,是一个不错的考虑,类的功能更加明确 开发成本小
方案二优 方案二优

综上考虑,暂使用方案二。

三、优化的具体细节

由于接口的占位符的字符串与对应的BO实体有强依赖关系,所以可以使用枚举进行强制绑定,也方便进行类型校验,同时如果用户使用API类型错误的话,还可以给用户发送消息,提示某某API使用错误,正确的API有xxx、xxx,进一步让用户更方便使用。

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Code reconstruction for accessing third-party alarm API

1. Problems with existing coding

1. The Controller layer has poor versatility

As shown below

image

Suppose we need to add an Alibaba Cloud API. We need to write a method similar to the Tencent Cloud API. The structure of the calling method is also similar. The only difference is alertConvertTenCloudService.convert(alertReport), which is written like this Some are redundant and need to be optimized.

2. The function of the Service layer is unclear

As shown below

image

Service is for business processing, but the method in this Service class only provides a function of converting String to BO entity of the corresponding API, which is not in line with the definition of the Service layer and needs to be optimized.

2. Optimization plan

1. Regarding question 1, the following solutions are proposed

Our goal is to improve the reusability of the interface, so the first step is that we need to use an interface method to accept requests from different third-party APIs, so we need to judge the source.

Plan Description Flexibility User experience Development costs
Option 1 Provides a generalized interface to determine the source by calling information, for example, the Origin field in the HTTP protocol, or the requested message content contains corresponding similar fields Strongly dependent on the provider's request to determine the source, lack of flexibility The experience is good and there is no need to distinguish APIs based on the corresponding cloud services The development cost is not high, but you need to find more request information, which may not be possible at the same time
Option 2 Provide an interface to obtain the request source using the placeholder URL in REST API Flexible, source judgment only depends on the specific settings when used, and it is also flexible at the code level, without the need to repeatedly define similar APIs The experience is average, because it cannot automatically identify the cloud for adaptation, and users need to read the documentation to understand which API should be used The development cost is low, just need to add type judgment
The second best option The best plan The second best option

After comprehensive consideration, we temporarily choose option two.

2. Regarding question 2, the following solutions are proposed

Since the function of this Service is not obvious, the basis for the following proposal is to cancel the Service and consider where to place the conversion capability.

The
Plan Description Reasonability Development costs
Option 1 Isolate the conversion capability as a convert package. The ability of this package is to package the request request into BO In terms of structure, it is more reasonable, because conversion is an independent capability, and a separate package can be used to make the functions of some classes clearer. However, considering the current project structure, there is no similar package. If Being independent at this time will cause the conversion capabilities to be scattered under multiple packages of the project, making it inconvenient to manage If the convergence of historical conversion capabilities is not considered, the development cost will be smaller
Option 2 Since the final result is to convert a String object into a BO object, we can add the corresponding constructor method to BO constructor can assume this part of the function, and at the same time converge the ability to generate objects or convert objects into BO. This is a good consideration when the conversion ability is not separated, and the functions of the class are more clear. Low development cost
The second best option The second best option

Based on the above considerations, option 2 is temporarily used.

3. Specific details of optimization

Since the placeholder string of the interface has a strong dependency on the corresponding BO entity, enumeration can be used for forced binding, which is also convenient for type verification. At the same time, if the user uses the wrong API type, it can also be sent to the user. The message indicates that a certain API is used incorrectly. The correct APIs are xxx and xxx, further making it easier for users to use.

@tomsun28
Copy link
Contributor

tomsun28 commented Nov 7, 2023

LGTM👍,问题一:方案二+1,问题二:方案二+1
我们接收请求对象然后convert转换实体属性那块,能不能直接使用@JsonProperty类似这样的注解转换成实体属性哦

@JsonProperty("msg_type")
private String msgType 

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


LGTM👍, Question 1: Option 2 + 1, Question 2: Option 2 + 1
We receive the request object and then convert the entity properties. Can we directly use @JsonProperty annotations like this to convert them into entity properties?

@JsonProperty("msg_type")
private String msgType

@tomsun28 tomsun28 assigned zqr10159 and unassigned zqr10159 Nov 7, 2023
@tomsun28
Copy link
Contributor

tomsun28 commented Nov 7, 2023

how about this @zqr10159

@zqr10159
Copy link
Member

zqr10159 commented Nov 7, 2023

LGTM👍🏻

@SurryChen
Copy link
Contributor Author

LGTM👍,问题一:方案二+1,问题二:方案二+1 我们接收请求对象然后convert转换实体属性那块,能不能直接使用@JsonProperty类似这样的注解转换成实体属性哦

@JsonProperty("msg_type")
private String msgType 

以下注解属于jackson,使用的使用需要新建一个ObjectMapper对象进行转换,观感上不够简洁

@JsonProperty("msg_type")
private String msgType;

找了fastjson,也有类似的注解(如下),使用方式依旧是JSON.parseObject()的静态方法,比较喜欢这种方式

@JSONField(name = "id")
private String id;

我想使用一下第二种方式,不过项目中是否有要求需要统一使用某种转换方式呢,另外这两种方式都会创建一个新的实体对象,这样直接放在BO里面倒是看起来怪怪的,因为该方法不是静态方法,只是BO的一个能力,所以不太应该出现这种BO里面新建一个BO对象的情况,而是应该填充属性。

解决思路:第一种是通过BeanUtil,可以将生成的新的对象复制到this对象中,第二种是看看有没有别的工具可以进行属性映射填充而不需要生成新对象。

后续我去实际编码测试一下这处的实现细节有没有问题:smile:

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


LGTM👍, Question 1: Option 2 + 1, Question 2: Option 2 + 1 We receive the request object and then convert the entity attributes. Can we directly use @JsonProperty annotations like this to convert into entity attributes?

@JsonProperty("msg_type")
private String msgType

The following annotations belong to jackson. To use them, you need to create a new ObjectMapper object for conversion, which is not simple enough in appearance.

@JsonProperty("msg_type")
private String msgType;

I found fastjson and there are similar annotations (as follows). The usage method is still the static method of JSON.parseObject(). I prefer this method.

@JSONField(name = "id")
private String id;

I want to use the second method, but is there a requirement in the project to use a certain conversion method? In addition, these two methods will create a new entity object, so it seems strange to put it directly in BO. Because this method is not a static method, but is just a capability of BO, it is unlikely that a new BO object will be created in BO. Instead, the properties should be filled in.

Solution: The first is to copy the generated new object into this object through BeanUtil. The second is to see if there are other tools that can fill in attribute mapping without generating new objects.

Later, I will go to the actual coding to test whether there are any problems with the implementation details here: smile:

@zqr10159
Copy link
Member

zqr10159 commented Nov 7, 2023

项目是统一使用jackson的,不需要new Objectmapper,直接注入即可,有统一配置

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


The project uses jackson uniformly, no new Objectmapper is needed, just inject it directly, with unified configuration

@tomsun28
Copy link
Contributor

tomsun28 commented Nov 7, 2023

建议统一使用 jackson, org.dromara.hertzbeat.common.util.JsonUtil fastjson漏洞比较多

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


It is recommended to use jackson uniformly, org.dromara.hertzbeat.common.util.JsonUtil fastjson has many vulnerabilities

@SurryChen
Copy link
Contributor Author

项目是统一使用jackson的,不需要new Objectmapper,直接注入即可,有统一配置

设计思路是想把转换能力收敛到BO中,所以不好在BO中进行注入:joy:

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


The project uses jackson uniformly, no new Objectmapper is needed, just inject it directly, with unified configuration

The design idea is to converge the conversion capabilities into BO, so it is not easy to inject into BO:joy:

@SurryChen
Copy link
Contributor Author

建议统一使用 jackson, org.dromara.hertzbeat.common.util.JsonUtil fastjson漏洞比较多

原来有写了对应的工具类呀,那就使用这个啦

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


It is recommended to use jackson uniformly, org.dromara.hertzbeat.common.util.JsonUtil fastjson has many vulnerabilities

It turns out that a corresponding tool class has been written, so use this one.

@SurryChen
Copy link
Contributor Author

优化1.0

枚举:

原项目没有枚举的包,我这里新建了一个:

image

CloudServiceAlarmInformationEnum

image

枚举这里将告警信息实体的Class与对应的云服务名称(URL中占位符)进行绑定,枚举获取是遍历方式的,虽然也可以不写枚举,直接用Map去做类似的操作,但是由于枚举类型不多,也并不耗时,所以使用枚举去获取,更便于代码管理一点

DTO改造:

改造思路:由于JSON字符串最终是修改为AlertReport这个实体,实际上需要先将JSON字符串转为对应的云服务告警实体,再由云服务告警实体转为AlertReport这个实体,由于第一个转换只需要使用JSONUtil一行代码即可,所以放弃了原来设计文档中把这个转换能力收敛到云服务告警实体中的想法,因为那样需要多创建一个对象,不够合理,现在改为在控制层进行转换。

还需要处理的地方在于云服务告警实体如何转换为AlertReport实体,由于AlertReport实体参数是基本固定的,而AlertReport的属性可以由云服务告警实体中的属性得出,所以我们可以将AlertReport的属性看为云服务告警实体的虚拟属性,把获取虚拟属性的能力收敛到云服务告警实体中,这样,我们就可以使用模板方法来进行改造。

CloudAlertReportAbstract(抽象父类)
image

代码解释:

  1. 所有的云服务告警实体都需要继承该抽象类并实现对应的抽象方法
  2. 该抽象类的抽象方法与AlertReport的属性有强关联关系,已使用@link标注
  3. 使用@JsonIgnore是因为jackson是通过getXxx()方法获取对象的序列化内容的,导致虚拟属性也被获取到,所以做一下过滤
  4. 抛异常是因为里面有一个日期格式的转换,需要抛一个格式异常,然后我干脆全部抛一下,看起来整齐一点

实现类举例
image

需要实现父类的抽象方法,关于里面的一些可以商讨的点:

  1. 有一些字符串或者数字,我是直接写上去了,并没有定义常量,如果是在类里面直接定义这些常量,不太符合云服务告警实体的语义,然后也没有在这个模块找到专门写静态变量的类,所以就暂时这样写一下。

控制层的改造:

只是使用枚举进行了一下校验,后面进行模板填充了一下,顺便优化了一下日志打印和原先的一个解析失败似乎还会进行一个空告警的小问题

代码:

image

还有一些商讨的地方:

  1. 我想给API使用错误的用户一个告警,也走了这个第三方告警的通道,但是这个信息,我暂时只是简单定义了一下,应该需要更改一下

测试:

根据之前的测试类小小改了一下,然后debug了一下,构造出来的信息模板与之前一致

测试代码:

image

改造后如何接入一款新的云服务告警

  1. 创建对应云服务的告警实体,并继承CloudAlertReportAbstract这个抽象类
  2. CloudServiceAlarmInformationEnum里面增加一个枚举类型

然后就可以了

各位看看哪里还需要修改一下呢:grin:

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Optimization 1.0

Enumeration:

The original project does not have enumerated packages, so I created a new one here:

image

CloudServiceAlarmInformationEnum:

image

Enumeration here binds the Class of the alarm information entity with the corresponding cloud service name (placeholder in the URL). The enumeration is obtained through traversal. Although you can also directly use Map to perform similar operations without writing the enumeration. , but since there are not many enumeration types and it is not time-consuming, using enumeration to obtain it is more convenient for code management.

DTO transformation:

Transformation idea: Since the JSON string is eventually modified into the entity AlertReport, it is actually necessary to convert the JSON string into the corresponding cloud service alarm entity first, and then convert the cloud service alarm entity into the entity AlertReport. A conversion only requires one line of code using JSONUtil, so I gave up the idea of ​​converging this conversion capability into the cloud service alarm entity in the original design document, because that would require the creation of one more object, which was not reasonable. Now it is changed to the control layer. Make the conversion.

What still needs to be processed is how to convert the cloud service alarm entity into an AlertReport entity. Since the AlertReport entity parameters are basically fixed, and the properties of AlertReport can be derived from the attributes in the cloud service alarm entity, so we can Consider the attributes of AlertReport as virtual attributes of the cloud service alarm entity, and integrate the ability to obtain virtual attributes into the cloud service alarm entity. In this way, we can use the template method to carry out transformation.

CloudAlertReportAbstract (abstract parent class):
image

Code explanation:

  1. All cloud service alarm entities need to inherit this abstract class and implement the corresponding abstract method
  2. The abstract method of this abstract class has a strong relationship with the properties of AlertReport and has been annotated with @link
  3. Use @JsonIgnore because jackson obtains the serialized content of the object through the getXxx() method, resulting in virtual attributes also being obtained, so do some filtering
  4. The exception is thrown because there is a date format conversion, and a format exception needs to be thrown. Then I just throw them all to make it look neater.

Implementation class example:
image

It is necessary to implement the abstract method of the parent class. There are some points that can be discussed:

  1. There are some strings or numbers, but I wrote them directly without defining constants. If these constants are directly defined in the class, it does not conform to the semantics of the cloud service alarm entity, and I have not found any specifically written strings or numbers in this module. The class of static variables, so let’s write it like this for the time being.

Transformation of control layer:

I just used enumeration for verification, and then filled in the template. By the way, I optimized the log printing and the original parsing failure seemed to cause an empty alarm.

Code:

image

There are some areas for discussion:

  1. I want to give an alert to users who use the API incorrectly, and I also use the third-party alert channel. However, I have only briefly defined this information for the time being, and it should be changed.

test:

I made a slight modification based on the previous test class, and then debugged it. The constructed information template is consistent with the previous one.

Test code:

image

How to access a new cloud service alarm after transformation

  1. Create an alarm entity corresponding to the cloud service and inherit the abstract class CloudAlertReportAbstract
  2. Add an enumeration type in CloudServiceAlarmInformationEnum

Then it's ok

Please take a look at where you still need to modify it: grin:

@tomsun28
Copy link
Contributor

tomsun28 commented Nov 8, 2023

👍👍👍 可以直接提交PR就行 我们在PR那边review

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


👍👍👍 You can just submit the PR directly and we will review it on the PR side

@github-project-automation github-project-automation bot moved this from To do to Done in hertzbeat-v1.0 Nov 8, 2023
@SurryChen SurryChen linked a pull request Nov 9, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment