From 4e479cfccd6bdf5a302fdf93b004d92b8cdb26a2 Mon Sep 17 00:00:00 2001 From: varsha Date: Tue, 15 Jun 2021 13:34:31 +0530 Subject: [PATCH 1/4] ENG-9598 Adding distribution summary support --- .../metrics/PlatformMetricsRegistry.java | 35 +++++++++++++++++++ .../metrics/PlatformMetricsRegistryTest.java | 35 +++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java b/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java index 0ecfede..43ee357 100644 --- a/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java +++ b/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java @@ -13,6 +13,7 @@ import io.github.mweirauch.micrometer.jvm.extras.ProcessThreadMetrics; import io.micrometer.core.instrument.Clock; import io.micrometer.core.instrument.Counter; +import io.micrometer.core.instrument.DistributionSummary; import io.micrometer.core.instrument.Gauge; import io.micrometer.core.instrument.ImmutableTag; import io.micrometer.core.instrument.MeterRegistry; @@ -339,6 +340,40 @@ public static T registerGauge(String name, Map + * See https://micrometer.io/docs/concepts#_distribution_summaries for more details. + */ + public static DistributionSummary registerDistributionSummary(String name, + Map tags) { + return registerDistributionSummary(name, tags, false); + } + + /** + * Registers a DistributionSummary for the given name with the service's metric registry and + * reports it periodically to the configured reporters Apart from the provided tags, the reporting + * service's default tags also will be reported with the metrics. + *

+ * Param histogram – Determines whether percentile histograms should be published. + *

+ * For more details refer - https://micrometer.io/docs/concepts#_distribution_summaries for more + * details, https://micrometer.io/docs/concepts#_histograms_and_percentiles + */ + public static DistributionSummary registerDistributionSummary(String name, + Map tags, boolean histogram) { + DistributionSummary.Builder builder = DistributionSummary.builder(name) + .publishPercentiles(0.5, 0.95, 0.99) + .tags(addDefaultTags(tags)); + if (histogram) { + builder = builder.publishPercentileHistogram(); + } + return builder.register(METER_REGISTRY); + } + /** * Registers metrics for the given executor service with the service's metric registry and * reports them periodically to the configured reporters. Apart from the given tags, the diff --git a/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java b/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java index ec28530..a45ed22 100644 --- a/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java +++ b/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java @@ -1,16 +1,21 @@ package org.hypertrace.core.serviceframework.metrics; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.typesafe.config.Config; import com.typesafe.config.ConfigFactory; import io.micrometer.core.instrument.Counter; +import io.micrometer.core.instrument.DistributionSummary; import io.micrometer.core.instrument.Timer; import io.micrometer.core.instrument.composite.CompositeMeterRegistry; +import java.util.Arrays; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -117,6 +122,36 @@ public void testGauge() { assertEquals(11, gauge.get()); } + @Test + public void testDistributionSummary() { + initializeCustomRegistry(List.of("testing")); + + DistributionSummary distribution = PlatformMetricsRegistry + .registerDistributionSummary("my.distribution", Map.of("foo", "bar")); + distribution.record(100); + assertEquals(1, distribution.count()); + assertEquals(100, distribution.totalAmount()); + + // Try to register the same timer again and we should get the same instance. + distribution = PlatformMetricsRegistry + .registerDistributionSummary("my.distribution", Map.of("foo", "bar")); + distribution.record(50); + assertEquals(2, distribution.count()); + assertEquals(150, distribution.totalAmount()); + assertEquals(75, distribution.mean()); + assertTrue( + Arrays.stream(distribution.takeSnapshot().percentileValues()).map(m -> m.percentile()) + .collect( + Collectors.toList()).containsAll(List.of(0.5, 0.95, 0.99))); + + // Create a new distribution + distribution = PlatformMetricsRegistry + .registerDistributionSummary("my.distribution", new HashMap<>()); + distribution.record(100); + assertEquals(1, distribution.count()); + assertEquals(100, distribution.totalAmount()); + } + @Test public void test_initializePrometheusPushGateway_withNullUrlAddress_throwsException() { Assertions.assertThrows(IllegalArgumentException.class, From 329d7848c80e632e90ef82adc8ae20514eb226aa Mon Sep 17 00:00:00 2001 From: varsha Date: Tue, 15 Jun 2021 13:45:55 +0530 Subject: [PATCH 2/4] Typo in docs --- .../serviceframework/metrics/PlatformMetricsRegistry.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java b/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java index 43ee357..83ad7dd 100644 --- a/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java +++ b/platform-metrics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java @@ -360,8 +360,8 @@ public static DistributionSummary registerDistributionSummary(String name, *

* Param histogram – Determines whether percentile histograms should be published. *

- * For more details refer - https://micrometer.io/docs/concepts#_distribution_summaries for more - * details, https://micrometer.io/docs/concepts#_histograms_and_percentiles + * For more details - https://micrometer.io/docs/concepts#_distribution_summaries, + * https://micrometer.io/docs/concepts#_histograms_and_percentiles */ public static DistributionSummary registerDistributionSummary(String name, Map tags, boolean histogram) { From 238f7e4d30da90382590e1a9be348576f48cdcd7 Mon Sep 17 00:00:00 2001 From: varsha Date: Tue, 15 Jun 2021 13:50:20 +0530 Subject: [PATCH 3/4] Increasing test coverage --- .../metrics/PlatformMetricsRegistryTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java b/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java index a45ed22..759cf69 100644 --- a/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java +++ b/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java @@ -144,12 +144,16 @@ public void testDistributionSummary() { .collect( Collectors.toList()).containsAll(List.of(0.5, 0.95, 0.99))); - // Create a new distribution + // Create a new distribution with histogram enabled distribution = PlatformMetricsRegistry - .registerDistributionSummary("my.distribution", new HashMap<>()); + .registerDistributionSummary("my.distribution", new HashMap<>(), true); distribution.record(100); assertEquals(1, distribution.count()); assertEquals(100, distribution.totalAmount()); + assertTrue( + Arrays.stream(distribution.takeSnapshot().percentileValues()).map(m -> m.percentile()) + .collect( + Collectors.toList()).containsAll(List.of(0.5, 0.95, 0.99))); } @Test From c26956ea59333bef032a137b9931151766a17eb2 Mon Sep 17 00:00:00 2001 From: Varsha Abhinandan Date: Tue, 15 Jun 2021 13:55:09 +0530 Subject: [PATCH 4/4] Review comments Co-authored-by: kotharironak <53209990+kotharironak@users.noreply.github.com> --- .../serviceframework/metrics/PlatformMetricsRegistryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java b/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java index 759cf69..046bf6e 100644 --- a/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java +++ b/platform-metrics/src/test/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistryTest.java @@ -132,7 +132,7 @@ public void testDistributionSummary() { assertEquals(1, distribution.count()); assertEquals(100, distribution.totalAmount()); - // Try to register the same timer again and we should get the same instance. + // Try to register the same summary again and we should get the same instance. distribution = PlatformMetricsRegistry .registerDistributionSummary("my.distribution", Map.of("foo", "bar")); distribution.record(50);