Add ability to config Timer and DistributionSummary with user defined… #1176

huydx · 2018-05-02T04:58:27Z

… config

codecov · 2018-05-02T05:27:37Z

Codecov Report

Merging #1176 into master will decrease coverage by 0.06%.
The diff coverage is 91.3%.

@@            Coverage Diff             @@
##           master    #1176      +/-   ##
==========================================
- Coverage    72.7%   72.63%   -0.07%     
==========================================
  Files         518      517       -1     
  Lines       23259    23282      +23     
  Branches     2910     2910              
==========================================
+ Hits        16910    16911       +1     
- Misses       4795     4823      +28     
+ Partials     1554     1548       -6

Impacted Files	Coverage Δ
.../armeria/internal/metric/RequestMetricSupport.java	`75.94% <100%> (ø)`	⬆️
...com/linecorp/armeria/common/metric/MoreMeters.java	`75% <90.24%> (-22.15%)`	⬇️
...necorp/armeria/server/cors/CorsServiceBuilder.java	`28.88% <0%> (-8.89%)`	⬇️
.../com/linecorp/armeria/server/cors/CorsService.java	`53% <0%> (-4.74%)`	⬇️
...linecorp/armeria/internal/Http2GoAwayListener.java	`76% <0%> (-4%)`	⬇️
...m/linecorp/armeria/client/HttpResponseDecoder.java	`82.05% <0%> (-1.29%)`	⬇️
...corp/armeria/common/logging/DefaultRequestLog.java	`79.94% <0%> (ø)`	⬆️
...om/linecorp/armeria/spring/ThriftServiceUtils.java
...linecorp/armeria/client/HttpRequestSubscriber.java	`67.15% <0%> (+0.72%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b79f25c...44f6dd6. Read the comment docs.

trustin · 2018-05-02T05:30:22Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java


 /**
 * Provides utilities for accessing {@link MeterRegistry}.
 */
 public final class MoreMeters {

+    private static DistributionStatisticConfig distributionStatisticConfig;
+
+    public static void withConfig(DistributionStatisticConfig config) {


Missing Javadoc

Rename to something like setDistributionStatisticConfig()?

requireNonNull(config, "config")

Add the getter as well: public static DistributionStatisticConfig distributionStatisticConfig()

trustin · 2018-05-02T05:30:31Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java


 /**
 * Provides utilities for accessing {@link MeterRegistry}.
 */
 public final class MoreMeters {

+    private static DistributionStatisticConfig distributionStatisticConfig;


Missing volatile

Could we set the non-null default config so that it never becomes null?

trustin · 2018-05-02T05:30:53Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+     * If distributionStatisticConfig is set, use the config.
+     * If the config is not set, use default percentile.
+     **/
+    public static DistributionSummary defaultDistributionSummary(MeterRegistry registry,


How about just newDistributionSummary?

trustin · 2018-05-02T05:31:39Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+                                      .tags(tags)
+                                      .publishPercentiles(distributionStatisticConfig.getPercentiles())
+                                      .publishPercentileHistogram(
+                                              distributionStatisticConfig.isPercentileHistogram())


How about renaming to a shorter name so the line does not wrap? e.g. distStatCfg?

trustin · 2018-05-02T05:36:40Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

-                                                                  String name, Iterable<Tag> tags) {
+     * Returns a newly-registered {@link DistributionSummary}.
+     * If distributionStatisticConfig is set, use the config.
+     * If the config is not set, use default percentile.


If .... If .... could be removed and the necessary documentation could be put in the getter and setter of distributionStatisticConfig.

trustin · 2018-05-02T05:37:18Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

    }

    /**
     * Returns a newly-registered {@link Timer} with percentile publication configured.
+     * If distributionStatisticConfig is set, use the config.
+     * If the config is not set, use default percentile.


If .... If .... could be removed and the necessary documentation could be put in the getter and setter of distributionStatisticConfig.

trustin · 2018-05-02T05:37:32Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+                                      .tags(tags)
+                                      .publishPercentiles(PERCENTILES)
+                                      .register(registry);
+        }
    }

    /**
     * Returns a newly-registered {@link Timer} with percentile publication configured.


Replace with percentile ... with with the current {@linkplain #distributionStatisticConfig() distribution statistic configuration}.

trustin · 2018-05-02T05:38:45Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

-     */
-    public static DistributionSummary summaryWithDefaultQuantiles(MeterRegistry registry,
-                                                                  String name, Iterable<Tag> tags) {
+     * Returns a newly-registered {@link DistributionSummary}.


Append: with the current {@linkplain #distributionStatisticConfig() distribution statistic configuration}.

trustin · 2018-05-02T05:39:24Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

-                                  .publishPercentiles(PERCENTILES)
-                                  .register(registry);
+
+        if (distributionStatisticConfig != null) {


No need to do null check if changed as I requested.

trustin · 2018-05-02T05:39:46Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+            return DistributionSummary.builder(name)
+                                      .tags(tags)
+                                      .publishPercentiles(PERCENTILES)
+                                      .register(registry);


This will go away if changed as I requested.

trustin · 2018-05-02T05:43:15Z

Thanks, @huydx!
/cc @jwills

trustin · 2018-05-02T05:44:17Z

You may want to keep the original methods and their old behavior to keep the backward compatibility, but I'm not strong on this.

hyangtack · 2018-05-02T08:33:33Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+                                       .build();
+
+    /**
+     * Set {@link DistributionStatisticConfig} to {@link MoreMeters}.


Set -> Sets

hyangtack · 2018-05-02T08:34:41Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+    }
+
+    /**
+     * Return the current {@link DistributionStatisticConfig} of {@link MoreMeters}.


Return -> Returns

hyangtack · 2018-05-02T08:43:43Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+    }
+
+    /**
+     * Returns a newly-registered {@link DistributionSummary} use


use -> configured by?

hyangtack · 2018-05-02T08:46:42Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+        requireNonNull(name, "name");
+        requireNonNull(tags, "tags");
+
+        Duration maxExpectedValue =


nit: final

hyangtack · 2018-05-02T08:46:50Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+        Duration maxExpectedValue =
+                Optional.ofNullable(distributionStatisticConfig.getMaximumExpectedValue())
+                        .map(Duration::ofNanos).orElse(null);
+        Duration minExpectedValue =


nit: final

hyangtack · 2018-05-02T08:52:08Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

@@ -52,9 +106,37 @@ public static DistributionSummary summaryWithDefaultQuantiles(MeterRegistry regi
                                  .register(registry);
    }

+    /**
+     * Returns a newly-registered {@link Timer} use {@link MoreMeters#distributionStatisticConfig}.


trustin

Minor stuff. Thanks!

trustin · 2018-05-02T11:12:52Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+     * {@link MoreMeters#newDistributionSummary(MeterRegistry, String, Iterable)} will use this config.
+     */
+    public static void setDistributionStatisticConfig(DistributionStatisticConfig config) {
+        requireNonNull(config, "distributionStatisticConfig");


"distributionStatisticConfig" -> "config"

trustin · 2018-05-02T11:14:11Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+                                  .tags(tags)
+                                  .publishPercentiles(distributionStatisticConfig.getPercentiles())
+                                  .publishPercentileHistogram(
+                                          distributionStatisticConfig.isPercentileHistogram())


All setter calls could be joined into a single line so they look prettier:

.publishPercentileHistogram(distributionStatisticConfig.isPercentileHistogram()) .maximumExpectedValue(distributionStatisticConfig.getMaximumExpectedValue()) ...

this line is over checkStyle quota when joins

That's why I suggested renaming the field to something like distStatCfg. Seriously, it's way too long.

Would you change the name plz?

trustin · 2018-05-02T11:15:02Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

    /**
     * Returns a newly-registered {@link DistributionSummary} with percentile publication configured.
+     * @deprecated use {@link MoreMeters#newDistributionSummary(MeterRegistry, String, Iterable)} instead.


MoreMeters in {@link } is redundant because we are referring to the member in the current class. i.e. {@link #newDistributionSummary(...)}

use -> Use

s/ instead// (instead is redundant)

trustin · 2018-05-02T11:15:48Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

@@ -52,9 +106,37 @@ public static DistributionSummary summaryWithDefaultQuantiles(MeterRegistry regi
                                  .register(registry);
    }

+    /**
+     * Returns a newly-registered {@link Timer} configured by {@link MoreMeters#distributionStatisticConfig}.


{@link #distributionStatisticConfig()}

We should not refer to a field but to a method.

MoreMeters is redundant.

trustin · 2018-05-02T11:17:33Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

    /**
     * Returns a newly-registered {@link Timer} with percentile publication configured.
+     * @deprecated use {@link MoreMeters#newTimer(MeterRegistry, String, Iterable)} instead.


s/MoreMeters//

use -> Use

s/ instead//

trustin · 2018-05-02T11:22:07Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+                                       .build();
+
+    /**
+     * Sets {@link DistributionStatisticConfig} to {@link MoreMeters}.


How about:

Sets the {@link DistributionStatisticConfig} to use when the factory methods in {@link MoreMeter} create a {@link Timer} or a {@link DistributionSummary}.

...?

Then the remaining sentences ('Any... will use this config.) would be OK to be removed.

trustin · 2018-05-02T11:22:49Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+    }
+
+    /**
+     * Returns the current {@link DistributionStatisticConfig} of {@link MoreMeters}.


How about adding @see #setDistributionStatisticConfig(..)?

Returns the {@link DistributionStatisticConfig} to use when the factory methods in {@link MoreMeter} create a {@link Timer} or a {@link DistributionSummary}.?

trustin

Just two more nits and we're done I guess. :-)

trustin · 2018-05-02T13:55:23Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

+    /**
+     * Returns a newly-registered {@link DistributionSummary} configured by
+     * {@link #distributionStatisticConfig()}.
+     **/


**/ -> */

trustin · 2018-05-02T13:56:11Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

    /**
     * Returns a newly-registered {@link Timer} with percentile publication configured.
+     * @deprecated Use {@link MoreMeters#newTimer(MeterRegistry, String, Iterable)}.


MoreMeters -> <nil>

huydx · 2018-05-07T07:33:24Z

PTAL~ @trustin

trustin · 2018-05-09T02:09:30Z

@minwoox Any comments?

… config

minwoox

Thank you~!

trustin · 2018-05-10T01:47:06Z

Thanks, @huydx and reviewers!

Motivation: It is currently not possible to customize the settings of `DistributionSummary`s and `Timer`s, such as `expiry` and `bucketLength`. See `MoreMeters.summary/timerWithDefaultQuantiles()`. Modifications: - Add `MoreMeters.distributionStatisticConfig` property so that a user can change the default settings - Deprecate `MoreMeters.summaryWithDefaultQuantiles` and `timerWithDefaultQuantiles` - Add `MoreMeters.newDistributionSummary()` and `newTimer()` Result: - A user can customize the `Timer`s and `DistributionSummary`s created by Armeria. - Fixes line#1170

huydx requested review from hyangtack, minwoox and trustin as code owners May 2, 2018 04:58

trustin added this to the 0.64.0 milestone May 2, 2018

trustin added the new feature label May 2, 2018

trustin requested changes May 2, 2018

View reviewed changes

trustin added the breaking change label May 2, 2018

huydx force-pushed the huydx-metrics-2 branch 6 times, most recently from e194f99 to 9f706a7 Compare May 2, 2018 07:21

hyangtack reviewed May 2, 2018

View reviewed changes

huydx force-pushed the huydx-metrics-2 branch from 9f706a7 to 777d765 Compare May 2, 2018 10:19

trustin requested changes May 2, 2018

View reviewed changes

trustin added deprecation and removed breaking change labels May 2, 2018

huydx force-pushed the huydx-metrics-2 branch 2 times, most recently from 2ff8dcc to 69bece0 Compare May 2, 2018 12:30

trustin requested changes May 2, 2018

View reviewed changes

huydx force-pushed the huydx-metrics-2 branch from 69bece0 to 327bef8 Compare May 6, 2018 11:56

trustin approved these changes May 8, 2018

View reviewed changes

hyangtack approved these changes May 8, 2018

View reviewed changes

Add ability to config Timer and DistributionSummary with user defined…

44f6dd6

… config

huydx force-pushed the huydx-metrics-2 branch from 327bef8 to 44f6dd6 Compare May 10, 2018 00:30

minwoox approved these changes May 10, 2018

View reviewed changes

trustin merged commit d070e06 into line:master May 10, 2018

huydx deleted the huydx-metrics-2 branch June 17, 2018 14:38

Add ability to config Timer and DistributionSummary with user defined… #1176

Add ability to config Timer and DistributionSummary with user defined… #1176

Conversation

huydx commented May 2, 2018

codecov bot commented May 2, 2018 • edited

Codecov Report

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

trustin commented May 2, 2018

trustin commented May 2, 2018 • edited

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

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huydx commented May 7, 2018

trustin commented May 9, 2018

minwoox left a comment

Choose a reason for hiding this comment

trustin commented May 10, 2018

codecov bot commented May 2, 2018 •

edited

trustin commented May 2, 2018 •

edited

trustin May 2, 2018 •

edited

trustin May 2, 2018 •

edited

trustin May 2, 2018 •

edited

trustin May 2, 2018 •

edited