advancedxy commented on code in PR #1991:
URL:
https://github.com/apache/incubator-uniffle/pull/1991#discussion_r1701809847
##########
common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java:
##########
@@ -112,4 +150,8 @@ public Summary.Child addLabeledSummary(String name) {
}
return builder.register(collectorRegistry).labels(defaultLabelValues);
}
+
+ public com.codahale.metrics.Gauge getGauge(String name) {
Review Comment:
Never used?
##########
common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java:
##########
@@ -44,6 +51,8 @@ public MetricsManager(CollectorRegistry collectorRegistry,
Map<String, String> d
} else {
this.collectorRegistry = collectorRegistry;
}
+ metricRegistry = new MetricRegistry();
Review Comment:
Nit: -> `this.metricRegistry = xxx`.
It's clear to explicitly use `this.xx` when updating the instance field.
##########
common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java:
##########
@@ -66,14 +75,43 @@ public Counter.Child addLabeledCounter(String name) {
return c.labels(this.defaultLabelValues);
}
+ @Deprecated
Review Comment:
I would like to add a javadoc to indicate which version this method will be
removed and what alternative should be used.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java:
##########
@@ -442,13 +438,7 @@ private static void setUpMetrics(ShuffleServerConf
serverConf) {
gaugeInFlushBufferSize =
metricsManager.addLabeledGauge(IN_FLUSH_BUFFER_SIZE);
gaugeUsedBufferSize = metricsManager.addLabeledGauge(USED_BUFFER_SIZE);
gaugeReadBufferUsedSize =
metricsManager.addLabeledGauge(READ_USED_BUFFER_SIZE);
- gaugeUsedDirectMemorySize =
metricsManager.addLabeledGauge(USED_DIRECT_MEMORY_SIZE);
- gaugeUsedDirectMemorySizeByNetty =
- metricsManager.addLabeledGauge(USED_DIRECT_MEMORY_SIZE_BY_NETTY);
- gaugeUsedDirectMemorySizeByGrpcNetty =
- metricsManager.addLabeledGauge(USED_DIRECT_MEMORY_SIZE_BY_GRPC_NETTY);
gaugeWriteHandler = metricsManager.addLabeledGauge(TOTAL_WRITE_HANDLER);
- gaugeEventQueueSize = metricsManager.addLabeledGauge(EVENT_QUEUE_SIZE);
Review Comment:
hmm. I think the default lables are lost by replacing this with codahale
gauge.
addLabeledGauge will add the default label and value to a gauge.
##########
common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java:
##########
@@ -19,20 +19,27 @@
import java.util.Arrays;
import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import com.codahale.metrics.CachedGauge;
+import com.codahale.metrics.MetricRegistry;
import com.google.common.collect.Maps;
import io.prometheus.client.CollectorRegistry;
import io.prometheus.client.Counter;
import io.prometheus.client.Gauge;
import io.prometheus.client.Histogram;
import io.prometheus.client.Summary;
+import io.prometheus.client.dropwizard.DropwizardExports;
public class MetricsManager {
+ private final MetricRegistry metricRegistry;
private final CollectorRegistry collectorRegistry;
private final String[] defaultLabelNames;
private final String[] defaultLabelValues;
private static final double[] QUANTILES = {0.50, 0.75, 0.90, 0.95, 0.99};
private static final double QUANTILE_ERROR = 0.01;
+ private static final String LABEL_SEPARATOR = ":";
+ private static final String NAME_SEPARATOR = "_";
Review Comment:
Seems these two are never used?
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java:
##########
@@ -513,4 +503,8 @@ private static void setUpMetrics(ShuffleServerConf
serverConf) {
.labelNames("app_id")
.register(metricsManager.getCollectorRegistry());
}
+
+ public static MetricsManager getMetricsManager() {
Review Comment:
I don't think we should expose metrics manager directly.
Instead, you could add methods in ShuffleServerMetrics to register new
metrics, such as:
```java
public static void
registerEventQueueSize(com.codahale.metrics.Gauge<Integer> eventQueueSize) {
// do all kinds of validation if needed.
metricsManager.registerGaugeIfAbsent(EVENT_QUEUE_SIZE, eventQueueSize);
}
```
Or we can expose the metrics names and the `registerMetrics` method combined.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
