Fix logic to choose ObservationConvention

fixes gh-3543

Author: jonatan-ivanov

micrometer-observation-test/src/main/java/io/micrometer/observation/tck/ObservationRegistryCompatibilityKit.java

@@ -765,25 +765,20 @@ void observationFieldsShouldBeSetOnContext() {
                 .highCardinalityKeyValue("hcTag1", "0")
                 // should override the previous line
                 .highCardinalityKeyValue("hcTag1", "3").highCardinalityKeyValues(KeyValues.of("hcTag2", "4"))
-                .observationConvention(new TestObservationConvention("local"))
-                .observationConvention(new UnsupportedObservationConvention("local"))
                 .contextualName("test.observation.42").error(exception).start();
         observation.stop();
 
         assertingHandler.checkAssertions(context -> {
             assertThat(context).isSameAs(testContext);
             assertThat(context.getName()).isEqualTo("test.observation");
             assertThat(context.getLowCardinalityKeyValues()).containsExactlyInAnyOrder(KeyValue.of("lcTag1", "1"),
-                    KeyValue.of("lcTag2", "2"), KeyValue.of("local.context.class", "TestContext"),
-                    KeyValue.of("global.context.class", "TestContext"));
+                    KeyValue.of("lcTag2", "2"), KeyValue.of("global.context.class", "TestContext"));
             assertThat(context.getHighCardinalityKeyValues()).containsExactlyInAnyOrder(KeyValue.of("hcTag1", "3"),
-                    KeyValue.of("hcTag2", "4"), KeyValue.of("local.uuid", testContext.uuid),
-                    KeyValue.of("global.uuid", testContext.uuid));
+                    KeyValue.of("hcTag2", "4"), KeyValue.of("global.uuid", testContext.uuid));
 
             assertThat(context.getAllKeyValues()).containsExactlyInAnyOrder(KeyValue.of("lcTag1", "1"),
-                    KeyValue.of("lcTag2", "2"), KeyValue.of("local.context.class", "TestContext"),
-                    KeyValue.of("global.context.class", "TestContext"), KeyValue.of("hcTag1", "3"),
-                    KeyValue.of("hcTag2", "4"), KeyValue.of("local.uuid", testContext.uuid),
+                    KeyValue.of("lcTag2", "2"), KeyValue.of("global.context.class", "TestContext"),
+                    KeyValue.of("hcTag1", "3"), KeyValue.of("hcTag2", "4"),
                     KeyValue.of("global.uuid", testContext.uuid));
 
             assertThat((String) context.get("context.field")).isEqualTo("42");
@@ -795,9 +790,9 @@ void observationFieldsShouldBeSetOnContext() {
                     .containsOnlyOnce("contextualName='test.observation.42'")
                     .containsOnlyOnce("error='java.io.IOException: simulated'")
                     .containsOnlyOnce(
-                            "lowCardinalityKeyValues=[global.context.class='TestContext', lcTag1='1', lcTag2='2', local.context.class='TestContext']")
-                    .containsOnlyOnce("highCardinalityKeyValues=[global.uuid='" + testContext.uuid
-                            + "', hcTag1='3', hcTag2='4', local.uuid='" + testContext.uuid + "']")
+                            "lowCardinalityKeyValues=[global.context.class='TestContext', lcTag1='1', lcTag2='2']")
+                    .containsOnlyOnce(
+                            "highCardinalityKeyValues=[global.uuid='" + testContext.uuid + "', hcTag1='3', hcTag2='4']")
                     .containsOnlyOnce("map=[context.field='42']");
         });
     }

micrometer-observation/src/main/java/io/micrometer/observation/Observation.java

@@ -127,6 +127,7 @@ static <T extends Context> Observation createNotStarted(String name, Supplier<T>
         return new SimpleObservation(name, registry, context == null ? new Context() : context);
     }
 
+    // @formatter:off
     /**
      * Creates but <b>does not start</b> an {@link Observation}. Remember to call
      * {@link Observation#start()} when you want the measurements to start. When the
@@ -143,15 +144,37 @@ static <T extends Context> Observation createNotStarted(String name, Supplier<T>
      * {@link ObservationRegistry.ObservationConfig#getObservationConvention(Context, ObservationConvention)})
      * was found. The {@link ObservationConvention} implementation can override
      * {@link Observation} names (i.e. name and contextual name) and key values.
+     * <pre>
+     * Here you can find the matrix of choosing the convention:
+     * 1. custom     default     no pre-configured  -> custom
+     * 2. custom     default     pre-configured     -> custom (not a valid case, just use custom)
+     * 3. no custom  default     no pre-configured  -> default
+     * 4. no custom  default     pre-configured     -> pre-configured
+     * 5. custom     no default  no pre-configured  -> custom (providing default is recommended)
+     * 6. custom     no default  pre-configured     -> custom (providing default is recommended)
+     * 7. no custom  no default  no pre-configured  -> local names/tags will be used
+     * 8. no custom  no default  pre-configured     -> pre-configured
+     * </pre>
+     * <p>
+     * Also, if you set a convention using,
+     * {@link Observation#observationConvention(ObservationConvention)} (not recommended),
+     * the convention you set here will be used and everything else (custom, default,
+     * pre-configured) will be ignored.
+     * </p>
+     * <p>
+     * If you want to add to the all the contexts or mutate them,
+     * use the ObservationFilter (e.g.: add region=cloud-1 or remove PII).
+     * </p>
      * @param <T> type of context
      * @param customConvention custom convention. If {@code null}, the default one will be
      * picked.
      * @param defaultConvention default convention when no custom convention was passed,
-     * nor a configured one was found
+     * nor a pre-configured one was found
      * @param contextSupplier supplier for the observation context
      * @param registry observation registry
      * @return created but not started observation
      */
+    // @formatter:on
     static <T extends Context> Observation createNotStarted(@Nullable ObservationConvention<T> customConvention,
             ObservationConvention<T> defaultConvention, Supplier<T> contextSupplier, ObservationRegistry registry) {
         if (registry.isNoop()) {
@@ -174,6 +197,11 @@ static <T extends Context> Observation createNotStarted(@Nullable ObservationCon
     /**
      * Creates and starts an {@link Observation}. When no registry is passed or
      * observation is not applicable will return a no-op observation.
+     * <p>
+     * Please check the javadoc of
+     * {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
+     * method for the logic of choosing the convention.
+     * </p>
      * @param observationConvention observation convention
      * @param registry observation registry
      * @return started observation
@@ -192,6 +220,11 @@ static Observation start(ObservationConvention<Context> observationConvention, O
      * {@link ObservationRegistry.ObservationConfig#observationPredicate(ObservationPredicate)
      * ObservationConfig#observationPredicate}), a no-op observation will also be
      * returned.
+     * <p>
+     * Please check the javadoc of
+     * {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
+     * method for the logic of choosing the convention.
+     * </p>
      * @param <T> type of context
      * @param observationConvention observation convention
      * @param contextSupplier mutable context supplier
@@ -216,6 +249,11 @@ static <T extends Context> Observation start(ObservationConvention<T> observatio
      * provide a default one if neither a custom nor a pre-configured one (via
      * {@link ObservationRegistry.ObservationConfig#getObservationConvention(Context, ObservationConvention)})
      * was found.
+     * <p>
+     * Please check the javadoc of
+     * {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
+     * method for the logic of choosing the convention.
+     * </p>
      * @param <T> type of context
      * @param registry observation registry
      * @param contextSupplier the observation context supplier
@@ -235,6 +273,11 @@ static <T extends Context> Observation start(@Nullable ObservationConvention<T>
      * {@link Observation#start()} when you want the measurements to start. When no
      * registry is passed or observation is not applicable will return a no-op
      * observation.
+     * <p>
+     * Please check the javadoc of
+     * {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
+     * method for the logic of choosing the convention.
+     * </p>
      * @param observationConvention observation convention
      * @param registry observation registry
      * @return created but not started observation
@@ -265,6 +308,11 @@ static Observation createNotStarted(ObservationConvention<Context> observationCo
      * of {@link Context} to be passed and if you're not providing one we won't be able to
      * initialize it ourselves.
      * </p>
+     * <p>
+     * Please check the javadoc of
+     * {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
+     * method for the logic of choosing the convention.
+     * </p>
      * @param <T> type of context
      * @param observationConvention observation convention
      * @param contextSupplier mutable context supplier
@@ -380,8 +428,8 @@ default boolean isNoop() {
 
     /**
      * Adds an observation convention that can be used to attach key values to the
-     * observation. WARNING: You must add ObservationConvention instances to the
-     * Observation before it is started.
+     * observation. WARNING: You must set the ObservationConvention to the Observation
+     * before it is started.
      * @param observationConvention observation convention
      * @return this
      */

micrometer-observation/src/main/java/io/micrometer/observation/ObservationRegistry.java

@@ -21,6 +21,7 @@
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Supplier;
 
 /**
  * Implementations of this interface are responsible for managing state of an
@@ -135,6 +136,11 @@ public ObservationConfig observationFilter(ObservationFilter observationFilter)
 
         /**
          * Register an {@link ObservationConvention}.
+         * <p>
+         * Please check the javadoc of
+         * {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
+         * method for the logic of choosing the convention.
+         * </p>
          * @param observationConvention observation convention
          * @return This configuration instance
          */

micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java

@@ -38,8 +38,9 @@ class SimpleObservation implements Observation {
 
     private final Context context;
 
+    @Nullable
     @SuppressWarnings("rawtypes")
-    private final Collection<ObservationConvention> conventions;
+    private ObservationConvention convention;
 
     @SuppressWarnings("rawtypes")
     private final Deque<ObservationHandler> handlers;
@@ -50,27 +51,31 @@ class SimpleObservation implements Observation {
         this.registry = registry;
         this.context = context;
         this.context.setName(name);
-        this.conventions = getConventionsFromConfig(registry, context);
+        this.convention = getConventionFromConfig(registry, context);
         this.handlers = getHandlersFromConfig(registry, context);
         this.filters = registry.observationConfig().getObservationFilters();
     }
 
     SimpleObservation(ObservationConvention<? extends Context> convention, ObservationRegistry registry,
             Context context) {
-        this((String) null, registry, context); // name is set later in start()
+        this.registry = registry;
+        this.context = context;
+        // name is set later in start()
+        this.handlers = getHandlersFromConfig(registry, context);
+        this.filters = registry.observationConfig().getObservationFilters();
         if (convention.supportsContext(context)) {
-            this.conventions.add(convention);
+            this.convention = convention;
         }
         else {
             throw new IllegalStateException(
                     "Convention [" + convention + "] doesn't support context [" + context + "]");
         }
     }
 
-    private static Collection<ObservationConvention> getConventionsFromConfig(ObservationRegistry registry,
-            Context context) {
+    @Nullable
+    private static ObservationConvention getConventionFromConfig(ObservationRegistry registry, Context context) {
         return registry.observationConfig().getObservationConventions().stream()
-                .filter(convention -> convention.supportsContext(context)).collect(Collectors.toList());
+                .filter(convention -> convention.supportsContext(context)).findFirst().orElse(null);
     }
 
     private static Deque<ObservationHandler> getHandlersFromConfig(ObservationRegistry registry, Context context) {
@@ -105,7 +110,7 @@ public Observation highCardinalityKeyValue(KeyValue keyValue) {
     @Override
     public Observation observationConvention(ObservationConvention<?> convention) {
         if (convention.supportsContext(context)) {
-            this.conventions.add(convention);
+            this.convention = convention;
         }
         return this;
     }
@@ -125,18 +130,13 @@ public Observation event(Event event) {
 
     @Override
     public Observation start() {
-        // We want to rename with the first matching convention
-        boolean nameChanged = false;
-        for (ObservationConvention convention : this.conventions) {
+        if (this.convention != null) {
             this.context.addLowCardinalityKeyValues(convention.getLowCardinalityKeyValues(context));
             this.context.addHighCardinalityKeyValues(convention.getHighCardinalityKeyValues(context));
 
-            if (!nameChanged) {
-                String newName = convention.getName();
-                if (StringUtils.isNotBlank(newName)) {
-                    this.context.setName(newName);
-                    nameChanged = true;
-                }
+            String newName = convention.getName();
+            if (StringUtils.isNotBlank(newName)) {
+                this.context.setName(newName);
             }
         }
 
@@ -152,18 +152,13 @@ public Context getContext() {
     @SuppressWarnings({ "rawtypes", "unchecked" })
     @Override
     public void stop() {
-        // We want to rename with the first matching convention
-        boolean contextualNameChanged = false;
-        for (ObservationConvention convention : this.conventions) {
+        if (this.convention != null) {
             this.context.addLowCardinalityKeyValues(convention.getLowCardinalityKeyValues(context));
             this.context.addHighCardinalityKeyValues(convention.getHighCardinalityKeyValues(context));
 
-            if (!contextualNameChanged) {
-                String newContextualName = convention.getContextualName(context);
-                if (StringUtils.isNotBlank(newContextualName)) {
-                    this.context.setContextualName(newContextualName);
-                    contextualNameChanged = true;
-                }
+            String newContextualName = convention.getContextualName(context);
+            if (StringUtils.isNotBlank(newContextualName)) {
+                this.context.setContextualName(newContextualName);
             }
         }
 

micrometer-observation/src/test/java/io/micrometer/observation/ObservationRegistryTest.java

@@ -96,8 +96,6 @@ void observationShouldWorkWithConventions() {
         registry.observationConfig().observationHandler(c -> true);
         // Define a convention
         MessagingConvention messagingConvention = new OurCompanyStandardMessagingConvention();
-        // Register a semantic name provider
-        registry.observationConfig().observationConvention(new OurCompanyObservationConvention());
 
         Observation.Context myContext = new MessagingContext().put("foo", "hello");
         // Observation convention wants to use a MessagingConvention
@@ -125,6 +123,12 @@ static class MessagingObservationConvention implements ObservationConvention<Mes
             this.messagingConvention = messagingConvention;
         }
 
+        // Here we override the default "observation" name
+        @Override
+        public String getName() {
+            return "new name";
+        }
+
         @Override
         public KeyValues getLowCardinalityKeyValues(MessagingContext context) {
             return KeyValues.of(this.messagingConvention.queueName(context.get("foo")));
@@ -153,22 +157,4 @@ public KeyValue queueName(String messagePayload) {
 
     }
 
-    static class OurCompanyObservationConvention implements GlobalObservationConvention<Observation.Context> {
-
-        // Here we override the default "observation" name
-        @Override
-        public String getName() {
-            return "new name";
-        }
-
-        // This semantic name provider is only applicable when we're using a messaging
-        // context
-
-        @Override
-        public boolean supportsContext(Observation.Context context) {
-            return context instanceof MessagingContext;
-        }
-
-    }
-
 }