Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
AlexStocks merged PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665 -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
sonarqubecloud[bot] commented on PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#issuecomment-5305318247 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
tangerineyu commented on code in PR #3665:
URL: https://github.com/apache/dubbo-go/pull/3665#discussion_r3790751586
##
metrics/rpc/error_classifier.go:
##
@@ -112,6 +118,12 @@ func classifyError(err error) ErrorType {
if err == nil {
return ErrorTypeUnknown
}
+ // A typed nil *triple_protocol.Error escapes err == nil, but CodeOf
+ // would panic while dereferencing it, so return ErrorTypeUnknown
instead.
+ var tripleErr *triple_protocol.Error
+ if errors.As(err, &tripleErr) && tripleErr == nil {
+ return ErrorTypeUnknown
+ }
Review Comment:
谢谢review, 已按建议把检查移进了 CodeOf, 没有用AsType, 这个是1.26的新增api, 目前go.mod还是1.25
我沿用了asError, 只是加了一个nil判断
--
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]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
sonarqubecloud[bot] commented on PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#issuecomment-5305303847 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
Alanxtl commented on code in PR #3665:
URL: https://github.com/apache/dubbo-go/pull/3665#discussion_r3790581661
##
metrics/rpc/error_classifier.go:
##
@@ -112,6 +118,12 @@ func classifyError(err error) ErrorType {
if err == nil {
return ErrorTypeUnknown
}
+ // A typed nil *triple_protocol.Error escapes err == nil, but CodeOf
+ // would panic while dereferencing it, so return ErrorTypeUnknown
instead.
+ var tripleErr *triple_protocol.Error
+ if errors.As(err, &tripleErr) && tripleErr == nil {
+ return ErrorTypeUnknown
+ }
Review Comment:
move this check inside the ` code := triple_protocol.CodeOf(err)` in line 129
for example
```go
if tripleErr, ok := errors.AsType[*triple_protocol.Error](err); ok &&
tripleErr == nil {
return ErrorTypeUnknown
}
```
--
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]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
tangerineyu commented on code in PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#discussion_r3788717344 ## metrics/options.go: ## @@ -26,14 +26,25 @@ import ( "dubbo.apache.org/dubbo-go/v3/global" ) +// Options holds the configuration of the metrics module. +// It wraps the global MetricsConfig, and can be built programmatically +// with NewOptions and a set of Option functions. +// +// The metrics module is disabled by default; use WithEnabled to turn it on. Review Comment: 已处理, 已在文档中区分构造期零值和初始化默认值 -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
sonarqubecloud[bot] commented on PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#issuecomment-5300882660 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
AlexStocks commented on code in PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#discussion_r3788488245 ## metrics/options.go: ## @@ -26,14 +26,25 @@ import ( "dubbo.apache.org/dubbo-go/v3/global" ) +// Options holds the configuration of the metrics module. +// It wraps the global MetricsConfig, and can be built programmatically +// with NewOptions and a set of Option functions. +// +// The metrics module is disabled by default; use WithEnabled to turn it on. Review Comment: [P2] 请区分 NewOptions() 的构造期零值和实例初始化后的有效默认值 global.DefaultMetricsConfig() 明确不填充带 default tag 的字段，当前新增测试也断言 NewOptions() 得到的 Port、Path、Protocol 都为空，Enable 为 nil；9090、/metrics、prometheus 等值是在更外层 client/server 初始化执行 defaults.Set 后才生效。这里以及 WithPort、WithPath、WithPrometheus 等注释直接写“默认值”，会让直接调用 NewOptions() 的用户误以为返回对象已经具备这些值。请明确这些是运行时实例初始化默认值；如果 API 设计要求 NewOptions() 立即返回它们，则应在该入口实际应用 defaults.Set 并同步测试。 -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
Copilot commented on code in PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#discussion_r3788382335 ## metrics/options.go: ## @@ -26,14 +26,25 @@ import ( "dubbo.apache.org/dubbo-go/v3/global" ) +// Options holds the configuration of the metrics module. +// It wraps the global MetricsConfig, and can be built programmatically +// with NewOptions and a set of Option functions. +// +// The metrics module is disabled by default; use WithEnabled to turn it on. Review Comment: The doc comment implies NewOptions/defaultOptions start with the effective defaults (e.g. port 9090, protocol prometheus), but global.DefaultMetricsConfig intentionally returns a config with zero values and relies on defaults.Set during instance initialization (see global/metric_config.go:68-71 and instance_options_init.go:246-248). Clarifying this here would prevent readers from assuming NewOptions immediately applies struct-tag defaults. -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
tangerineyu commented on code in PR #3665:
URL: https://github.com/apache/dubbo-go/pull/3665#discussion_r3788328409
##
metrics/rpc/error_classifier.go:
##
@@ -112,6 +118,12 @@ func classifyError(err error) ErrorType {
if err == nil {
return ErrorTypeUnknown
}
+ // A typed nil *triple_protocol.Error escapes err == nil, but CodeOf
+ // would panic while dereferencing it, so return ErrorTypeUnknown
instead.
+ var tripleErr *triple_protocol.Error
+ if errors.As(err, &tripleErr) && tripleErr == nil {
+ return ErrorTypeUnknown
+ }
Review Comment:
typed-nil (*triple_protocol.Error)(nil)会被err == nil放行, 然后传给Codeof,
Codeof调用Code解引用会panic
--
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]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
Alanxtl commented on code in PR #3665:
URL: https://github.com/apache/dubbo-go/pull/3665#discussion_r3788231988
##
metrics/rpc/error_classifier.go:
##
@@ -112,6 +118,12 @@ func classifyError(err error) ErrorType {
if err == nil {
return ErrorTypeUnknown
}
+ // A typed nil *triple_protocol.Error escapes err == nil, but CodeOf
+ // would panic while dereferencing it, so return ErrorTypeUnknown
instead.
+ var tripleErr *triple_protocol.Error
+ if errors.As(err, &tripleErr) && tripleErr == nil {
+ return ErrorTypeUnknown
+ }
Review Comment:
为什么要加这几行
--
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]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
sonarqubecloud[bot] commented on PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#issuecomment-521627 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
codecov-commenter commented on PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#issuecomment-5299977614 ## [Codecov](https://app.codecov.io/gh/apache/dubbo-go/pull/3665?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 54.57%. Comparing base ([`60d1c2a`](https://app.codecov.io/gh/apache/dubbo-go/commit/60d1c2a949f0ee0da4be3fe09fb79d295491e040?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8557c59`](https://app.codecov.io/gh/apache/dubbo-go/commit/8557c5977c422e00352efcd5f06ec8067bab17ce?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 923 commits behind head on develop. Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop#3665 +/- ## === + Coverage46.76% 54.57% +7.81% === Files 295 474 +179 Lines1717236734 +19562 === + Hits 803120049 +12018 - Misses828715069+6782 - Partials 854 1616 +762 ``` [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/dubbo-go/pull/3665?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] docs(metrics): improve comments and test coverage of metrics options and rpc error classifier [dubbo-go]
sonarqubecloud[bot] commented on PR #3665: URL: https://github.com/apache/dubbo-go/pull/3665#issuecomment-5299939510 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3665&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3665&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3665&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3665) -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
