From 77ab6091c492211d3ed18d6c78409da69aa6a750 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Fri, 4 Oct 2019 09:40:04 -0500 Subject: [PATCH] Brings our 1.1 and 2.0 impls in line with clarification of metric reuse (#1080) MP metrics reusability spec clarification: should apply only to annotations --- .../java/io/helidon/metrics/Registry.java | 9 --- .../java/io/helidon/metrics/Registry.java | 13 +--- .../java/io/helidon/metrics/RegistryTest.java | 11 ---- .../microprofile/metrics/MetricProducer.java | 51 ++++++++++++--- .../microprofile/metrics/MetricProducer.java | 62 +++++++++++++++---- .../metrics/ReusabilityMpServiceTest.java | 21 ------- 6 files changed, 94 insertions(+), 73 deletions(-) diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/Registry.java b/metrics/metrics/src/main/java/io/helidon/metrics/Registry.java index 4f2f1ed99..e26c8a245 100644 --- a/metrics/metrics/src/main/java/io/helidon/metrics/Registry.java +++ b/metrics/metrics/src/main/java/io/helidon/metrics/Registry.java @@ -452,14 +452,6 @@ class Registry extends MetricRegistry implements io.helidon.common.metrics.Inter .map(metric -> toType(metric, clazz)); } - private static boolean enforceReusability(T metric, Metadata metadata) { - if (!(metric.isReusable() && metadata.isReusable())) { - throw new IllegalArgumentException("Attempting to re-register metric " - + metric.getName() + " that is already registered with different reusability"); - } - return true; - } - /** * Returns an existing metric with the requested name, or if none is already * registered registers a new metric using the name and type. @@ -498,7 +490,6 @@ class Registry extends MetricRegistry implements io.helidon.common.metrics.Inter Class clazz) { return getOptionalMetric(metadata.getName(), clazz) .filter(metric -> enforceConsistentMetadata(metric, metadata)) - .filter(metric -> enforceReusability(metric, metadata)) .orElseGet(() -> { return registerMetric(metadata.getName(), metadata, metricFactory); }); diff --git a/metrics2/metrics2/src/main/java/io/helidon/metrics/Registry.java b/metrics2/metrics2/src/main/java/io/helidon/metrics/Registry.java index f8be7cf07..ecc16b8cd 100644 --- a/metrics2/metrics2/src/main/java/io/helidon/metrics/Registry.java +++ b/metrics2/metrics2/src/main/java/io/helidon/metrics/Registry.java @@ -579,8 +579,7 @@ public class Registry extends MetricRegistry implements io.helidon.common.metric /** * Returns an existing metric (if one is already registered with the name * from the metadata plus the tags, and if the existing metadata is - * consistent with the new metadata, and if reuse is permitted) or a new - * metric, registered using the metadata and tags. + * consistent with the new metadata) or a new metric, registered using the metadata and tags. * * @param type of the metric * @param newMetadata metadata describing the metric @@ -605,7 +604,6 @@ public class Registry extends MetricRegistry implements io.helidon.common.metric */ return getOptionalMetric(metricName, clazz, tags) .filter(existingMetric -> enforceConsistentMetadata(existingMetric, newMetadata, tags)) - .filter(existingMetric -> enforceReusability(existingMetric, newMetadata, tags)) .orElseGet(() -> { final Metadata metadata = getOrRegisterMetadata(metricName, newMetadata, tags); return registerMetric(metricName, @@ -706,15 +704,6 @@ public class Registry extends MetricRegistry implements io.helidon.common.metric return true; } - private static boolean enforceReusability(T metric, Metadata metadata, Tag... tags) { - // We are here because a metric already exists during an attempt to register. - if (!metadata.isReusable()) { - throw new IllegalArgumentException("Attempting to re-register metric " - + metric.getName() + " that is already registered as non-reusable"); - } - return true; - } - private U toType(T m1, Class clazz) { MetricType type1 = toType(m1); MetricType type2 = toType(clazz); diff --git a/metrics2/metrics2/src/test/java/io/helidon/metrics/RegistryTest.java b/metrics2/metrics2/src/test/java/io/helidon/metrics/RegistryTest.java index d3f43193d..89db9d64e 100644 --- a/metrics2/metrics2/src/test/java/io/helidon/metrics/RegistryTest.java +++ b/metrics2/metrics2/src/test/java/io/helidon/metrics/RegistryTest.java @@ -108,15 +108,4 @@ public class RegistryTest { () -> registry.counter(metadata2, tag1)); assertThat(ex.getMessage(), containsString("conflicts with")); } - - @Test - void testInvalidReregistration() { - Metadata metadata1 = new HelidonMetadata("counter6", "display name", - "description", MetricType.COUNTER, MetricUnits.NONE, false); - registry.counter(metadata1, tag1); - IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, - () -> registry.counter(metadata1,tag1)); - assertThat(ex.getMessage(), containsString("re-register")); - assertThat(ex.getMessage(), containsString("already registered as non-reusable")); - } } \ No newline at end of file diff --git a/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java b/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java index 2c5930de3..50ef82ed5 100644 --- a/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java +++ b/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java @@ -16,6 +16,7 @@ package io.helidon.microprofile.metrics; +import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; import java.util.Map; import java.util.function.Function; @@ -34,7 +35,10 @@ import org.eclipse.microprofile.metrics.MetricRegistry; import org.eclipse.microprofile.metrics.MetricType; import org.eclipse.microprofile.metrics.MetricUnits; import org.eclipse.microprofile.metrics.Timer; +import org.eclipse.microprofile.metrics.annotation.Counted; +import org.eclipse.microprofile.metrics.annotation.Metered; import org.eclipse.microprofile.metrics.annotation.Metric; +import org.eclipse.microprofile.metrics.annotation.Timed; /** * Class MetricProducer. @@ -88,7 +92,8 @@ class MetricProducer { @Produces @VendorDefined private Counter produceCounter(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getCounters, registry::counter, Counter.class); + return produceMetric(registry, ip, Counted.class, registry::getCounters, registry::counter, + Counter.class); } @Produces @@ -99,7 +104,8 @@ class MetricProducer { @Produces @VendorDefined private Meter produceMeter(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getMeters, registry::meter, Meter.class); + return produceMetric(registry, ip, Metered.class, registry::getMeters, registry::meter, + Meter.class); } @Produces @@ -110,7 +116,8 @@ class MetricProducer { @Produces @VendorDefined private Timer produceTimer(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getTimers, registry::timer, Timer.class); + return produceMetric(registry, ip, Timed.class, registry::getTimers, registry::timer, + Timer.class); } @Produces @@ -121,7 +128,8 @@ class MetricProducer { @Produces @VendorDefined private Histogram produceHistogram(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getHistograms, registry::histogram, Histogram.class); + return produceMetric(registry, ip, null, registry::getHistograms, registry::histogram, + Histogram.class); } /** @@ -157,18 +165,43 @@ class MetricProducer { return produceGauge(registry, ip); } - private T produceMetric(InjectionPoint ip, Supplier> getTypedMetricsFn, + private T produceMetric(MetricRegistry registry, + InjectionPoint ip, Class annotationClass, Supplier> getTypedMetricsFn, Function registerFn, Class clazz) { Metric metricAnno = ip.getAnnotated().getAnnotation(Metric.class); - T result = getTypedMetricsFn.get().get(getName(metricAnno, ip)); + final String metricName = getName(metricAnno, ip); + T result = getTypedMetricsFn.get().get(metricName); + final Metadata newMetadata = newMetadata(ip, metricAnno, MetricType.from(clazz)); /* - * For an injection point, refer to a previously-registered metric that matches the name, - * if any. Register a new instance only if none is already present. + * If the injection point does not include the corresponding metric annotation which would + * declare the metric, then we do not need to enforce reuse restrictions because an @Inject + * or a @Metric by itself on an injection point is lookup-or-register. */ + if (result != null) { + final Annotation specificMetricAnno = annotationClass == null ? null + : ip.getAnnotated().getAnnotation(annotationClass); + if (specificMetricAnno == null) { + return result; + } + final Metadata existingMetadata = registry.getMetadata().get(metricName); + enforceReusability(metricName, existingMetadata, newMetadata); + } if (result == null) { - result = registerFn.apply(newMetadata(ip, metricAnno, MetricType.from(clazz))); + result = registerFn.apply(newMetadata); } return result; } + + private static void enforceReusability(String metricName, Metadata existingMetadata, + Metadata newMetadata) { + if (existingMetadata.isReusable() != newMetadata.isReusable()) { + throw new IllegalArgumentException("Attempt to reuse metric " + metricName + + " with inconsistent isReusable setting"); + } + if (!newMetadata.isReusable()) { + throw new IllegalArgumentException("Attempting to reuse metric " + + metricName + " that is not reusable"); + } + } } diff --git a/microprofile/metrics2/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java b/microprofile/metrics2/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java index 7ca274bab..bae357d5c 100644 --- a/microprofile/metrics2/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java +++ b/microprofile/metrics2/src/main/java/io/helidon/microprofile/metrics/MetricProducer.java @@ -16,6 +16,7 @@ package io.helidon.microprofile.metrics; +import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; import java.util.ArrayList; import java.util.List; @@ -41,7 +42,10 @@ import org.eclipse.microprofile.metrics.MetricType; import org.eclipse.microprofile.metrics.MetricUnits; import org.eclipse.microprofile.metrics.Tag; import org.eclipse.microprofile.metrics.Timer; +import org.eclipse.microprofile.metrics.annotation.Counted; +import org.eclipse.microprofile.metrics.annotation.Metered; import org.eclipse.microprofile.metrics.annotation.Metric; +import org.eclipse.microprofile.metrics.annotation.Timed; /** * Class MetricProducer. @@ -123,7 +127,8 @@ class MetricProducer { @Produces @VendorDefined private Counter produceCounter(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getCounters, registry::counter, Counter.class); + return produceMetric(registry, ip, Counted.class, registry::getCounters, + registry::counter, Counter.class); } @Produces @@ -134,7 +139,8 @@ class MetricProducer { @Produces @VendorDefined private Meter produceMeter(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getMeters, registry::meter, Meter.class); + return produceMetric(registry, ip, Metered.class, registry::getMeters, + registry::meter, Meter.class); } @Produces @@ -145,7 +151,7 @@ class MetricProducer { @Produces @VendorDefined private Timer produceTimer(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getTimers, registry::timer, Timer.class); + return produceMetric(registry, ip, Timed.class, registry::getTimers, registry::timer, Timer.class); } @Produces @@ -156,7 +162,8 @@ class MetricProducer { @Produces @VendorDefined private Histogram produceHistogram(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getHistograms, registry::histogram, Histogram.class); + return produceMetric(registry, ip, null, registry::getHistograms, + registry::histogram, Histogram.class); } @Produces @@ -167,7 +174,8 @@ class MetricProducer { @Produces @VendorDefined private ConcurrentGauge produceConcurrentGauge(MetricRegistry registry, InjectionPoint ip) { - return produceMetric(ip, registry::getConcurrentGauges, registry::concurrentGauge, ConcurrentGauge.class); + return produceMetric(registry, ip, org.eclipse.microprofile.metrics.annotation.ConcurrentGauge.class, + registry::getConcurrentGauges, registry::concurrentGauge, ConcurrentGauge.class); } /** @@ -204,28 +212,60 @@ class MetricProducer { } /** - * Returns an existing metric if one exists matching the injection point - * criteria, or if there is none registers and returns a new one using the - * caller-provided code. + * Returns an existing metric if one exists that matches the injection point + * criteria and is also reusable, or if there is none registers and returns a new one + * using the caller-provided function. If the caller refers to an existing metric that is + * not reusable then the method throws an {@code IllegalArgumentException}. * * @param the type of the metric + * @param the type of the annotation which marks a registration of the metric type + * @param registry metric registry to use * @param ip the injection point + * @param annotationClass annotation which represents a declaration of a metric * @param getTypedMetricsFn caller-provided factory for creating the correct * type of metric (if there is no pre-existing one) * @param registerFn caller-provided function for registering a newly-created metric * @param clazz class for the metric type of interest * @return the existing metric (if any), or the newly-created and registered one */ - private T produceMetric(InjectionPoint ip, Supplier> getTypedMetricsFn, + private T produceMetric(MetricRegistry registry, + InjectionPoint ip, Class annotationClass, Supplier> getTypedMetricsFn, BiFunction registerFn, Class clazz) { final Metric metricAnno = ip.getAnnotated().getAnnotation(Metric.class); final Tag[] tags = tags(metricAnno); final MetricID metricID = new MetricID(getName(metricAnno, ip), tags); + T result = getTypedMetricsFn.get().get(metricID); - if (result == null) { - result = registerFn.apply(newMetadata(ip, metricAnno, MetricType.from(clazz)), tags); + final Metadata newMetadata = newMetadata(ip, metricAnno, MetricType.from(clazz)); + /* + * If the injection point does not include the corresponding metric annotation which would + * declare the metric, then we do not need to enforce reuse restrictions because an @Inject + * or a @Metric by itself on an injection point is lookup-or-register. + */ + if (result != null) { + final Annotation specificMetricAnno = annotationClass == null ? null + : ip.getAnnotated().getAnnotation(annotationClass); + if (specificMetricAnno == null) { + return result; + } + final Metadata existingMetadata = registry.getMetadata().get(metricID.getName()); + enforceReusability(metricID, existingMetadata, newMetadata); + } else { + result = registerFn.apply(newMetadata, tags); } return result; } + + private static void enforceReusability(MetricID metricID, Metadata existingMetadata, + Metadata newMetadata, Tag... tags) { + if (existingMetadata.isReusable() != newMetadata.isReusable()) { + throw new IllegalArgumentException("Attempt to reuse metric " + metricID + + " with inconsistent isReusable setting"); + } + if (!newMetadata.isReusable()) { + throw new IllegalArgumentException("Attempting to reuse metric " + + metricID + " that is not reusable"); + } + } } diff --git a/microprofile/metrics2/src/test/java/io/helidon/microprofile/metrics/ReusabilityMpServiceTest.java b/microprofile/metrics2/src/test/java/io/helidon/microprofile/metrics/ReusabilityMpServiceTest.java index 248defa51..505016b76 100644 --- a/microprofile/metrics2/src/test/java/io/helidon/microprofile/metrics/ReusabilityMpServiceTest.java +++ b/microprofile/metrics2/src/test/java/io/helidon/microprofile/metrics/ReusabilityMpServiceTest.java @@ -43,16 +43,6 @@ public class ReusabilityMpServiceTest { .build(); } - @Test - public void tryToStartServerWithIllegalAnnotationReuse() throws Exception { - - final DefinitionException ex = assertThrows(DefinitionException.class, () -> { - initServer(ResourceWithIllegallyReusedMetrics.class);; - }); - assertThat(ex.getCause(), is(instanceOf(IllegalArgumentException.class))); - assertThat(ex.getCause().getMessage(), containsString("already registered")); - } - @Test public void tryToStartServerWithLegalAnnotationReuse() throws Exception { Server server = initServer(ResourceWithLegallyReusedMetrics.class); @@ -62,15 +52,4 @@ public class ReusabilityMpServiceTest { server.stop(); } } - - @Test - public void tryToStartServerWithMixedReuseAnnotations() throws Exception { - DefinitionException ex = assertThrows(DefinitionException.class, () -> { - initServer(ResourceWithMixedReusability.class);; - }); - - assertThat(ex.getCause(), is(instanceOf(IllegalArgumentException.class))); - assertThat(ex.getCause().getMessage(), containsString("already registered")); - } - }