Polish MeterTag changes (#3769)

* Polish MeterTag changes See gh-3727 * Update micrometer-commons/src/main/java/io/micrometer/common/annotation/AnnotationUtils.java Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com> --------- Co-authored-by: Marcin Grzejszczak <[email protected]> Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>
micrometer-metrics · Apr 18, 2023 · 9b61c23 · 9b61c23
1 parent 23804d5
commit 9b61c23
Show file tree

Hide file tree

Showing 13 changed files with 97 additions and 134 deletions.
diff --git a/micrometer-commons/src/main/java/io/micrometer/common/annotation/AnnotationHandler.java b/micrometer-commons/src/main/java/io/micrometer/common/annotation/AnnotationHandler.java
@@ -103,45 +103,43 @@ public void addAnnotatedParameters(T objectToModify, ProceedingJoinPoint pjp) {
     private void getAnnotationsFromInterfaces(ProceedingJoinPoint pjp, Method mostSpecificMethod,
             List<AnnotatedParameter> annotatedParameters) {
         Class<?>[] implementedInterfaces = pjp.getThis().getClass().getInterfaces();
-        if (implementedInterfaces.length > 0) {
-            for (Class<?> implementedInterface : implementedInterfaces) {
-                for (Method methodFromInterface : implementedInterface.getMethods()) {
-                    if (methodsAreTheSame(mostSpecificMethod, methodFromInterface)) {
-                        List<AnnotatedParameter> annotatedParametersForActualMethod = AnnotationUtils
-                            .findAnnotatedParameters(annotationClass, methodFromInterface, pjp.getArgs());
-                        mergeAnnotatedParameters(annotatedParameters, annotatedParametersForActualMethod);
-                    }
+        for (Class<?> implementedInterface : implementedInterfaces) {
+            for (Method methodFromInterface : implementedInterface.getMethods()) {
+                if (methodsAreTheSame(mostSpecificMethod, methodFromInterface)) {
+                    List<AnnotatedParameter> annotatedParametersForActualMethod = AnnotationUtils
+                        .findAnnotatedParameters(annotationClass, methodFromInterface, pjp.getArgs());
+                    mergeAnnotatedParameters(annotatedParameters, annotatedParametersForActualMethod);
                 }
             }
         }
     }
 
-    private boolean methodsAreTheSame(Method mostSpecificMethod, Method method1) {
-        return method1.getName().equals(mostSpecificMethod.getName())
-                && Arrays.equals(method1.getParameterTypes(), mostSpecificMethod.getParameterTypes());
+    private boolean methodsAreTheSame(Method mostSpecificMethod, Method method) {
+        return method.getName().equals(mostSpecificMethod.getName())
+                && Arrays.equals(method.getParameterTypes(), mostSpecificMethod.getParameterTypes());
     }
 
-    private void mergeAnnotatedParameters(List<AnnotatedParameter> annotatedParametersIndices,
-            List<AnnotatedParameter> annotatedParametersIndicesForActualMethod) {
-        for (AnnotatedParameter container : annotatedParametersIndicesForActualMethod) {
+    private void mergeAnnotatedParameters(List<AnnotatedParameter> annotatedParameters,
+            List<AnnotatedParameter> annotatedParametersForActualMethod) {
+        for (AnnotatedParameter container : annotatedParametersForActualMethod) {
             final int index = container.parameterIndex;
             boolean parameterContained = false;
-            for (AnnotatedParameter parameterContainer : annotatedParametersIndices) {
+            for (AnnotatedParameter parameterContainer : annotatedParameters) {
                 if (parameterContainer.parameterIndex == index) {
                     parameterContained = true;
                     break;
                 }
             }
             if (!parameterContained) {
-                annotatedParametersIndices.add(container);
+                annotatedParameters.add(container);
             }
         }
     }
 
-    private void addAnnotatedArguments(T objectToTag, List<AnnotatedParameter> toBeAdded) {
+    private void addAnnotatedArguments(T objectToModify, List<AnnotatedParameter> toBeAdded) {
         for (AnnotatedParameter container : toBeAdded) {
             KeyValue keyValue = toKeyValue.apply(container.annotation, container.argument);
-            keyValueConsumer.accept(keyValue, objectToTag);
+            keyValueConsumer.accept(keyValue, objectToModify);
         }
     }
 

diff --git a/micrometer-commons/src/main/java/io/micrometer/common/annotation/AnnotationUtils.java b/micrometer-commons/src/main/java/io/micrometer/common/annotation/AnnotationUtils.java
@@ -34,14 +34,14 @@ private AnnotationUtils() {
 
     }
 
-    static List<AnnotatedParameter> findAnnotatedParameters(Class<? extends Annotation> tagClazz, Method method,
+    static List<AnnotatedParameter> findAnnotatedParameters(Class<? extends Annotation> annotationClazz, Method method,
             Object[] args) {
         Annotation[][] parameters = method.getParameterAnnotations();
         List<AnnotatedParameter> result = new ArrayList<>();
         int i = 0;
         for (Annotation[] parameter : parameters) {
             for (Annotation parameter2 : parameter) {
-                if (tagClazz.isAssignableFrom(parameter2.getClass())) {
+                if (annotationClazz.isAssignableFrom(parameter2.annotationType())) {
                     result.add(new AnnotatedParameter(i, parameter2, args[i]));
                 }
             }

diff --git a/micrometer-commons/src/main/java/io/micrometer/common/annotation/NoOpValueResolver.java b/micrometer-commons/src/main/java/io/micrometer/common/annotation/NoOpValueResolver.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2022 the original author or authors.
+ * Copyright 2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

diff --git a/...ometer-commons/src/main/java/io/micrometer/common/annotation/ValueExpressionResolver.java b/...ometer-commons/src/main/java/io/micrometer/common/annotation/ValueExpressionResolver.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2022 the original author or authors.
+ * Copyright 2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

diff --git a/micrometer-commons/src/main/java/io/micrometer/common/annotation/ValueResolver.java b/micrometer-commons/src/main/java/io/micrometer/common/annotation/ValueResolver.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2022 the original author or authors.
+ * Copyright 2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

diff --git a/micrometer-commons/src/main/java/io/micrometer/common/annotation/package-info.java b/micrometer-commons/src/main/java/io/micrometer/common/annotation/package-info.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 VMware, Inc.
+ * Copyright 2023 VMware, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

diff --git a/micrometer-core/src/main/java/io/micrometer/core/aop/MeterTag.java b/micrometer-core/src/main/java/io/micrometer/core/aop/MeterTag.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2022 the original author or authors.
+ * Copyright 2023 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,8 +28,8 @@
  * expression just return a {@code toString()} value of the parameter.
  *
  * IMPORTANT: Provided tag values MUST BE of LOW-CARDINALITY. If you fail to provide
- * low-cardinality values, that can lead to performance issues of your metrics back end.
- * Values should not come from the end-user since those most likely be high-cardinality.
+ * low-cardinality values, that can lead to performance issues of your metrics backend.
+ * Values should not come from the end-user since those could be high-cardinality.
  *
  * @author Christian Schwerdtfeger
  * @author Marcin Grzejszczak
@@ -42,22 +42,22 @@
 
     /**
      * The name of the key of the tag which should be created. This is an alias for
-     * {@link MeterTag#key()}.
-     * @return the tag key
+     * {@link #key()}.
+     * @return the tag key name
      */
     String value() default "";
 
     /**
      * The name of the key of the tag which should be created.
-     * @return the tag value
+     * @return the tag key name
      */
     String key() default "";
 
     /**
-     * Execute this expression to calculate the tag value. Will be analyzed if no value of
-     * the {@link MeterTag#resolver()} was set. You need to have a
-     * {@link ValueExpressionResolver} registered on the {@link MeterAnnotationHandler} to
-     * provide the expression resolution engine.
+     * Execute this expression to calculate the tag value. Will be evaluated if no value
+     * of the {@link #resolver()} was set. You need to have a
+     * {@link ValueExpressionResolver} registered on the {@link MeterTagAnnotationHandler}
+     * to provide the expression resolution engine.
      * @return an expression
      */
     String expression() default "";

diff --git a/...eter/core/aop/MeterAnnotationHandler.java → ...r/core/aop/MeterTagAnnotationHandler.java b/...eter/core/aop/MeterAnnotationHandler.java → ...r/core/aop/MeterTagAnnotationHandler.java
@@ -28,19 +28,19 @@
 /**
  * Annotation handler for {@link MeterTag}. To add support for {@link MeterTag} on
  * {@link TimedAspect} check the
- * {@link TimedAspect#setMetricsTagAnnotationHandler(MeterAnnotationHandler)} method.
+ * {@link TimedAspect#setMeterTagAnnotationHandler(MeterTagAnnotationHandler)} method.
  *
  * @since 1.11.0
  */
-public class MeterAnnotationHandler extends AnnotationHandler<Timer.Builder> {
+public class MeterTagAnnotationHandler extends AnnotationHandler<Timer.Builder> {
 
     /**
-     * Creates a new instance of {@link MeterAnnotationHandler}.
+     * Creates a new instance of {@link MeterTagAnnotationHandler}.
      * @param resolverProvider function to retrieve a {@link ValueResolver}
      * @param expressionResolverProvider function to retrieve a
      * {@link ValueExpressionResolver}
      */
-    public MeterAnnotationHandler(Function<Class<? extends ValueResolver>, ? extends ValueResolver> resolverProvider,
+    public MeterTagAnnotationHandler(Function<Class<? extends ValueResolver>, ? extends ValueResolver> resolverProvider,
             Function<Class<? extends ValueExpressionResolver>, ? extends ValueExpressionResolver> expressionResolverProvider) {
         super((keyValue, builder) -> builder.tag(keyValue.getKey(), keyValue.getValue()), resolverProvider,
                 expressionResolverProvider, MeterTag.class, (annotation, o) -> {

diff --git a/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java b/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java
@@ -67,8 +67,9 @@
  * }
  * </pre>
  *
- * To add support for {@link MeterTag} annotations set the {@link MeterAnnotationHandler}
- * via {@link TimedAspect#setMetricsTagAnnotationHandler(MeterAnnotationHandler)}.
+ * To add support for {@link MeterTag} annotations set the
+ * {@link MeterTagAnnotationHandler} via
+ * {@link TimedAspect#setMeterTagAnnotationHandler(MeterTagAnnotationHandler)}.
  *
  * @author David J. M. Karlsen
  * @author Jon Schneider
@@ -101,7 +102,7 @@ public class TimedAspect {
 
     private final Predicate<ProceedingJoinPoint> shouldSkip;
 
-    private MeterAnnotationHandler meterTagAnnotationHandler;
+    private MeterTagAnnotationHandler meterTagAnnotationHandler;
 
     /**
      * Creates a {@code TimedAspect} instance with {@link Metrics#globalRegistry}.
@@ -324,9 +325,9 @@ private Optional<LongTaskTimer> buildLongTaskTimer(ProceedingJoinPoint pjp, Time
 
     /**
      * Setting this enables support for {@link MeterTag}.
-     * @param meterTagAnnotationHandler metrics tag annotation handler
+     * @param meterTagAnnotationHandler meter tag annotation handler
      */
-    public void setMetricsTagAnnotationHandler(MeterAnnotationHandler meterTagAnnotationHandler) {
+    public void setMeterTagAnnotationHandler(MeterTagAnnotationHandler meterTagAnnotationHandler) {
         this.meterTagAnnotationHandler = meterTagAnnotationHandler;
     }
 

diff --git a/...core/aop/MeterAnnotationHandlerTests.java → ...e/aop/MeterTagAnnotationHandlerTests.java b/...core/aop/MeterAnnotationHandlerTests.java → ...e/aop/MeterTagAnnotationHandlerTests.java
@@ -18,72 +18,50 @@
 import io.micrometer.common.annotation.ValueExpressionResolver;
 import io.micrometer.common.annotation.ValueResolver;
 import io.micrometer.core.annotation.Timed;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
 
-class MeterAnnotationHandlerTests {
+class MeterTagAnnotationHandlerTests {
 
     ValueResolver valueResolver = parameter -> "Value from myCustomTagValueResolver";
 
     ValueExpressionResolver valueExpressionResolver = new SpelValueExpressionResolver();
 
-    MeterAnnotationHandler handler;
-
-    @BeforeEach
-    void setup() {
-        this.handler = new MeterAnnotationHandler(aClass -> valueResolver, aClass -> valueExpressionResolver);
-    }
-
     @Test
     void shouldUseCustomTagValueResolver() throws NoSuchMethodException, SecurityException {
         Method method = AnnotationMockClass.class.getMethod("getAnnotationForTagValueResolver", String.class);
         Annotation annotation = method.getParameterAnnotations()[0][0];
-        if (annotation instanceof MeterTag) {
-            String resolvedValue = this.handler.resolveTagValue((MeterTag) annotation, "test", aClass -> valueResolver,
-                    aClass -> valueExpressionResolver);
-            assertThat(resolvedValue).isEqualTo("Value from myCustomTagValueResolver");
-        }
-        else {
-            fail("Annotation was not MetricTag");
-        }
+        assertThat(annotation).isInstanceOf(MeterTag.class);
+        String resolvedValue = MeterTagAnnotationHandler.resolveTagValue((MeterTag) annotation, "test",
+                aClass -> valueResolver, aClass -> valueExpressionResolver);
+        assertThat(resolvedValue).isEqualTo("Value from myCustomTagValueResolver");
     }
 
     @Test
     void shouldUseTagValueExpression() throws NoSuchMethodException, SecurityException {
         Method method = AnnotationMockClass.class.getMethod("getAnnotationForTagValueExpression", String.class);
         Annotation annotation = method.getParameterAnnotations()[0][0];
-        if (annotation instanceof MeterTag) {
-            String resolvedValue = this.handler.resolveTagValue((MeterTag) annotation, "test", aClass -> valueResolver,
-                    aClass -> valueExpressionResolver);
-
-            assertThat(resolvedValue).isEqualTo("hello characters");
-        }
-        else {
-            fail("Annotation was not MetricTag");
-        }
+        assertThat(annotation).isInstanceOf(MeterTag.class);
+        String resolvedValue = MeterTagAnnotationHandler.resolveTagValue((MeterTag) annotation, "test value",
+                aClass -> valueResolver, aClass -> valueExpressionResolver);
+        assertThat(resolvedValue).isEqualTo("hello test value characters");
     }
 
     @Test
     void shouldReturnArgumentToString() throws NoSuchMethodException, SecurityException {
         Method method = AnnotationMockClass.class.getMethod("getAnnotationForArgumentToString", Long.class);
         Annotation annotation = method.getParameterAnnotations()[0][0];
-        if (annotation instanceof MeterTag) {
-            String resolvedValue = this.handler.resolveTagValue((MeterTag) annotation, 15, aClass -> valueResolver,
-                    aClass -> valueExpressionResolver);
-            assertThat(resolvedValue).isEqualTo("15");
-        }
-        else {
-            fail("Annotation was not MetricTag");
-        }
+        assertThat(annotation).isInstanceOf(MeterTag.class);
+        String resolvedValue = MeterTagAnnotationHandler.resolveTagValue((MeterTag) annotation, 15,
+                aClass -> valueResolver, aClass -> valueExpressionResolver);
+        assertThat(resolvedValue).isEqualTo("15");
     }
 
-    protected class AnnotationMockClass {
+    static class AnnotationMockClass {
 
         @Timed
         public void getAnnotationForTagValueResolver(
@@ -92,7 +70,7 @@ public void getAnnotationForTagValueResolver(
 
         @Timed
         public void getAnnotationForTagValueExpression(
-                @MeterTag(key = "test", expression = "'hello' + ' characters'") String test) {
+                @MeterTag(key = "test", expression = "'hello ' + #this + ' characters'") String test) {
         }
 
         @Timed

diff --git a/...ometer-core/src/test/java/io/micrometer/core/aop/NullMetricTagAnnotationHandlerTests.java b/...ometer-core/src/test/java/io/micrometer/core/aop/NullMetricTagAnnotationHandlerTests.java
@@ -24,61 +24,44 @@
 import java.lang.reflect.Method;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
 
-class NullMeterAnnotationHandlerTests {
+class NullMeterTagAnnotationHandlerTests {
 
     ValueResolver valueResolver = parameter -> null;
 
-    ValueExpressionResolver valueExpressionResolver = (expression, parameter) -> "";
-
-    MeterAnnotationHandler handler = new MeterAnnotationHandler(aClass -> valueResolver,
-            aClass -> valueExpressionResolver);
+    ValueExpressionResolver valueExpressionResolver = (expression, parameter) -> null;
 
     @Test
-    void shouldUseEmptyStringWheCustomTagValueResolverReturnsNull() throws NoSuchMethodException, SecurityException {
+    void shouldUseEmptyStringWhenCustomTagValueResolverReturnsNull() throws NoSuchMethodException, SecurityException {
         Method method = AnnotationMockClass.class.getMethod("getAnnotationForTagValueResolver", String.class);
         Annotation annotation = method.getParameterAnnotations()[0][0];
-        if (annotation instanceof MeterTag) {
-            String resolvedValue = this.handler.resolveTagValue((MeterTag) annotation, "test", aClass -> valueResolver,
-                    aClass -> valueExpressionResolver);
-            assertThat(resolvedValue).isEqualTo("");
-        }
-        else {
-            fail("Annotation was not MetricTag");
-        }
+        assertThat(annotation).isInstanceOf(MeterTag.class);
+        String resolvedValue = MeterTagAnnotationHandler.resolveTagValue((MeterTag) annotation, "test",
+                aClass -> valueResolver, aClass -> valueExpressionResolver);
+        assertThat(resolvedValue).isEmpty();
     }
 
     @Test
     void shouldUseEmptyStringWhenTagValueExpressionReturnNull() throws NoSuchMethodException, SecurityException {
         Method method = AnnotationMockClass.class.getMethod("getAnnotationForTagValueExpression", String.class);
         Annotation annotation = method.getParameterAnnotations()[0][0];
-        if (annotation instanceof MeterTag) {
-            String resolvedValue = this.handler.resolveTagValue((MeterTag) annotation, "test", aClass -> valueResolver,
-                    aClass -> valueExpressionResolver);
-
-            assertThat(resolvedValue).isEqualTo("");
-        }
-        else {
-            fail("Annotation was not MetricTag");
-        }
+        assertThat(annotation).isInstanceOf(MeterTag.class);
+        String resolvedValue = MeterTagAnnotationHandler.resolveTagValue((MeterTag) annotation, "test",
+                aClass -> valueResolver, aClass -> valueExpressionResolver);
+        assertThat(resolvedValue).isEmpty();
     }
 
     @Test
     void shouldUseEmptyStringWhenArgumentIsNull() throws NoSuchMethodException, SecurityException {
         Method method = AnnotationMockClass.class.getMethod("getAnnotationForArgumentToString", Long.class);
         Annotation annotation = method.getParameterAnnotations()[0][0];
-        if (annotation instanceof MeterTag) {
-            String resolvedValue = this.handler.resolveTagValue((MeterTag) annotation, null, aClass -> valueResolver,
-                    aClass -> valueExpressionResolver);
-            assertThat(resolvedValue).isEqualTo("");
-        }
-        else {
-            fail("Annotation was not SpanTag");
-        }
+        assertThat(annotation).isInstanceOf(MeterTag.class);
+        String resolvedValue = MeterTagAnnotationHandler.resolveTagValue((MeterTag) annotation, null,
+                aClass -> valueResolver, aClass -> valueExpressionResolver);
+        assertThat(resolvedValue).isEmpty();
     }
 
-    protected class AnnotationMockClass {
+    static class AnnotationMockClass {
 
         @Timed
         public void getAnnotationForTagValueResolver(