4.1 dev slf4j metric attributes #1275

fabienrenaud · 2018-02-21T20:21:41Z

arteam

Looks good, I've left some comments.

arteam · 2018-02-23T09:26:42Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

        }
    }

    private final LoggerProxy loggerProxy;
    private final Marker marker;
    private final String prefix;
+    private final ThreadLocal<StringBuilder> threadLocalStringBuilder = ThreadLocal.withInitial(StringBuilder::new);


I feel that using a ThreadLocal is a premature optimization. It's not clear for me that it will provide a big performance boost over allocating a new StringBuilder. Also, using ThreadLocal can cause strange classloader leaks if one fail to clear a reference to it.

removed ThreadLocal. StringBuilder has to be a local variable to the report method since there is no guarantee the method won't be executed in parallel.

arteam · 2018-02-23T09:29:10Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

            }

-            for (Entry<String, Counter> entry : counters.entrySet()) {
-                logCounter(entry.getKey(), entry.getValue());
+            if (!getDisabledMetricAttributes().contains(COUNT)) {


count is not used to filtering counters. It's used only to filter the count attribute of histograms, meters, and timers. Users can filter counters by MetricFilter.

removed if.

arteam · 2018-02-23T09:38:03Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

+        }
+    }
+
+    private void append(StringBuilder b, String key, long value) {


It seems to me that perfomance benefits of not conveting primtives to wrappers are slim to have 4 apennd methods. I think one method accepting an Object should be enough, especially considering that in most cases we use Supplier which forces auto-boxing.

I think they are worth keeping:

they are very simple methods, easy to read, understand, change. no hidden code complexity here.

convertDuration and convertRate return double so only having an append(...,Object) could cause extra auto-boxing

the JVM can likely optimize primitive -> box(primitive) -> primitive call chains

and yes, StringBuilder.append(long) is slightly more efficient than StringBuilder.append(Object) called with a Long (unless the JVM un-erases types for some core classes? -- Hotspot JVM does some surprising optimizations some times :) ).

I can remove them if you feel strongly about it but now that the code is there, I think it's worth keeping.

Fair enough

arteam · 2018-02-23T09:42:05Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

 import java.util.SortedMap;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Supplier;
+
+import static com.codahale.metrics.MetricAttribute.*;


Please don't use star imports, in the codebase we tend to use direct imports.

arteam · 2018-02-23T09:43:55Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

+         * Don't report the passed metric attributes for all metrics (e.g. "p999", "stddev" or "m15").
+         * See {@link MetricAttribute}.
+         *
+         * @param disabledMetricAttributes a {@link MetricFilter}


s/MetricFilter/a set of MetricAttribute

fixed. fixed GraphiteReporter as well.

arteam · 2018-02-23T09:44:32Z

metrics-core/src/test/java/com/codahale/metrics/Slf4jReporterTest.java

-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
+import static com.codahale.metrics.MetricAttribute.*;


Please replace the star imports with direct imports.

arteam · 2018-02-23T10:04:19Z

metrics-core/src/test/java/com/codahale/metrics/Slf4jReporterTest.java

-            .filter(MetricFilter.ALL)
-            .build();
+
+    private Set<MetricAttribute> disabledMetricAttributes = null;


Instead of declaring disabledMetricAttributes as a field, I think we should it make a parameter to the infoReporter and errorReporter methods. Othwerise it's not clear how what's a relationship between it and the actual reporter.

I like to use this pattern to override default values passed to the constructor of the main unit tested component. I think in the context of unit tests it makes sense since there is only 1 class that is supposed to be tested. In the context of Slf4jReporter, it should be clear that the only way of testing Slf4jReporter is to use its builder and that its builder takes an optional Set<MetricAttribute> disabledMetricAttributes.
This pattern becomes particularly handy when there might be more than 1 default value to overwrite for some tests. The alternatives would be to either invoke the full builder with all the proper default values set but this leads to code duplication; or to add more arguments to the infoReporter and errorReporter factory methods to overwrite the default value but this doesn't scale well when more than a couple of variables needs their default values overwritten and the factory method may end up having as many parameters as the builder...
I'll add a comment clarifying this is used to build the reporter in each test and can be overridden. I will also move at the top of the infoReporter and errorReporter methods to hep identify the relationship.

arteam · 2018-02-23T10:06:12Z

metrics-core/src/test/java/com/codahale/metrics/Slf4jReporterTest.java

+    }
+
+    private void verifyError(String expectedLog) {
+        if (expectedLog == null) {


Do we really need conditional checks if we use it only in one place?

this is no longer necessary since attribute filtering doesn't apply to counters

arteam · 2018-02-23T10:06:33Z

metrics-core/src/test/java/com/codahale/metrics/Slf4jReporterTest.java

+
+    private void verifyInfo(String expectedLog) {
+        if (expectedLog == null) {
+            verify(logger, never()).info(marker, expectedLog);


It looks like this branch of the execution isn't used at all.

the idea was to be consistent with how tests were written.
removed.

fabienrenaud · 2018-02-23T15:52:50Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

 import java.util.SortedMap;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Supplier;
+
+import static com.codahale.metrics.MetricAttribute.*;


fabienrenaud · 2018-02-23T15:53:01Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

+         * Don't report the passed metric attributes for all metrics (e.g. "p999", "stddev" or "m15").
+         * See {@link MetricAttribute}.
+         *
+         * @param disabledMetricAttributes a {@link MetricFilter}


fixed. fixed GraphiteReporter as well.

fabienrenaud · 2018-02-23T15:54:40Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

        }
    }

    private final LoggerProxy loggerProxy;
    private final Marker marker;
    private final String prefix;
+    private final ThreadLocal<StringBuilder> threadLocalStringBuilder = ThreadLocal.withInitial(StringBuilder::new);


removed ThreadLocal. StringBuilder has to be a local variable to the report method since there is no guarantee the method won't be executed in parallel.

fabienrenaud · 2018-02-23T15:54:47Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

            }

-            for (Entry<String, Counter> entry : counters.entrySet()) {
-                logCounter(entry.getKey(), entry.getValue());
+            if (!getDisabledMetricAttributes().contains(COUNT)) {


removed if.

fabienrenaud · 2018-02-23T16:07:05Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

+        }
+    }
+
+    private void append(StringBuilder b, String key, long value) {


I think they are worth keeping:

they are very simple methods, easy to read, understand, change. no hidden code complexity here.

convertDuration and convertRate return double so only having an append(...,Object) could cause extra auto-boxing

the JVM can likely optimize primitive -> box(primitive) -> primitive call chains

and yes, StringBuilder.append(long) is slightly more efficient than StringBuilder.append(Object) called with a Long (unless the JVM un-erases types for some core classes? -- Hotspot JVM does some surprising optimizations some times :) ).

I can remove them if you feel strongly about it but now that the code is there, I think it's worth keeping.

fabienrenaud · 2018-02-23T16:09:07Z

metrics-core/src/test/java/com/codahale/metrics/Slf4jReporterTest.java

+
+    private void verifyInfo(String expectedLog) {
+        if (expectedLog == null) {
+            verify(logger, never()).info(marker, expectedLog);


the idea was to be consistent with how tests were written.
removed.

fabienrenaud · 2018-02-23T16:09:59Z

metrics-core/src/test/java/com/codahale/metrics/Slf4jReporterTest.java

+    }
+
+    private void verifyError(String expectedLog) {
+        if (expectedLog == null) {


this is no longer necessary since attribute filtering doesn't apply to counters

fabienrenaud · 2018-02-23T16:12:44Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

+        }
+    }
+
+    private void appendLongIfEnabled(StringBuilder b, MetricAttribute metricAttribute, Supplier<Long> durationSupplier) {


renamed durationSupplier here since this isn't a duration.

fabienrenaud · 2018-02-23T16:12:57Z

metrics-core/src/main/java/com/codahale/metrics/Slf4jReporter.java

+        }
+    }
+
+    private void appendDoubleIfEnabled(StringBuilder b, MetricAttribute metricAttribute, Supplier<Double> durationSupplier) {


fabienrenaud · 2018-02-23T16:23:14Z

metrics-core/src/test/java/com/codahale/metrics/Slf4jReporterTest.java

-            .filter(MetricFilter.ALL)
-            .build();
+
+    private Set<MetricAttribute> disabledMetricAttributes = null;


I like to use this pattern to override default values passed to the constructor of the main unit tested component. I think in the context of unit tests it makes sense since there is only 1 class that is supposed to be tested. In the context of Slf4jReporter, it should be clear that the only way of testing Slf4jReporter is to use its builder and that its builder takes an optional Set<MetricAttribute> disabledMetricAttributes.
This pattern becomes particularly handy when there might be more than 1 default value to overwrite for some tests. The alternatives would be to either invoke the full builder with all the proper default values set but this leads to code duplication; or to add more arguments to the infoReporter and errorReporter factory methods to overwrite the default value but this doesn't scale well when more than a couple of variables needs their default values overwritten and the factory method may end up having as many parameters as the builder...
I'll add a comment clarifying this is used to build the reporter in each test and can be overridden. I will also move at the top of the infoReporter and errorReporter methods to hep identify the relationship.

arteam · 2018-02-25T20:51:43Z

Thank you for the contribution!

* Support for disabledMetricAttributes in Slf4jReporter * Tests for metric attribute filter in Slf4jReporter * Slf4jReporter: addressed PR feedback

fabienrenaud · 2018-10-13T18:29:45Z

@arteam Could you please release this change? It's been 8 months...

arteam · 2018-10-15T06:11:31Z

Sure, will do a 4.1.0 release this week.

fabienrenaud added 2 commits February 21, 2018 11:44

Support for disabledMetricAttributes in Slf4jReporter

3968e0e

Tests for metric attribute filter in Slf4jReporter

1b39f3f

arteam reviewed Feb 23, 2018

View reviewed changes

Slf4jReporter: addressed PR feedback

d3ccf74

fabienrenaud commented Feb 23, 2018

View reviewed changes

arteam merged commit de99b11 into dropwizard:4.1-development Feb 25, 2018

arteam mentioned this pull request Feb 25, 2018

Expansion filter for Slf4jReporter #1273

Closed

arteam added the improvement label Feb 25, 2018

arteam modified the milestones: 4.1.0, 4.0.1 Feb 25, 2018

fabienrenaud deleted the 4.1-dev-slf4j-metricAttributes branch February 26, 2018 17:13

chids pushed a commit to chids/metrics that referenced this pull request Mar 16, 2018

4.1 dev slf4j metric attributes (dropwizard#1275)

7542100

* Support for disabledMetricAttributes in Slf4jReporter * Tests for metric attribute filter in Slf4jReporter * Slf4jReporter: addressed PR feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4.1 dev slf4j metric attributes #1275

4.1 dev slf4j metric attributes #1275

fabienrenaud commented Feb 21, 2018

arteam left a comment

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 25, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

fabienrenaud Feb 23, 2018

arteam commented Feb 25, 2018

fabienrenaud commented Oct 13, 2018

arteam commented Oct 15, 2018

4.1 dev slf4j metric attributes #1275

4.1 dev slf4j metric attributes #1275

Conversation

fabienrenaud commented Feb 21, 2018

arteam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arteam commented Feb 25, 2018

fabienrenaud commented Oct 13, 2018

arteam commented Oct 15, 2018