You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "YuanbenWang (via GitHub)" <gi...@apache.org> on 2023/10/23 07:02:45 UTC
[PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
YuanbenWang opened a new pull request, #5479:
URL: https://github.com/apache/ozone/pull/5479
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1391918890
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ registry = new MetricsRegistry(name + "MessageMetrics");
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ value.toString() + "RpcTime" + intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latency", intervals[i]);
Review Comment:
I think that `InfoVolumeRpcTime127s90thPercentileLatencyMs` is available, the `127s` is the time of sampling, `Ms` is the unit of the Metrics value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1811728901
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1409212297
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
Thank you for reviewing this pr~!
The value is updated in this method.
In local test, it does work like the picture below.
<img width="330" alt="metrics_before" src="https://github.com/apache/ozone/assets/48795318/94901b11-deb1-479d-9b6a-def505c1b93d">
For the 127s is the time of sampling, the value will decrease if no RPC comes, just like the picture below.
Therefore, the value is able to be updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1439616902
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
@xBis7 @tanvipenumudy Thank you for your valuable feedback. I will put the new value back in the map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1875187858
@YuanbenWang Thanks for the changes. With `ozone freon ockg`, percentiles get updated
<br/><br/>
<img src="https://github.com/apache/ozone/assets/74301312/a1b3e35a-4ac0-4dee-a7dd-711eb7946369" width="500">
<br/><br/>
<img src="https://github.com/apache/ozone/assets/74301312/992c72ce-7c66-4690-b605-247a09ecae8b" width="500">
<br/><br/>
<img src="https://github.com/apache/ozone/assets/74301312/ef5924aa-ceb4-4b31-a1ce-3187a8dfbf52" width="500">
<br/><br/>
LGTM! I'm triggering the CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1875211476
@YuanbenWang Can you add a simple unit test for `ProtocolMessageMetrics.java`? You can initialize everything and get the metrics like [TestSCMHAMetrics](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMHAMetrics.java), mock and stub everything else and check that values increment.
I think it's odd there are no tests for protocol message metrics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1813942479
Thanks @YuanbenWang, LGTM +1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1409212297
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
The value is updated in this method.
In local test, it does work like the picture below.
<img width="330" alt="metrics_before" src="https://github.com/apache/ozone/assets/48795318/94901b11-deb1-479d-9b6a-def505c1b93d">
For the 127s is the time of sampling, the value will decrease if no RPC comes, just like the picture below.
Therefore, the value is able to be updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1827120926
@ChenSammi @xBis7 Hello! Could you please review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1409188507
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -129,4 +168,8 @@ public String description() {
return description;
}
}
+
+ public boolean isQuantileEnable() {
Review Comment:
OK, I will clean it up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-2112284651
> > 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.
I'm sorry for not updating this PR earlier for my own busy work. Could you please help reopen this PR now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1391932090
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ registry = new MetricsRegistry(name + "MessageMetrics");
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ value.toString() + "RpcTime" + intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latency", intervals[i]);
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1393649737
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ registry = new MetricsRegistry(name + "MessageMetrics");
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ value.toString() + "RpcTime" + intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latency", intervals[i]);
Review Comment:
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`
```suggestion
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1874257071
@xBis7 I'm sorry for the long delay in responding to your review. I'v changed my code and passed the CI successfully in my project. Could you please review the updated code again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1776615053
@tanvipenumudy please review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1406549126
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -129,4 +168,8 @@ public String description() {
return description;
}
}
+
+ public boolean isQuantileEnable() {
Review Comment:
This doesn't seem used anywhere.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
Since using a Map for tracking the quantiles and accessing them, shouldn't you also update the value on the map? You get the `MutableQuantiles` from the Map, update their value but don't put the new value back in the Map.
I haven't tested it locally but it seems that it will always read the same original value because it doesn't get updated on the map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1409212297
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
@xBis7 Thank you for reviewing this pr~!The value is updated in this method.
In local test, it does work like the picture below.
<img width="330" alt="metrics_before" src="https://github.com/apache/ozone/assets/48795318/94901b11-deb1-479d-9b6a-def505c1b93d">
For the 127s is the time of sampling, the value will decrease if no RPC comes, just like the picture below.
Therefore, the value is able to be updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1415657254
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
I tested it locally. `counter`, `time` and other fields get updated but percentiles are always 0 just like your screenshot.
I think that after `q.add(delta);` you should put the new value back in the map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1390856064
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -89,6 +126,7 @@ public void unregister() {
@Override
public void getMetrics(MetricsCollector collector, boolean all) {
+ registry.snapshot(collector.addRecord(registry.info()), all);
Review Comment:
Good idea! I agree with your points, and I'll incorporate the suggested improvements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1390871149
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ registry = new MetricsRegistry(name + "MessageMetrics");
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ value.toString() + "RpcTime" + intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latency", intervals[i]);
Review Comment:
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. @xichen01
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-2069180802
/pending tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1815003839
@duongkame @kerneltime @tanvipenumudy please review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1774624568
> 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 push the new PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1386451752
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -89,6 +126,7 @@ public void unregister() {
@Override
public void getMetrics(MetricsCollector collector, boolean all) {
+ registry.snapshot(collector.addRecord(registry.info()), all);
Review Comment:
It is better to put the type of operation in the tag rather than the name of the metric, as with other metrics. This makes it easier to write SQL for monitoring such as Grafana.
```
om_client_protocol_message_metrics_time15s50th_percentile_latency{hostname="xxx", type="CreateKey"} 0
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ registry = new MetricsRegistry(name + "MessageMetrics");
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ value.toString() + "RpcTime" + intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latency", intervals[i]);
Review Comment:
It would be better to clarify the unit of time in the name of the Metrics, such as Use `latencyMS`
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
Review Comment:
The `ProtocolMessageMetrics` constructor can be made private, and we have a `create` for creating `ProtocolMessageMetrics`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-2112293101
> Could you please help reopen this PR now?
"Reopen and comment" is disabled for me, it says "branch was force-pushed or recreated".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-2109039759
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](https://github.com/apache/hadoop-ozone#contact) on the mailing list or the slack channel."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5479: HDDS-9522. Add percentile for ProtocolMessageMetrics
URL: https://github.com/apache/ozone/pull/5479
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1814168958
@adoroszlai Hello~! Could you please help trigger the CI? In my own project, it has been success.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1390857398
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
Review Comment:
OK, I will do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1390871149
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -40,32 +44,59 @@ public class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String description;
+ private final MetricsRegistry registry;
+
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ registry = new MetricsRegistry(name + "MessageMetrics");
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ value.toString() + "RpcTime" + intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latency", intervals[i]);
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1875265874
> @YuanbenWang Can you add a simple unit test for `ProtocolMessageMetrics.java`? You can initialize everything and get the metrics like [TestSCMHAMetrics](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMHAMetrics.java), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-2047606135
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-2112297122
> > 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "tanvipenumudy (via GitHub)" <gi...@apache.org>.
tanvipenumudy commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1429650075
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
Yes, I agree with @xBis7.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1874275008
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5479:
URL: https://github.com/apache/ozone/pull/5479#discussion_r1406582582
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -22,50 +22,80 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsTag;
import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.ratis.util.UncheckedAutoCloseable;
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public final class ProtocolMessageMetrics<KEY> implements MetricsSource {
private final String name;
private final String description;
+ private final boolean quantileEnable;
+
private final Map<KEY, AtomicLong> counters =
new ConcurrentHashMap<>();
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();
+ private final Map<KEY, MutableQuantiles[]> quantiles =
+ new ConcurrentHashMap<>();
+
private final AtomicInteger concurrency = new AtomicInteger(0);
public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
- String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ String description, KEY[] types, ConfigurationSource conf) {
+ return new ProtocolMessageMetrics<KEY>(name, description, types, conf);
}
- public ProtocolMessageMetrics(String name, String description,
- KEY[] values) {
+ private ProtocolMessageMetrics(String name, String description,
+ KEY[] values, ConfigurationSource conf) {
this.name = name;
this.description = description;
+ int[] intervals = conf.getInts(
+ OzoneConfigKeys.OZONE_PROTOCOL_MESSAGE_METRICS_PERCENTILES_INTERVALS);
+ quantileEnable = (intervals.length > 0);
for (KEY value : values) {
counters.put(value, new AtomicLong(0));
elapsedTimes.put(value, new AtomicLong(0));
+ if (quantileEnable) {
+ MetricsRegistry registry =
+ new MetricsRegistry(value.toString() + "MessageMetrics");
+ MutableQuantiles[] mutableQuantiles =
+ new MutableQuantiles[intervals.length];
+ quantiles.put(value, mutableQuantiles);
+ for (int i = 0; i < intervals.length; i++) {
+ mutableQuantiles[i] = registry.newQuantiles(
+ intervals[i] + "s",
+ value.toString() + "rpc time in milli second",
+ "ops", "latencyMs", intervals[i]);
+ }
+ }
}
}
public void increment(KEY key, long duration) {
counters.get(key).incrementAndGet();
elapsedTimes.get(key).addAndGet(duration);
+ if (quantileEnable) {
+ for (MutableQuantiles q : quantiles.get(key)) {
Review Comment:
Do we usually keep track of the quantile values?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1776593006
@adoroszlai CI has been success in my own project. Could you plz help review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1774608237
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9522. Add percentile for ProtocolMessageMetrics [ozone]
Posted by "YuanbenWang (via GitHub)" <gi...@apache.org>.
YuanbenWang commented on PR #5479:
URL: https://github.com/apache/ozone/pull/5479#issuecomment-1814163677
@xichen01 Thank you for the review and precious suggestions !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org