Throw exception in case of invalid Observations with TestObservationRegistry (#5307)

Logs can be very easily missed in tests. This switches to throwing an exception which will be much more noticeable.
See gh-5300

Author: jonatan-ivanov

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

@@ -0,0 +1,41 @@
+/*
+ * Copyright 2024 VMware, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.micrometer.observation.tck;
+
+import io.micrometer.observation.Observation;
+import io.micrometer.observation.Observation.Context;
+
+/**
+ * A {@link RuntimeException} that can be thrown when an invalid {@link Observation}
+ * detected.
+ *
+ * @author Jonatan Ivanov
+ * @since 1.14.0
+ */
+public class InvalidObservationException extends RuntimeException {
+
+    private final Context context;
+
+    InvalidObservationException(String message, Context context) {
+        super(message);
+        this.context = context;
+    }
+
+    public Context getContext() {
+        return context;
+    }
+
+}

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

@@ -16,8 +16,6 @@
 package io.micrometer.observation.tck;
 
 import io.micrometer.common.lang.Nullable;
-import io.micrometer.common.util.internal.logging.InternalLogger;
-import io.micrometer.common.util.internal.logging.InternalLoggerFactory;
 import io.micrometer.observation.Observation.Context;
 import io.micrometer.observation.Observation.Event;
 import io.micrometer.observation.ObservationHandler;
@@ -35,14 +33,12 @@
  */
 class ObservationValidator implements ObservationHandler<Context> {
 
-    private static final InternalLogger LOGGER = InternalLoggerFactory.getInstance(ObservationValidator.class);
-
     private final Consumer<ValidationResult> consumer;
 
     private final Predicate<Context> supportsContextPredicate;
 
     ObservationValidator() {
-        this(validationResult -> LOGGER.warn(validationResult.toString()));
+        this(ObservationValidator::throwInvalidObservationException);
     }
 
     ObservationValidator(Consumer<ValidationResult> consumer) {
@@ -116,6 +112,10 @@ else if (status.isStopped()) {
         return status;
     }
 
+    private static void throwInvalidObservationException(ValidationResult validationResult) {
+        throw new InvalidObservationException(validationResult.getMessage(), validationResult.getContext());
+    }
+
     static class ValidationResult {
 
         private final String message;

micrometer-observation-test/src/test/java/io/micrometer/observation/tck/ObservationValidatorTests.java

@@ -19,13 +19,9 @@
 import io.micrometer.observation.Observation.Event;
 import io.micrometer.observation.Observation.Scope;
 import io.micrometer.observation.ObservationRegistry;
-import io.micrometer.observation.tck.ObservationValidator.ValidationResult;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-import java.util.function.Consumer;
-
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 /**
  * Tests for {@link ObservationValidator}.
@@ -34,109 +30,141 @@
  */
 class ObservationValidatorTests {
 
-    private TestConsumer testConsumer;
-
-    private ObservationRegistry registry;
-
-    @BeforeEach
-    void setUp() {
-        testConsumer = new TestConsumer();
-        registry = ObservationRegistry.create();
-        registry.observationConfig().observationHandler(new ObservationValidator(testConsumer));
-    }
+    private final ObservationRegistry registry = TestObservationRegistry.create();
 
     @Test
     void doubleStartShouldBeInvalid() {
-        Observation.start("test", registry).start();
-        assertThat(testConsumer.toString()).isEqualTo("Invalid start: Observation has already been started");
+        assertThatThrownBy(() -> Observation.start("test", registry).start())
+            .isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid start: Observation has already been started");
     }
 
     @Test
     void stopBeforeStartShouldBeInvalid() {
-        Observation.createNotStarted("test", registry).stop();
-        assertThat(testConsumer.toString()).isEqualTo("Invalid stop: Observation has not been started yet");
+        assertThatThrownBy(() -> Observation.createNotStarted("test", registry).stop())
+            .isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid stop: Observation has not been started yet");
     }
 
     @Test
     void errorBeforeStartShouldBeInvalid() {
-        Observation.createNotStarted("test", registry).error(new RuntimeException());
-        assertThat(testConsumer.toString()).isEqualTo("Invalid error signal: Observation has not been started yet");
+        assertThatThrownBy(() -> Observation.createNotStarted("test", registry).error(new RuntimeException()))
+            .isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid error signal: Observation has not been started yet");
     }
 
     @Test
     void eventBeforeStartShouldBeInvalid() {
-        Observation.createNotStarted("test", registry).event(Event.of("test"));
-        assertThat(testConsumer.toString()).isEqualTo("Invalid event signal: Observation has not been started yet");
+        assertThatThrownBy(() -> Observation.createNotStarted("test", registry).event(Event.of("test")))
+            .isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid event signal: Observation has not been started yet");
     }
 
     @Test
+    @SuppressWarnings("resource")
     void scopeBeforeStartShouldBeInvalid() {
-        Scope scope = Observation.createNotStarted("test", registry).openScope();
-        scope.reset();
-        scope.close();
-        assertThat(testConsumer.toString()).isEqualTo("Invalid scope opening: Observation has not been started yet\n"
-                + "Invalid scope resetting: Observation has not been started yet\n"
-                + "Invalid scope closing: Observation has not been started yet");
+        // Since openScope throws an exception, reset and close can't happen
+        assertThatThrownBy(() -> Observation.createNotStarted("test", registry).openScope())
+            .isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid scope opening: Observation has not been started yet");
     }
 
     @Test
     void observeAfterStartShouldBeInvalid() {
-        Observation.start("test", registry).observe(() -> "");
-        assertThat(testConsumer.toString()).isEqualTo("Invalid start: Observation has already been started");
+        assertThatThrownBy(() -> Observation.start("test", registry).observe(() -> ""))
+            .isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid start: Observation has already been started");
     }
 
     @Test
     void doubleStopShouldBeInvalid() {
-        Observation observation = Observation.start("test", registry);
-        observation.stop();
-        observation.stop();
-        assertThat(testConsumer.toString()).isEqualTo("Invalid stop: Observation has already been stopped");
+        assertThatThrownBy(() -> {
+            Observation observation = Observation.start("test", registry);
+            observation.stop();
+            observation.stop();
+        }).isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid stop: Observation has already been stopped");
     }
 
     @Test
     void errorAfterStopShouldBeInvalid() {
-        Observation observation = Observation.start("test", registry);
-        observation.stop();
-        observation.error(new RuntimeException());
-        assertThat(testConsumer.toString()).isEqualTo("Invalid error signal: Observation has already been stopped");
+        assertThatThrownBy(() -> {
+            Observation observation = Observation.start("test", registry);
+            observation.stop();
+            observation.error(new RuntimeException());
+        }).isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid error signal: Observation has already been stopped");
     }
 
     @Test
     void eventAfterStopShouldBeInvalid() {
-        Observation observation = Observation.start("test", registry);
-        observation.stop();
-        observation.event(Event.of("test"));
-        assertThat(testConsumer.toString()).isEqualTo("Invalid event signal: Observation has already been stopped");
+        assertThatThrownBy(() -> {
+            Observation observation = Observation.start("test", registry);
+            observation.stop();
+            observation.event(Event.of("test"));
+        }).isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid event signal: Observation has already been stopped");
     }
 
     @Test
-    void scopeAfterStopShouldBeInvalid() {
-        Observation observation = Observation.start("test", registry);
-        observation.stop();
-        Scope scope = observation.openScope();
-        scope.reset();
-        scope.close();
-        assertThat(testConsumer.toString()).isEqualTo("Invalid scope opening: Observation has already been stopped\n"
-                + "Invalid scope resetting: Observation has already been stopped\n"
-                + "Invalid scope closing: Observation has already been stopped");
+    @SuppressWarnings("resource")
+    void scopeOpenAfterStopShouldBeInvalid() {
+        assertThatThrownBy(() -> {
+            Observation observation = Observation.start("test", registry);
+            observation.stop();
+            observation.openScope();
+        }).isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid scope opening: Observation has already been stopped");
+    }
+
+    @Test
+    @SuppressWarnings("resource")
+    void scopeResetAfterStopShouldBeInvalid() {
+        assertThatThrownBy(() -> {
+            Observation observation = Observation.start("test", registry);
+            Scope scope = observation.openScope();
+            observation.stop();
+            scope.reset();
+        }).isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid scope resetting: Observation has already been stopped");
+    }
+
+    @Test
+    void scopeCloseAfterStopShouldBeInvalid() {
+        assertThatThrownBy(() -> {
+            Observation observation = Observation.start("test", registry);
+            Scope scope = observation.openScope();
+            observation.stop();
+            scope.close();
+        }).isExactlyInstanceOf(InvalidObservationException.class)
+            .hasNoCause()
+            .hasMessage("Invalid scope closing: Observation has already been stopped");
     }
 
     @Test
     void startEventStopShouldBeValid() {
         Observation.start("test", registry).event(Event.of("test")).stop();
-        assertThat(testConsumer.toString()).isEmpty();
     }
 
     @Test
     void startEventErrorStopShouldBeValid() {
         Observation.start("test", registry).event(Event.of("test")).error(new RuntimeException()).stop();
-        assertThat(testConsumer.toString()).isEmpty();
     }
 
     @Test
     void startErrorEventStopShouldBeValid() {
         Observation.start("test", registry).error(new RuntimeException()).event(Event.of("test")).stop();
-        assertThat(testConsumer.toString()).isEmpty();
     }
 
     @Test
@@ -145,7 +173,6 @@ void startScopeEventStopShouldBeValid() {
         observation.openScope().close();
         observation.event(Event.of("test"));
         observation.stop();
-        assertThat(testConsumer.toString()).isEmpty();
     }
 
     @Test
@@ -156,7 +183,6 @@ void startScopeEventErrorStopShouldBeValid() {
         observation.error(new RuntimeException());
         scope.close();
         observation.stop();
-        assertThat(testConsumer.toString()).isEmpty();
     }
 
     @Test
@@ -167,23 +193,6 @@ void startScopeErrorEventStopShouldBeValid() {
         observation.event(Event.of("test"));
         scope.close();
         observation.stop();
-        assertThat(testConsumer.toString()).isEmpty();
-    }
-
-    static class TestConsumer implements Consumer<ValidationResult> {
-
-        private final StringBuilder stringBuilder = new StringBuilder();
-
-        @Override
-        public void accept(ValidationResult validationResult) {
-            stringBuilder.append(validationResult.getMessage()).append("\n");
-        }
-
-        @Override
-        public String toString() {
-            return stringBuilder.toString().trim();
-        }
-
     }
 
 }

micrometer-observation-test/src/test/java/io/micrometer/observation/tck/TestObservationRegistryAssertTests.java

@@ -64,7 +64,7 @@ void should_not_break_on_multiple_threads() {
 
     @Test
     void should_fail_when_observation_not_started() {
-        Observation.createNotStarted("foo", registry).stop();
+        Observation.createNotStarted("foo", registry);
 
         thenThrownBy(
                 () -> TestObservationRegistryAssert.assertThat(registry).hasSingleObservationThat().hasBeenStarted())