apoorvmittal10 commented on code in PR #22129:
URL: https://github.com/apache/kafka/pull/22129#discussion_r3324162972
##########
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##########
@@ -124,4 +124,25 @@ public void config(MetricConfig config) {
this.config = config;
}
}
+
+ /**
+ * Returns a human-readable representation of this metric, primarily
useful for logging
+ * in contexts like {@link MetricsReporter#metricChange(KafkaMetric)}.
+ *
+ * <p>The metricValueProvider is represented by its simple class name
rather than its full
+ * toString() to avoid dumping internal stat state (e.g. SampledStat's
samples list) into
+ * logs, which could be verbose and is rarely useful for identifying which
metric changed.
+ */
+ @Override
+ public String toString() {
+ Class<?> cls = metricValueProvider.getClass();
+ if (cls.isSynthetic() || cls.isAnonymousClass()) {
+ return "KafkaMetric [metricName=" + metricName + "]";
+ }
+
+ return "KafkaMetric [" +
+ "metricName=" + metricName +
+ ", metricValueProvider=" +
metricValueProvider.getClass().getSimpleName() +
Review Comment:
Simple name will strip the package, shall we use getName() instead?
```suggestion
", metricValueProvider=" +
metricValueProvider.getClass().getName() +
```
##########
clients/src/test/java/org/apache/kafka/common/metrics/KafkaMetricTest.java:
##########
@@ -30,20 +31,26 @@
public class KafkaMetricTest {
- private static final MetricName METRIC_NAME = new MetricName("name",
"group", "description", Collections.emptyMap());
+ private static final MetricName METRIC_NAME_1 = new MetricName("name",
"group", "description", Collections.emptyMap());
Review Comment:
```suggestion
private static final MetricName METRIC_NAME_1 = new MetricName("name",
"group", "description", Map.of());
```
##########
clients/src/test/java/org/apache/kafka/common/metrics/KafkaMetricTest.java:
##########
@@ -70,7 +77,62 @@ public void
testKafkaMetricAcceptsNonMeasurableNonGaugeProvider() {
@Test
public void testConstructorWithNullProvider() {
assertThrows(NullPointerException.class, () ->
- new KafkaMetric(new Object(), METRIC_NAME, null, new
MetricConfig(), new MockTime())
+ new KafkaMetric(new Object(), METRIC_NAME_1, null, new
MetricConfig(), new MockTime())
);
}
+
+ /**
+ * Verifies that toString produces a human-readable representation
suitable for logging,
+ * e.g. in {@link
org.apache.kafka.common.metrics.MetricsReporter#metricChange(KafkaMetric)}.
+ * Note that we skip the metric provider in this case, but this is still a
+ * significant improvement over the default Object.toString() output (e.g.
"KafkaMetric@62a7d6c6").
+ */
+ @Test
+ public void testToStringWithLambdaProvider() {
+ Measurable metricValueProvider = (config, now) -> 0;
+ testToStringOnLambdaOrAnonymousClass(metricValueProvider);
+ }
+
+ /**
+ * Verifies that toString produces a human-readable representation
suitable for logging,
+ * e.g. in {@link
org.apache.kafka.common.metrics.MetricsReporter#metricChange(KafkaMetric)}.
+ * Note that we skip the metric provider in this case, but this is still a
+ * significant improvement over the default Object.toString() output (e.g.
"KafkaMetric@62a7d6c6").
+ */
+ @Test
+ public void testToStringWithAnonymousClassProvider() {
+ //noinspection Convert2Lambda
+ Measurable metricValueProvider = new Measurable() {
+ @Override
+ public double measure(MetricConfig config, long now) {
+ return 0;
+ }
+ };
+ testToStringOnLambdaOrAnonymousClass(metricValueProvider);
+ }
+
+ private void testToStringOnLambdaOrAnonymousClass(Measurable
metricValueProvider) {
+ KafkaMetric metric = new KafkaMetric(
+ new Object(), METRIC_NAME_2, metricValueProvider, new
MetricConfig(), new MockTime());
+
+ String result = metric.toString();
+ assertEquals("KafkaMetric [metricName=MetricName
[name=request-latency-avg, "
+ + "group=consumer-fetch-manager-metrics, "
+ + "description=The average request latency in ms, "
+ + "tags={client-id=consumer-1}]]",
+ result);
+ }
Review Comment:
Move this method after public methods for readability.
##########
clients/src/test/java/org/apache/kafka/common/metrics/KafkaMetricTest.java:
##########
@@ -70,7 +77,62 @@ public void
testKafkaMetricAcceptsNonMeasurableNonGaugeProvider() {
@Test
public void testConstructorWithNullProvider() {
assertThrows(NullPointerException.class, () ->
- new KafkaMetric(new Object(), METRIC_NAME, null, new
MetricConfig(), new MockTime())
+ new KafkaMetric(new Object(), METRIC_NAME_1, null, new
MetricConfig(), new MockTime())
);
}
+
+ /**
+ * Verifies that toString produces a human-readable representation
suitable for logging,
+ * e.g. in {@link
org.apache.kafka.common.metrics.MetricsReporter#metricChange(KafkaMetric)}.
+ * Note that we skip the metric provider in this case, but this is still a
+ * significant improvement over the default Object.toString() output (e.g.
"KafkaMetric@62a7d6c6").
Review Comment:
There are no such logs currently in any Kafka implementation of
MetricsReporter#metricChange(KafkaMetric) hence this line is confusing, please
remove it.
##########
clients/src/test/java/org/apache/kafka/common/metrics/KafkaMetricTest.java:
##########
@@ -70,7 +77,62 @@ public void
testKafkaMetricAcceptsNonMeasurableNonGaugeProvider() {
@Test
public void testConstructorWithNullProvider() {
assertThrows(NullPointerException.class, () ->
- new KafkaMetric(new Object(), METRIC_NAME, null, new
MetricConfig(), new MockTime())
+ new KafkaMetric(new Object(), METRIC_NAME_1, null, new
MetricConfig(), new MockTime())
);
}
+
+ /**
+ * Verifies that toString produces a human-readable representation
suitable for logging,
+ * e.g. in {@link
org.apache.kafka.common.metrics.MetricsReporter#metricChange(KafkaMetric)}.
+ * Note that we skip the metric provider in this case, but this is still a
+ * significant improvement over the default Object.toString() output (e.g.
"KafkaMetric@62a7d6c6").
+ */
+ @Test
+ public void testToStringWithLambdaProvider() {
+ Measurable metricValueProvider = (config, now) -> 0;
+ testToStringOnLambdaOrAnonymousClass(metricValueProvider);
+ }
+
+ /**
+ * Verifies that toString produces a human-readable representation
suitable for logging,
+ * e.g. in {@link
org.apache.kafka.common.metrics.MetricsReporter#metricChange(KafkaMetric)}.
+ * Note that we skip the metric provider in this case, but this is still a
+ * significant improvement over the default Object.toString() output (e.g.
"KafkaMetric@62a7d6c6").
+ */
+ @Test
+ public void testToStringWithAnonymousClassProvider() {
+ //noinspection Convert2Lambda
+ Measurable metricValueProvider = new Measurable() {
+ @Override
+ public double measure(MetricConfig config, long now) {
+ return 0;
+ }
+ };
+ testToStringOnLambdaOrAnonymousClass(metricValueProvider);
+ }
+
+ private void testToStringOnLambdaOrAnonymousClass(Measurable
metricValueProvider) {
+ KafkaMetric metric = new KafkaMetric(
+ new Object(), METRIC_NAME_2, metricValueProvider, new
MetricConfig(), new MockTime());
+
+ String result = metric.toString();
+ assertEquals("KafkaMetric [metricName=MetricName
[name=request-latency-avg, "
+ + "group=consumer-fetch-manager-metrics, "
+ + "description=The average request latency in ms, "
+ + "tags={client-id=consumer-1}]]",
+ result);
+ }
+
+ @Test
+ public void testToStringWithRealStat() {
Review Comment:
```suggestion
public void testToStringWithStatProvider() {
```
##########
clients/src/test/java/org/apache/kafka/common/metrics/KafkaMetricTest.java:
##########
@@ -30,20 +31,26 @@
public class KafkaMetricTest {
- private static final MetricName METRIC_NAME = new MetricName("name",
"group", "description", Collections.emptyMap());
+ private static final MetricName METRIC_NAME_1 = new MetricName("name",
"group", "description", Collections.emptyMap());
+ private static final MetricName METRIC_NAME_2 = new MetricName(
+ "request-latency-avg",
+ "consumer-fetch-manager-metrics",
+ "The average request latency in ms",
+ Collections.singletonMap("client-id", "consumer-1")
Review Comment:
```suggestion
Map.of("client-id", "consumer-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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
