You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@dubbo.apache.org by "panxiaojun233 (GitHub)" <gi...@apache.org> on 2019/04/09 05:43:40 UTC

[GitHub] [incubator-dubbo] panxiaojun233 opened pull request #3833: metrics service

exposing Metrics data by metrics service

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on issue #3833: metrics service

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

@nzomkxia Would you please help to review the Metrics collecting part?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] ralf0131 commented on issue #3833: metrics service

Posted by "ralf0131 (GitHub)" <gi...@apache.org>.

Hi, could you please resolve the conflict first?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3833: metrics service

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

`NetUtils.getLocalAddress().getHostName()`

The IP address used here may have a problem, we should think using a unified tool for getting outward IP, I remember there's one thread discussing the same topic. 

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] nzomkxia commented on pull request #3833: metrics service

Posted by "nzomkxia (GitHub)" <gi...@apache.org>.

this will create a new object every time

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] nzomkxia commented on issue #3833: metrics service

Posted by "nzomkxia (GitHub)" <gi...@apache.org>.

the metrics stuff can be stored in metadata center

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3833: metrics service

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=h1) Report
> Merging [#3833](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/72020dfb380ea4081d26f5c7527fb4d4320d4908?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `77.04%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3833      +/-   ##
============================================
+ Coverage     63.57%   63.64%   +0.07%     
  Complexity       71       71              
============================================
  Files           705      709       +4     
  Lines         31128    31273     +145     
  Branches       5048     5057       +9     
============================================
+ Hits          19789    19904     +115     
- Misses         9063     9081      +18     
- Partials       2276     2288      +12
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/main/java/org/apache/dubbo/common/Constants.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vQ29uc3RhbnRzLmphdmE=) | `93.33% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [.../java/org/apache/dubbo/config/ReferenceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9SZWZlcmVuY2VDb25maWcuamF2YQ==) | `61.23% <100%> (+0.14%)` | `0 <0> (ø)` | :arrow_down: |
| [...in/java/org/apache/dubbo/config/ServiceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9TZXJ2aWNlQ29uZmlnLmphdmE=) | `56.58% <100%> (+0.09%)` | `0 <0> (ø)` | :arrow_down: |
| [...bo/config/spring/schema/DubboNamespaceHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvc2NoZW1hL0R1YmJvTmFtZXNwYWNlSGFuZGxlci5qYXZh) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...in/java/org/apache/dubbo/config/MetricsConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9NZXRyaWNzQ29uZmlnLmphdmE=) | `18.18% <18.18%> (ø)` | `0 <0> (?)` | |
| [.../org/apache/dubbo/monitor/dubbo/MetricsFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tbW9uaXRvci9kdWJiby1tb25pdG9yLWRlZmF1bHQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL21vbml0b3IvZHViYm8vTWV0cmljc0ZpbHRlci5qYXZh) | `82.4% <82.4%> (ø)` | `0 <0> (?)` | |
| [.../apache/dubbo/qos/protocol/QosProtocolWrapper.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tcGx1Z2luL2R1YmJvLXFvcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcW9zL3Byb3RvY29sL1Fvc1Byb3RvY29sV3JhcHBlci5qYXZh) | `69.23% <0%> (-12.83%)` | `0% <0%> (ø)` | |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `63.04% <0%> (-4.35%)` | `0% <0%> (ø)` | |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `80.14% <0%> (-0.74%)` | `0% <0%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=footer). Last update [72020df...5ac3264](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3833: metrics service

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Why not use `appendParameters(map, metricsConfig);` directly as the other Configs do?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3833: metrics service

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=h1) Report
> Merging [#3833](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/72020dfb380ea4081d26f5c7527fb4d4320d4908?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `62.41%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##             master   #3833      +/-   ##
===========================================
+ Coverage     63.57%   63.6%   +0.03%     
  Complexity       71      71              
===========================================
  Files           705     709       +4     
  Lines         31128   31304     +176     
  Branches       5048    5070      +22     
===========================================
+ Hits          19789   19912     +123     
- Misses         9063    9102      +39     
- Partials       2276    2290      +14
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/main/java/org/apache/dubbo/common/Constants.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vQ29uc3RhbnRzLmphdmE=) | `93.33% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...va/org/apache/dubbo/config/spring/ServiceBean.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvU2VydmljZUJlYW4uamF2YQ==) | `48.21% <0%> (-3.38%)` | `0 <0> (ø)` | |
| [...in/java/org/apache/dubbo/config/MetricsConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9NZXRyaWNzQ29uZmlnLmphdmE=) | `0% <0%> (ø)` | `0 <0> (?)` | |
| [.../org/apache/dubbo/config/spring/ReferenceBean.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvUmVmZXJlbmNlQmVhbi5qYXZh) | `16.81% <0%> (-1.82%)` | `0 <0> (ø)` | |
| [...bo/config/spring/schema/DubboNamespaceHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvc2NoZW1hL0R1YmJvTmFtZXNwYWNlSGFuZGxlci5qYXZh) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [.../java/org/apache/dubbo/config/ReferenceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9SZWZlcmVuY2VDb25maWcuamF2YQ==) | `60.93% <50%> (-0.16%)` | `0 <0> (ø)` | |
| [...in/java/org/apache/dubbo/config/ServiceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9TZXJ2aWNlQ29uZmlnLmphdmE=) | `56.43% <50%> (-0.06%)` | `0 <0> (ø)` | |
| [.../org/apache/dubbo/monitor/dubbo/MetricsFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tbW9uaXRvci9kdWJiby1tb25pdG9yLWRlZmF1bHQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL21vbml0b3IvZHViYm8vTWV0cmljc0ZpbHRlci5qYXZh) | `80% <80%> (ø)` | `0 <0> (?)` | |
| [...pache/dubbo/registry/support/AbstractRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L0Fic3RyYWN0UmVnaXN0cnkuamF2YQ==) | `79.13% <0%> (-1.19%)` | `0% <0%> (ø)` | |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `80.14% <0%> (-0.74%)` | `0% <0%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3833/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=footer). Last update [72020df...644b07b](https://codecov.io/gh/apache/incubator-dubbo/pull/3833?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3833: metrics service

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

`metricsInvoker` is only used in here,  it can be a local variable instead of defined as MetricsFilter's property.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] ralf0131 commented on issue #3833: metrics service

Posted by "ralf0131 (GitHub)" <gi...@apache.org>.

Hi, could you please resolve the UT failure first?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] nzomkxia commented on issue #3833: metrics service

Posted by "nzomkxia (GitHub)" <gi...@apache.org>.

> @nzomkxia Would you please help to review the Metrics collecting part?

metrics collecting part looks good. but  how to pass the metrics protocol and port to dubbo admin? they are not in the registry center under [simplified URL](http://dubbo.apache.org/zh-cn/docs/user/demos/simplify-registry-data.html) mode

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3833: metrics service

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

I find Protocol has a `getDefaultPort()` method to get each Protocol's default port. So maybe there's no need to define a default port here? How about following each protocol's default port. 

One thing to notice is that you may need to create a separate connection for metrics service, this can be helpful in isolating from normal RPC calls especially when they are working on the same port.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3833 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org