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

Use summary type to observe p99 #1875

Merged
merged 6 commits into from
May 8, 2022
Merged

Conversation

jyz0309
Copy link
Contributor

@jyz0309 jyz0309 commented May 1, 2022

Signed-off-by: jyz0309 45495947@qq.com

What this PR does:
Change metric type to observe RT P99 value.
Which issue(s) this PR fixes:
None.

You should pay attention to items below to ensure your pr passes our ci test
We do not merge pr with ci tests failed

  • All ut passed (run 'go test ./...' in project root)
  • After go-fmt ed , run 'go fmt project' using goland.
  • Golangci-lint passed, run 'sudo golangci-lint run' in project root.
  • After import formatted, (using imports-formatter to run 'imports-formatter .' in project root, to format your import blocks, mentioned in CONTRIBUTING.md above)
  • Your new-created file needs to have apache license at the top, like other existed file does.
  • All integration test passed. You can run integration test locally (with docker env). Clone our dubbo-go-samples project and replace the go.mod to your dubbo-go, and run 'sudo sh start_integration_test.sh' at root of samples project root. (M1 Slice is not Support)

Signed-off-by: jyz0309 <45495947@qq.com>
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2022

Codecov Report

Merging #1875 (88ee3c0) into 3.0 (759a9b5) will decrease coverage by 2.05%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##              3.0    #1875      +/-   ##
==========================================
- Coverage   46.87%   44.82%   -2.06%     
==========================================
  Files         298      283      -15     
  Lines       17178    16933     -245     
==========================================
- Hits         8053     7591     -462     
- Misses       8272     8533     +261     
+ Partials      853      809      -44     
Impacted Files Coverage Δ
metrics/prometheus/reporter.go 33.33% <91.66%> (+4.76%) ⬆️
config/metric_config.go 68.00% <100.00%> (+1.33%) ⬆️
registry/nacos/listener.go 0.00% <0.00%> (-80.87%) ⬇️
remoting/nacos/builder.go 0.00% <0.00%> (-73.92%) ⬇️
registry/etcdv3/registry.go 3.07% <0.00%> (-70.77%) ⬇️
metadata/report/etcd/report.go 1.11% <0.00%> (-61.12%) ⬇️
registry/etcdv3/listener.go 0.00% <0.00%> (-50.00%) ⬇️
config_center/nacos/listener.go 0.00% <0.00%> (-50.00%) ⬇️
remoting/etcdv3/listener.go 3.53% <0.00%> (-43.37%) ⬇️
config_center/nacos/impl.go 40.59% <0.00%> (-34.66%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 759a9b5...88ee3c0. Read the comment docs.

Signed-off-by: jyz0309 <45495947@qq.com>
Signed-off-by: jyz0309 <45495947@qq.com>
@LaurenceLiZhixin
Copy link
Contributor

LaurenceLiZhixin commented May 5, 2022

Good, and it's better to add Max Age configuration at
https://github.com/apache/dubbo-go/pull/1875/files#diff-dabf5562f4d32b8bc69d22e50f651690c48257c90f56f7104cde9fc014af1cd3R217

The sdk:
https://github.com/prometheus/client_golang/blob/fa4aa9000d2863904891d193dea354d23f3d712a/prometheus/summary.go#L207

and export the configuration to dubbogo.yaml , metrics config struct of root config.

@jyz0309

@jyz0309
Copy link
Contributor Author

jyz0309 commented May 5, 2022

Good, and it's better to add Max Age configuration at https://github.com/apache/dubbo-go/pull/1875/files#diff-dabf5562f4d32b8bc69d22e50f651690c48257c90f56f7104cde9fc014af1cd3R217

The sdk: https://github.com/prometheus/client_golang/blob/fa4aa9000d2863904891d193dea354d23f3d712a/prometheus/summary.go#L207

and export the configuration to dubbogo.yaml , metrics config struct of root config.

@jyz0309

Can you tell me how to export configuration to dubbogo.yaml? It will help a lot if there is a sample code. @LaurenceLiZhixin

@LaurenceLiZhixin
Copy link
Contributor

Add a config field named "RTSummaryMaxAge" of metrics config here: https://github.com/apache/dubbo-go/blob/master/config/metric_config.go#L31
And set this config field to your summary.
@jyz0309

Signed-off-by: jyz0309 <45495947@qq.com>
Signed-off-by: jyz0309 <45495947@qq.com>
Signed-off-by: jyz0309 <45495947@qq.com>
@AlexStocks AlexStocks merged commit b27ec53 into apache:3.0 May 8, 2022
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.

6 participants