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

[#513] Virtual thread pin event metric and alarm #517

Open
wants to merge 21 commits into
base: springboot3-dev
Choose a base branch
from

Conversation

EachannChan
Copy link
Contributor

@EachannChan EachannChan commented Dec 25, 2024

[#513] Virtual thread pin event metric and alarm

@yanhom1314
Copy link
Collaborator

代码改的太乱,改动太大了。我理解这是一个新功能,期望的应该是以最小代价、最小改动集成进去,对扩展开放,对修改关闭,而不是大量改动现有逻辑,分散在其中。

public static final String LARK_ALARM_JSON_COMMON_STR =
"{\"tag\":\"div\",\"fields\":[{\"is_short\":true,\"text\":{\"tag\":\"lark_md\",\"content\":\"%s\"}}]},";

public static final String LARK_ALARM_JSON_STR_SUFFIX =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这是一个完整的json,不要拆分开,很难维护,格式容易配置错

/**
* Pin timeout alarm.
*/
PIN_TIMEOUT("pin_timeout", "");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个不要归类到该枚举里,枚举里是线程池层面常规项,这个加入不太合适

/**
* 拓展字段
*/
private final Map<String, Double> extMap = new ConcurrentHashMap<>(2);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

扩展字段类型可以是任意,不要用double,用Object,初始长度不用指定

@@ -102,11 +102,16 @@ public static List<NotifyItem> getAllNotifyItems() {
queueTimeoutNotify.setType(NotifyItemEnum.QUEUE_TIMEOUT.getValue());
queueTimeoutNotify.setThreshold(10);

List<NotifyItem> notifyItems = new ArrayList<>(6);
NotifyItem pinTimeoutNotify = new NotifyItem();
pinTimeoutNotify.setType(NotifyItemEnum.PIN_TIMEOUT.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个不要归类到现有类型里

@@ -220,6 +221,10 @@ private static void refresh(ExecutorWrapper executorWrapper, DtpExecutorProps pr

private static void doRefresh(ExecutorWrapper executorWrapper, DtpExecutorProps props) {
ExecutorAdapter<?> executor = executorWrapper.getExecutor();
if (executorWrapper.isVirtualThreadExecutor()) {
doRefreshCommon(executorWrapper, props);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个判断可以去掉?走现有逻辑

NotifyItem notifyItem = DtpNotifyCtxHolder.get().getNotifyItem();
BaseNotifyCtx notifyCtx = DtpNotifyCtxHolder.get();
if (notifyCtx.isToLog()) {
log.warn("DynamicTp alarm, executor [" + notifyCtx.getExecutorWrapper().getThreadPoolName() + "]: \n" + Arrays.toString(notifyCtx.getContent()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里是业务无关逻辑,不要硬加判断搞的很乱

if (notifyCtx.isCommonNotify()) {
notifier.sendCommonAlarmMsg(p, notifyItemEnum, notifyCtx.getContent());
} else {
if (!notifyCtx.getExecutorWrapper().isVirtualThreadExecutor()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上

@@ -35,11 +35,24 @@ public class BaseNotifyCtx {

private NotifyItem notifyItem;

private boolean isCommonNotify;

private boolean isToLog;
Copy link
Collaborator

@yanhom1314 yanhom1314 Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不懂加这些字段意义是啥

@@ -88,12 +89,22 @@ public void sendAlarmMsg(NotifyPlatform notifyPlatform, NotifyItemEnum notifyIte
notifier.send(newTargetPlatform(notifyPlatform), content);
}

@Override
public void sendCommonAlarmMsg(NotifyPlatform notifyPlatform, NotifyItemEnum notifyItemEnum, String... content) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

通用告警接口期望的是调用方传进来最终content,直接发送,不用再做组装

@EachannChan
Copy link
Contributor Author

编译时需要将core,spring模块的JDK版本更改为21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants