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

optimize BlockException log #2853

Merged
merged 2 commits into from Aug 31, 2022
Merged

optimize BlockException log #2853

merged 2 commits into from Aug 31, 2022

Conversation

icodening
Copy link
Contributor

Describe what this PR does / why we need it

优化BlockException日志,方便了解触发阻断异常的规则

Does this pull request fix one issue?

Fixes #2851

Describe how you did it

AbstractRule添加id属性即可

Describe how to verify it

配置并触发规则后即可看到id

Special notes for reviews

image

@sczyh30 sczyh30 added the kind/enhancement Category issues or prs related to enhancement. label Aug 30, 2022
@brucelwl
Copy link

@icodening 把ruleId放在, 具体exception后面是不是更加直观点, 如下格式
image

public static void log(String resource, String exceptionName, String ruleLimitApp, String origin, int count) {
statLogger.stat(resource, exceptionName, ruleLimitApp, origin).count(count);
}

public static void log(Long ruleId, String resource, String exceptionName, String ruleLimitApp, String origin, int count) {
statLogger.stat(String.valueOf(ruleId), resource, exceptionName, ruleLimitApp, origin).count(count);
Copy link
Member

Choose a reason for hiding this comment

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

  1. ruleId 为空的时候,这里建议做空处理,不要把 null 打出来
  2. 建议保持格式的兼容性,新的字段往后 append 而不是在最前面加

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK 有道理 可以兼顾对于旧版本的日志采集 晚点我改一下

Copy link
Member

Choose a reason for hiding this comment

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

这里面其实还有一个点,就是 ID 用 Long 类型可能不利于未来扩展,社区可能需要考虑下未来是否搞成 String 类型的。当然这里面可能会有连带影响,比如集群流控的 requestToken 的格式

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里面其实还有一个点,就是 ID 用 Long 类型可能不利于未来扩展,社区可能需要考虑下未来是否搞成 String 类型的。当然这里面可能会有连带影响,比如集群流控的 requestToken 的格式

是的,这个问题我们也遇到了,Long ID不好扩展。我们需要在client端拼接一个ID,并不好拼成一个集群中都一致的Long ID

Copy link
Member

Choose a reason for hiding this comment

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

可以提个 issue 记录下哈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的 回头我记录下

Choose a reason for hiding this comment

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

这里面其实还有一个点,就是 ID 用 Long 类型可能不利于未来扩展,社区可能需要考虑下未来是否搞成 String 类型的。当然这里面可能会有连带影响,比如集群流控的 requestToken 的格式

是的,这个问题我们也遇到了,Long ID不好扩展。我们需要在client端拼接一个ID,并不好拼成一个集群中都一致的Long ID

这里的改动其实改变了统计维度,原来是按照Exception统计的, 现在变成了按照规则维度统计打印block日志

@@ -40,7 +40,12 @@ public class EagleEyeLogUtil {
.buildSingleton();
}

//TEST
Copy link
Member

Choose a reason for hiding this comment

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

Unused comment?

@icodening icodening requested a review from sczyh30 August 31, 2022 02:21
Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit 6c74a4c into alibaba:master Aug 31, 2022
@sczyh30
Copy link
Member

sczyh30 commented Aug 31, 2022

Nice work. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: record rule ID in Sentinel | 希望能够在客户端保存规则的Id
3 participants