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

MetricsFilter implements BaseFilter.Listener #10589

Merged
merged 21 commits into from
Sep 11, 2022
Merged

Conversation

jojocodeX
Copy link
Contributor

@jojocodeX jojocodeX commented Sep 9, 2022

@@ -48,38 +49,21 @@ public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcExcept
if (collector == null || !collector.isCollectEnabled()) {
return invoker.invoke(invocation);
}
MetricsCollectExecutor collectorExecutor = new MetricsCollectExecutor(collector, invocation);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line of code appears in multiple places, can you extract it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已处理

private String version;


private static final String METRIC_FILTER_START_TIME = "metric_filter_start_time";
Copy link
Contributor

Choose a reason for hiding this comment

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

This line of code can be placed in src/main/java/org/apache/dubbo/common/constants/MetricsConstants.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已处理

Result result = filter.invoke(invoker, invocation);

filter.onResponse(result, invoker, invocation);

Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to use format checking to remove extra blank lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已处理

@@ -0,0 +1,82 @@
package org.apache.dubbo.metrics.filter;
Copy link
Contributor

Choose a reason for hiding this comment

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

apache's open source protocol should be copy-pasted in the file header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已处理

Comment on lines 60 to 62
if (throwable instanceof RpcException) {
collector.increaseFailedRequests(interfaceName, methodName, group, version);
}
Copy link
Member

Choose a reason for hiding this comment

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

需要处理非 RpcException 的情况,通过 onError 过来的基本都是框架异常
onResponse 是业务正常返回(包括业务正常返回、业务抛异常返回),这里可能需要独立判断采集下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是否可以这样理解为,在非rpc异常的情况下,不需要统计rt时间

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已处理

@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2022

Codecov Report

Merging #10589 (f8b7a69) into 3.1 (eeac1c3) will increase coverage by 0.08%.
The diff coverage is 96.29%.

@@             Coverage Diff              @@
##                3.1   #10589      +/-   ##
============================================
+ Coverage     64.76%   64.84%   +0.08%     
- Complexity      397      414      +17     
============================================
  Files          1331     1334       +3     
  Lines         56564    56667     +103     
  Branches       8377     8378       +1     
============================================
+ Hits          36632    36747     +115     
+ Misses        15987    15984       -3     
+ Partials       3945     3936       -9     
Impacted Files Coverage Δ
...e/dubbo/metrics/filter/MetricsCollectExecutor.java 89.36% <89.36%> (ø)
...mon/metrics/collector/DefaultMetricsCollector.java 98.66% <97.95%> (+5.48%) ⬆️
...n/metrics/collector/stat/MetricsStatComposite.java 98.11% <98.11%> (ø)
...rics/collector/stat/DefaultMetricsStatHandler.java 100.00% <100.00%> (ø)
...pache/dubbo/common/metrics/event/RequestEvent.java 83.33% <100.00%> (+1.51%) ⬆️
.../apache/dubbo/common/metrics/model/MetricsKey.java 100.00% <100.00%> (ø)
...o/metrics/collector/AggregateMetricsCollector.java 90.76% <100.00%> (+0.60%) ⬆️
...org/apache/dubbo/metrics/filter/MetricsFilter.java 93.75% <100.00%> (-3.03%) ⬇️
...dubbo/remoting/exchange/support/DefaultFuture.java 87.93% <0.00%> (-3.45%) ⬇️
.../dubbo/rpc/protocol/dubbo/DecodeableRpcResult.java 58.97% <0.00%> (-2.57%) ⬇️
... and 18 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

4 participants