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

[Dubbo-1510] dubbo monitor missing parameters #1407

Merged
merged 1 commit into from
Apr 10, 2018
Merged

[Dubbo-1510] dubbo monitor missing parameters #1407

merged 1 commit into from
Apr 10, 2018

Conversation

YoungHu
Copy link
Contributor

@YoungHu YoungHu commented Feb 28, 2018

fix #1510

What is the purpose of the change

  1. dubbo monitor collection missing the group version dubbo parameters
  2. dubbo monitor interval default set to 60s and can't override by outside setting

Brief changelog

  1. dubbo monitor parameters add group, version,dubbo parameters
  2. monitor interval can be override by outside setting

Verifying this change

XXXX

2. monitor interval can be override by outside setting
@codecov-io
Copy link

Codecov Report

Merging #1407 into master will decrease coverage by 0.11%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1407      +/-   ##
=========================================
- Coverage   33.01%   32.9%   -0.12%     
=========================================
  Files         691     691              
  Lines       34624   34630       +6     
  Branches     6850    6850              
=========================================
- Hits        11431   11394      -37     
- Misses      21247   21288      +41     
- Partials     1946    1948       +2
Impacted Files Coverage Δ
...n/java/com/alibaba/dubbo/config/MonitorConfig.java 0% <0%> (ø) ⬆️
.../com/alibaba/dubbo/monitor/dubbo/DubboMonitor.java 73.39% <100%> (+0.24%) ⬆️
...m/alibaba/dubbo/monitor/support/MonitorFilter.java 47.27% <100%> (+1.98%) ⬆️
...ubbo/rpc/protocol/dubbo/ChannelWrappedInvoker.java 41.66% <0%> (-8.34%) ⬇️
...mon/serialize/support/dubbo/GenericDataOutput.java 64.91% <0%> (-7.02%) ⬇️
...om/alibaba/dubbo/rpc/filter/ActiveLimitFilter.java 77.77% <0%> (-5.56%) ⬇️
...mmon/serialize/support/dubbo/GenericDataInput.java 57.91% <0%> (-3.87%) ⬇️
...ba/dubbo/remoting/transport/netty/NettyServer.java 67.85% <0%> (-3.58%) ⬇️
.../dubbo/rpc/protocol/dubbo/filter/FutureFilter.java 54.54% <0%> (-2.03%) ⬇️

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 3be23d9...eb6aac9. Read the comment docs.

@YoungHu YoungHu changed the title [fix] dubbo monitor missing parameters [Dubbo-1510] dubbo monitor missing parameters Mar 26, 2018
this.interval = interval;
}

public String getInterval(){

Choose a reason for hiding this comment

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

Who calls getInterval()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no one call getInterval() direct, this is used for fastjson serialize, so it can be passed to URL

@@ -92,6 +93,7 @@ public void send() {
long maxOutput = numbers[7];
long maxElapsed = numbers[8];
long maxConcurrent = numbers[9];
String version = getUrl().getParameter(Constants.DEFAULT_PROTOCOL);

Choose a reason for hiding this comment

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

IMO, this variable is named protocol may be better.

Copy link
Member

Choose a reason for hiding this comment

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

why DEFAULT_PROTOCOL is necessary to collect?

Copy link
Contributor Author

@YoungHu YoungHu Apr 8, 2018

Choose a reason for hiding this comment

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

DEFAULT_PROTOCOL value is dubbo, this code is to get dubbo version but not protocol name. dubbo now upgrade fast, for a team, different project may use different dubbo version(eg 2.5.3, 2.6.0, 2.6.1), IMO, monitor need to know each project's dubbo version, so we can decide how to process the monitor data.

Copy link
Member

Choose a reason for hiding this comment

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

sounds a reasonable data to collect, let me think about it more.

Copy link
Member

@beiwei30 beiwei30 left a comment

Choose a reason for hiding this comment

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

why DEFAULT_PROTOCOL is necessary to collect?

@@ -92,6 +93,7 @@ public void send() {
long maxOutput = numbers[7];
long maxElapsed = numbers[8];
long maxConcurrent = numbers[9];
String version = getUrl().getParameter(Constants.DEFAULT_PROTOCOL);
Copy link
Member

Choose a reason for hiding this comment

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

why DEFAULT_PROTOCOL is necessary to collect?

@beiwei30 beiwei30 merged commit 2e84f07 into apache:master Apr 10, 2018
@chickenlj chickenlj added this to the 2.6.2 milestone Apr 24, 2018
rolandhe pushed a commit to rolandhe/dubbo that referenced this pull request Sep 9, 2019
2. monitor interval can be override by outside setting
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.

dubbo monitor missing parameters
5 participants