HDDS-9522. Add percentile for ProtocolMessageMetrics #5479

YuanbenWang · 2023-10-23T07:02:42Z

What changes were proposed in this pull request?

This PR aims to add percentile for ProtocolMessageMetrics.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9522

How was this patch tested?

Tested locally.
The result is shown on the Jira page.

adoroszlai · 2023-10-23T07:45:03Z

Thanks @YuanbenWang for the patch.

There are some checkstyle failures:

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
 185: Line is longer than 80 characters (found 83).
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java
 49: Variable 'quantileEnable' must be private and have accessor methods.
 79: Line is longer than 80 characters (found 85).
 82: Line is longer than 80 characters (found 82).
 83: Line is longer than 80 characters (found 86).

https://github.com/YuanbenWang/ozone/actions/runs/6609721675/job/17950326373#step:7:11

YuanbenWang · 2023-10-23T07:55:25Z

Thanks @YuanbenWang for the patch.

There are some checkstyle failures:

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
 185: Line is longer than 80 characters (found 83).
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java
 49: Variable 'quantileEnable' must be private and have accessor methods.
 79: Line is longer than 80 characters (found 85).
 82: Line is longer than 80 characters (found 82).
 83: Line is longer than 80 characters (found 86).

https://github.com/YuanbenWang/ozone/actions/runs/6609721675/job/17950326373#step:7:11

Thanks for the review~! I have fixed it and pushed the new PR.

YuanbenWang · 2023-10-24T06:13:52Z

@adoroszlai CI has been success in my own project. Could you plz help review this PR?

adoroszlai · 2023-10-24T06:38:49Z

@tanvipenumudy please review

xichen01

@YuanbenWang thanks for working on this! The change looks good. Just a few comment inlined.

xichen01 · 2023-11-08T10:44:15Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java

+          mutableQuantiles[i] = registry.newQuantiles(
+              value.toString() + "RpcTime" + intervals[i] + "s",
+              value.toString() + "rpc time in milli second",
+              "ops", "latency", intervals[i]);


It would be better to clarify the unit of time in the name of the Metrics, such as Use latencyMS

Now the name of the Metric will be like “InfoVolumeRpcTime127s90thPercentileLatency” and the unit of time is including in it as "s". It will be appreciated if you could tell me how to let it more suitable. How about "InfoVolumeRpcTime127latencySeconds90thPercentileLatency"? @xichen01

I think that InfoVolumeRpcTime127s90thPercentileLatencyMs is available, the 127s is the time of sampling, Ms is the unit of the Metrics value.

InfoVolumeRpcTime127latencySeconds90thPercentileLatency

Thank you for your feedback! I appreciate your clarification on the suggestion you provided. I now understand it better, and I will proceed to make the necessary modifications according to your advice.

Sorry for not expressing it clearly, I think we can remove the type from the Metrics name when we move it to the tag, so the name can be 127s90thPercentileLatencyMs

Suggested change

"ops", "latency", intervals[i]);

for (KEY value : values) {

MetricsRegistry registry = new MetricsRegistry(name + "MessageMetrics");

registries.put(value, registry);

//...

if (quantileEnable) {

mutableQuantiles[i] = registry.newQuantiles(

"_" + intervals[i] + "s_",

value.toString() + "rpc time in milli second",

"ops", "latencyMS", intervals[i]);

The Metrics name will be like this om_client_protocol_127s_99th_percentile_latency_ms{type="TransferLeadership",hostname="xxx"} 0 om_client_protocol_127s_num_ops{type="TransferLeadership",hostname="xxx"}
This name is consistent with the original Metrics style and is simpler.

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java

YuanbenWang · 2023-11-15T02:53:32Z

@xichen01 Thank you for reviewing my code and providing valuable suggestions. I have made the necessary modifications based on your feedback. Could you please review the updated code again?

xichen01 · 2023-11-16T07:48:34Z

Thanks @YuanbenWang, LGTM +1.

YuanbenWang · 2023-11-16T10:22:52Z

@xichen01 Thank you for the review and precious suggestions !

YuanbenWang · 2023-11-16T10:26:06Z

@adoroszlai Hello~! Could you please help trigger the CI? In my own project, it has been success.

adoroszlai · 2023-11-16T18:24:58Z

@duongkame @kerneltime @tanvipenumudy please review

YuanbenWang · 2023-11-27T04:49:54Z

@ChenSammi @xBis7 Hello! Could you please review this PR?

xBis7

@YuanbenWang Thanks for the patch. I've left some comments.

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java

YuanbenWang · 2024-01-02T16:46:58Z

@xBis7 I'm sorry for the long delay in responding to your review. I'v changed my code. Could you please review the updated code again?

xBis7 · 2024-01-03T11:01:47Z

@YuanbenWang Thanks for the changes. With ozone freon ockg, percentiles get updated

LGTM! I'm triggering the CI.

xBis7 · 2024-01-03T11:20:26Z

@YuanbenWang Can you add a simple unit test for ProtocolMessageMetrics.java? You can initialize everything and get the metrics like TestSCMHAMetrics, mock and stub everything else and check that values increment.

I think it's odd there are no tests for protocol message metrics.

YuanbenWang · 2024-01-03T12:02:00Z

@YuanbenWang Can you add a simple unit test for ProtocolMessageMetrics.java? You can initialize everything and get the metrics like TestSCMHAMetrics, mock and stub everything else and check that values increment.

I think it's odd there are no tests for protocol message metrics.

OK, I will do it in the next few days.

adoroszlai · 2024-04-10T13:50:34Z

Can you add a simple unit test for ProtocolMessageMetrics.java?

We should be checking metrics as part of normal functional tests, not in separate metrics-specific tests.

adoroszlai · 2024-04-22T11:42:22Z

/pending tests

github-actions

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

tests

github-actions · 2024-05-14T00:16:10Z

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

YuanbenWang · 2024-05-15T11:31:12Z

Can you add a simple unit test for ProtocolMessageMetrics.java?

We should be checking metrics as part of normal functional tests, not in separate metrics-specific tests.
@adoroszlai
I'm sorry for not updating this PR earlier for my own busy work. Could you please help reopen this PR now?

This pr is reviewed except I'm not sure if I should add unit tests and where they should be added. Could you give me some advice?

adoroszlai · 2024-05-15T11:36:13Z

Could you please help reopen this PR now?

"Reopen and comment" is disabled for me, it says "branch was force-pushed or recreated". Please feel free to open a new PR and reference this one. Sorry for the trouble.

YuanbenWang · 2024-05-15T11:38:32Z

Could you please help reopen this PR now?

"Reopen and comment" is disabled for me, it says "branch was force-pushed or recreated". Please feel free to open a new PR and reference this one. Sorry for the trouble.

Thank you for your help~! I will open new one

HDDS-9522. Add percentile for ProtocolMessageMetrics

e520901

YuanbenWang force-pushed the HDDS-9522 branch from 5a4e1ea to e520901 Compare October 24, 2023 02:42

adoroszlai requested review from duongkame and kerneltime October 24, 2023 06:38

adoroszlai added performance needs review labels Oct 30, 2023

xichen01 reviewed Nov 8, 2023

View reviewed changes

wangyuanben added 2 commits November 14, 2023 11:12

change the unit of time

b194f41

fix checkstyle

810d2ea

fix metrics name

e8df67c

xBis7 reviewed Nov 27, 2023

View reviewed changes

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java Outdated Show resolved Hide resolved

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java Outdated Show resolved Hide resolved

cleanup useless method

7f83f36

adoroszlai removed the needs review label Dec 11, 2023

put quantiles back after adding

e40b747

YuanbenWang force-pushed the HDDS-9522 branch from 5715cb4 to e40b747 Compare January 2, 2024 16:44

adoroszlai added the metrics label Mar 13, 2024

github-actions bot added the pending label Apr 22, 2024

github-actions bot requested changes Apr 22, 2024

View reviewed changes

github-actions bot closed this May 14, 2024

YuanbenWang mentioned this pull request May 15, 2024

HDDS-9522. Add percentile for ProtocolMessageMetrics #6681

Closed

-              "ops", "latency", intervals[i]);
+    for (KEY value : values) {
+      MetricsRegistry registry = new MetricsRegistry(name + "MessageMetrics");
+      registries.put(value, registry);
+      //...
+      if (quantileEnable) {
+          mutableQuantiles[i] = registry.newQuantiles(
+              "_" + intervals[i] + "s_",
+              value.toString() + "rpc time in milli second",
+              "ops", "latencyMS", intervals[i]);

HDDS-9522. Add percentile for ProtocolMessageMetrics #5479

HDDS-9522. Add percentile for ProtocolMessageMetrics #5479

Uh oh!

Conversation

YuanbenWang commented Oct 23, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai commented Oct 23, 2023

Uh oh!

YuanbenWang commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YuanbenWang commented Oct 24, 2023

Uh oh!

adoroszlai commented Oct 24, 2023

Uh oh!

xichen01 left a comment

Choose a reason for hiding this comment

Uh oh!

xichen01 Nov 8, 2023

Choose a reason for hiding this comment

Uh oh!

YuanbenWang Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xichen01 Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

YuanbenWang Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

xichen01 Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

YuanbenWang commented Nov 15, 2023

Uh oh!

xichen01 commented Nov 16, 2023

Uh oh!

YuanbenWang commented Nov 16, 2023

Uh oh!

YuanbenWang commented Nov 16, 2023

Uh oh!

adoroszlai commented Nov 16, 2023

Uh oh!

YuanbenWang commented Nov 27, 2023

Uh oh!

xBis7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

YuanbenWang commented Jan 2, 2024

Uh oh!

xBis7 commented Jan 3, 2024

Uh oh!

xBis7 commented Jan 3, 2024

Uh oh!

YuanbenWang commented Jan 3, 2024

Uh oh!

adoroszlai commented Apr 10, 2024

Uh oh!

adoroszlai commented Apr 22, 2024

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 14, 2024

Uh oh!

YuanbenWang commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adoroszlai commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YuanbenWang commented May 15, 2024

YuanbenWang commented Oct 23, 2023 •

edited

Loading

YuanbenWang Nov 13, 2023 •

edited

Loading

YuanbenWang commented May 15, 2024 •

edited

Loading

adoroszlai commented May 15, 2024 •

edited

Loading