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

Adds metrics initialization metrics for the service interface(#12850) #12892

Merged
merged 23 commits into from
Aug 26, 2023

Conversation

robin977
Copy link
Contributor

Adds metrics initialization metrics for the service interface(#12850)

Copy link
Member

@AlbumenJ AlbumenJ left a comment

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 Aug 14, 2023

Codecov Report

Merging #12892 (b62177b) into 3.2 (2a5b437) will increase coverage by 0.73%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##                3.2   #12892       +/-   ##
=============================================
+ Coverage     68.76%   69.50%    +0.73%     
+ Complexity      318        2      -316     
=============================================
  Files          3657     1649     -2008     
  Lines        171093    68486   -102607     
  Branches      28245     9996    -18249     
=============================================
- Hits         117660    47599    -70061     
+ Misses        43258    16294    -26964     
+ Partials      10175     4593     -5582     

see 2056 files with indirect coverage changes

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

@songxiaosheng
Copy link
Member

init metric as follow
image

Copy link
Member

@songxiaosheng songxiaosheng left a comment

Choose a reason for hiding this comment

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

LGTM

initWindowCounter(event,MetricsKey.METRIC_REQUESTS_NETWORK_FAILED_AGG);
initWindowCounter(event,MetricsKey.METRIC_REQUESTS_CODEC_FAILED_AGG);
initWindowCounter(event,MetricsKey.METRIC_REQUESTS_TOTAL_SERVICE_UNAVAILABLE_FAILED_AGG);
}
Copy link
Member

Choose a reason for hiding this comment

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

These constants can be placed in DefaultConstants in the form of List, and then foreach does init, which can reduce duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

}
methodNumStats.get(wrapper).computeIfAbsent(new MethodMetric(getApplicationModel(), invocation), k -> new AtomicLong(0L)).set(size);
}

Copy link
Member

Choose a reason for hiding this comment

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

The init method can directly set 0, no need to transfer parameters

onInitRtRequest(metric);
onInitRtAgrRequest(metric);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

RequestInitEvent and RequestEvent look different, one is triggered by app init, the other is triggered by rpc request, is it better to separate the listener?

@AlbumenJ AlbumenJ requested a review from wxbty August 22, 2023 07:15
@robin977
Copy link
Contributor Author

企业微信截图_705265f9-eee7-427e-8456-4cf3cffc1b99
企业微信截图_db714257-9123-4f99-bca5-ff472448b4a4
企业微信截图_340eefef-f167-4d51-b07e-846e75a78469

public static MetricsInitEvent toMetricsInitEvent(ApplicationModel applicationModel, Invocation invocation) {
MethodMetric methodMetric = new MethodMetric(applicationModel, invocation);
MetricsInitEvent initEvent = new MetricsInitEvent(applicationModel, new TypeWrapper(MetricsLevel.SERVICE, METRIC_REQUESTS));
initEvent.putAttachment(MetricsConstants.INVOCATION, invocation);
Copy link
Member

Choose a reason for hiding this comment

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

Do not create immutable objects every time.

@@ -69,7 +70,7 @@ private CategoryOverall initMethodRequest() {
MetricsSupport.increment(key, dynamicPlaceType, (MethodMetricsCollector) collector, event);
MetricsSupport.increment(MetricsKey.METRIC_REQUESTS_PROCESSING, dynamicPlaceType, (MethodMetricsCollector) collector, event);
})),
new MetricsCat(MetricsKey.METRIC_REQUESTS_SUCCEED, (key, placeType, collector) -> AbstractMetricsKeyListener.onFinish(key,
new MetricsCat(METRIC_REQUESTS_SUCCEED, (key, placeType, collector) -> AbstractMetricsKeyListener.onFinish(key,
Copy link
Member

Choose a reason for hiding this comment

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

Don't change that. Keep it the same as before

@sonarcloud
Copy link

sonarcloud bot commented Aug 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

61.8% 61.8% Coverage
0.0% 0.0% Duplication

@wxbty
Copy link
Member

wxbty commented Aug 26, 2023

LGTM

@wxbty wxbty merged commit 711de7c into apache:3.2 Aug 26, 2023
14 checks passed
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.

5 participants