Test against ObservationDocumentation in instrumentation TCK (micrometer-metrics#3372)

Instrumentation being tested against the TCK can optionally provide the ObservationDocumentation that specifies the expected semantic naming to be used in the instrumentation. This ensures that the implementation matches the specified documentation, which may be used to generate written documentation.

* Introduce InstrumentationTimingVerificationTests

This adds a new abstract class in between `InstrumentationVerificationTests` and the `HttpClientTimingInstrumentationVerificationTests`/`HttpServerTimingInstrumentationVerificationTests`. The class `InstrumentationTimingVerificationTests` contains methods and logic specifically for timing instrumentation. The goal is to leave `InstrumentationVerificationTests` generic enough that it may be used for other kinds of instrumentation verification.

Author: shakuzen

micrometer-core/src/main/java/io/micrometer/core/instrument/binder/httpcomponents/ApacheHttpClientObservationDocumentation.java

@@ -64,18 +64,33 @@ public String asString() {
             public String asString() {
                 return "target.scheme";
             }
+
+            @Override
+            public boolean isRequired() {
+                return false;
+            }
         },
         TARGET_HOST {
             @Override
             public String asString() {
                 return "target.host";
             }
+
+            @Override
+            public boolean isRequired() {
+                return false;
+            }
         },
         TARGET_PORT {
             @Override
             public String asString() {
                 return "target.port";
             }
+
+            @Override
+            public boolean isRequired() {
+                return false;
+            }
         }
 
     }

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

@@ -27,6 +27,9 @@
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
+import static java.util.stream.Collectors.toList;
+import static org.assertj.core.util.Streams.stream;
+
 /**
  * Assertion methods for {@code Observation.Context}s and
  * {@link Observation.ContextView}s.
@@ -197,6 +200,24 @@ else if (!extraKeys.isEmpty()) {
         return (SELF) this;
     }
 
+    /**
+     * Verifies that the Observation key-value keys are a subset of the given set of keys.
+     */
+    public SELF hasSubsetOfKeys(String... keys) {
+        isNotNull();
+        Set<String> actualKeys = new LinkedHashSet<>(allKeys());
+        Set<String> expectedKeys = new LinkedHashSet<>(Arrays.asList(keys));
+
+        List<String> extra = stream(actualKeys).filter(actualElement -> !expectedKeys.contains(actualElement))
+                .collect(toList());
+
+        if (extra.size() > 0) {
+            failWithMessage("Observation keys are not a subset of %s. Found extra keys: %s", keys, extra);
+        }
+
+        return (SELF) this;
+    }
+
     private List<String> lowCardinalityKeys() {
         return this.actual.getLowCardinalityKeyValues().stream().map(KeyValue::getKey).collect(Collectors.toList());
     }

micrometer-test/src/main/java/io/micrometer/core/instrument/HttpClientTimingInstrumentationVerificationTests.java

@@ -47,10 +47,9 @@
 @WireMockTest
 @Incubating(since = "1.8.8")
 public abstract class HttpClientTimingInstrumentationVerificationTests<CLIENT>
-        extends InstrumentationVerificationTests {
-
-    private TestType testType;
+        extends InstrumentationTimingVerificationTests {
 
+    @Nullable
     private CLIENT createdClient;
 
     /**
@@ -82,11 +81,11 @@ public enum HttpMethod {
      * @return instrumented client with either {@link MeterRegistry} or
      * {@link ObservationRegistry}
      */
-    private CLIENT instrumentedClient() {
+    private CLIENT instrumentedClient(TestType testType) {
         if (this.createdClient != null) {
             return this.createdClient;
         }
-        if (this.testType == TestType.METRICS_VIA_METER_REGISTRY) {
+        if (testType == TestType.METRICS_VIA_METER_REGISTRY) {
             this.createdClient = clientInstrumentedWithMetrics();
         }
         else {
@@ -95,12 +94,7 @@ private CLIENT instrumentedClient() {
         return this.createdClient;
     }
 
-    /**
-     * A default is provided that should be preferred by new instrumentations. Existing
-     * instrumentations that use a different value to maintain backwards compatibility may
-     * override this method to run tests with a different name used in assertions.
-     * @return name of the meter timing http client requests
-     */
+    @Override
     protected String timerName() {
         return "http.client.requests";
     }
@@ -154,7 +148,7 @@ void getTemplatedPathForUri(TestType testType, WireMockRuntimeInfo wmRuntimeInfo
         stubFor(get(anyUrl()).willReturn(ok()));
 
         String templatedPath = "/customers/{customerId}/carts/{cartId}";
-        sendHttpRequest(instrumentedClient(), HttpMethod.GET, null, URI.create(wmRuntimeInfo.getHttpBaseUrl()),
+        sendHttpRequest(instrumentedClient(testType), HttpMethod.GET, null, URI.create(wmRuntimeInfo.getHttpBaseUrl()),
                 templatedPath, "112", "5");
 
         Timer timer = getRegistry().get(timerName()).tags("method", "GET", "status", "200", "uri", templatedPath)
@@ -175,8 +169,8 @@ void timedWhenServerIsMissing(TestType testType) throws IOException {
         }
 
         try {
-            sendHttpRequest(instrumentedClient(), HttpMethod.GET, null, URI.create("http://localhost:" + unusedPort),
-                    "/anything");
+            sendHttpRequest(instrumentedClient(testType), HttpMethod.GET, null,
+                    URI.create("http://localhost:" + unusedPort), "/anything");
         }
         catch (Throwable ignore) {
         }
@@ -194,7 +188,7 @@ void serverException(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) {
 
         stubFor(get(anyUrl()).willReturn(serverError()));
 
-        sendHttpRequest(instrumentedClient(), HttpMethod.GET, null, URI.create(wmRuntimeInfo.getHttpBaseUrl()),
+        sendHttpRequest(instrumentedClient(testType), HttpMethod.GET, null, URI.create(wmRuntimeInfo.getHttpBaseUrl()),
                 "/socks");
 
         Timer timer = getRegistry().get(timerName()).tags("method", "GET", "status", "500").timer();
@@ -209,8 +203,9 @@ void clientException(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) {
 
         stubFor(post(anyUrl()).willReturn(badRequest()));
 
-        sendHttpRequest(instrumentedClient(), HttpMethod.POST, new byte[0], URI.create(wmRuntimeInfo.getHttpBaseUrl()),
-                "/socks");
+        // Some HTTP clients fail POST requests with a null body
+        sendHttpRequest(instrumentedClient(testType), HttpMethod.POST, new byte[0],
+                URI.create(wmRuntimeInfo.getHttpBaseUrl()), "/socks");
 
         Timer timer = getRegistry().get(timerName()).tags("method", "POST", "status", "400").timer();
         assertThat(timer.count()).isEqualTo(1);
@@ -222,7 +217,6 @@ private void checkAndSetupTestForTestType(TestType testType) {
             Assumptions.assumeTrue(clientInstrumentedWithObservations() != null,
                     "You must implement the <clientInstrumentedWithObservations> method to test your instrumentation against an ObservationRegistry");
         }
-        this.testType = testType;
     }
 
 }

micrometer-test/src/main/java/io/micrometer/core/instrument/HttpServerTimingInstrumentationVerificationTests.java

@@ -41,20 +41,15 @@
  */
 @Incubating(since = "1.8.9")
 @ExtendWith(InstrumentationVerificationTests.AfterBeforeParameterResolver.class)
-public abstract class HttpServerTimingInstrumentationVerificationTests extends InstrumentationVerificationTests {
+public abstract class HttpServerTimingInstrumentationVerificationTests extends InstrumentationTimingVerificationTests {
 
     private final HttpSender sender = new HttpUrlConnectionSender();
 
     private URI baseUri;
 
     private boolean assumptionSucceeded = true;
 
-    /**
-     * A default is provided that should be preferred by new instrumentations. Existing
-     * instrumentations that use a different value to maintain backwards compatibility may
-     * override this method to run tests with a different name used in assertions.
-     * @return name of the meter timing http server requests
-     */
+    @Override
     protected String timerName() {
         return "http.server.requests";
     }

micrometer-test/src/main/java/io/micrometer/core/instrument/InstrumentationTimingVerificationTests.java

@@ -0,0 +1,108 @@
+/*
+ * Copyright 2022 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.core.instrument;
+
+import io.micrometer.common.docs.KeyName;
+import io.micrometer.common.lang.Nullable;
+import io.micrometer.observation.docs.ObservationDocumentation;
+import io.micrometer.observation.tck.TestObservationRegistryAssert;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+@ExtendWith(InstrumentationVerificationTests.AfterBeforeParameterResolver.class)
+abstract class InstrumentationTimingVerificationTests extends InstrumentationVerificationTests {
+
+    /**
+     * A default is provided that should be preferred by new instrumentations. Existing
+     * instrumentations that use a different value to maintain backwards compatibility may
+     * override this method to run tests with a different name used in assertions.
+     * @return name of the Timer meter produced from the timing instrumentation under test
+     */
+    protected abstract String timerName();
+
+    /**
+     * If a {@link ObservationDocumentation} is provided the tests run will check that the
+     * produced instrumentation matches the given {@link ObservationDocumentation}.
+     * @return the documented observation to compare results against, or null to do
+     * nothing
+     */
+    @Nullable
+    protected ObservationDocumentation observationDocumentation() {
+        return null;
+    }
+
+    @AfterEach
+    void verifyObservationDocumentation(TestType testType) {
+        ObservationDocumentation observationDocumentation = observationDocumentation();
+        if (observationDocumentation == null) {
+            return;
+        }
+
+        Timer timer = getRegistry().get(timerName()).timer();
+        Set<String> requiredDocumentedLowCardinalityKeys = getRequiredLowCardinalityKeyNames(observationDocumentation);
+        Set<String> requiredTagKeys = new HashSet<>(requiredDocumentedLowCardinalityKeys);
+        if (testType == TestType.METRICS_VIA_OBSERVATIONS_WITH_METRICS_HANDLER) {
+            requiredTagKeys.add("error");
+        }
+        Set<String> allDocumentedLowCardinalityKeys = getLowCardinalityKeyNames(observationDocumentation);
+        Set<String> allPossibleTagKeys = new HashSet<>(allDocumentedLowCardinalityKeys);
+        if (testType == TestType.METRICS_VIA_OBSERVATIONS_WITH_METRICS_HANDLER) {
+            allPossibleTagKeys.add("error");
+        }
+
+        // must have all required tag keys
+        assertThat(timer.getId().getTags()).extracting(Tag::getKey).containsAll(requiredTagKeys);
+        // must not contain tag keys that aren't documented
+        assertThat(timer.getId().getTags()).extracting(Tag::getKey).isSubsetOf(allPossibleTagKeys);
+
+        if (testType == TestType.METRICS_VIA_OBSERVATIONS_WITH_METRICS_HANDLER) {
+            if (observationDocumentation.getDefaultConvention() == null) {
+                TestObservationRegistryAssert.assertThat(getObservationRegistry())
+                        .hasObservationWithNameEqualTo(observationDocumentation.getName()).that()
+                        .hasContextualNameEqualTo(observationDocumentation.getContextualName());
+            }
+            TestObservationRegistryAssert.assertThat(getObservationRegistry())
+                    .hasObservationWithNameEqualTo(timerName()).that()
+                    .hasSubsetOfKeys(getAllKeyNames(observationDocumentation));
+        }
+    }
+
+    private Set<String> getRequiredLowCardinalityKeyNames(ObservationDocumentation observationDocumentation) {
+        return Arrays.stream(observationDocumentation.getLowCardinalityKeyNames()).filter(KeyName::isRequired)
+                .map(KeyName::asString).collect(Collectors.toSet());
+    }
+
+    private Set<String> getLowCardinalityKeyNames(ObservationDocumentation observationDocumentation) {
+        return Arrays.stream(observationDocumentation.getLowCardinalityKeyNames()).map(KeyName::asString)
+                .collect(Collectors.toSet());
+    }
+
+    private String[] getAllKeyNames(ObservationDocumentation observationDocumentation) {
+        return Stream
+                .concat(Arrays.stream(observationDocumentation.getLowCardinalityKeyNames()),
+                        Arrays.stream(observationDocumentation.getHighCardinalityKeyNames()))
+                .map(KeyName::asString).toArray(String[]::new);
+    }
+
+}

micrometer-test/src/main/java/io/micrometer/core/instrument/InstrumentationVerificationTests.java

@@ -17,7 +17,6 @@
 
 import io.micrometer.core.instrument.observation.DefaultMeterObservationHandler;
 import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
-import io.micrometer.observation.ObservationRegistry;
 import io.micrometer.observation.tck.TestObservationRegistry;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -31,9 +30,7 @@
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Parameter;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Optional;
+import java.util.*;
 
 abstract class InstrumentationVerificationTests {
 
@@ -61,7 +58,7 @@ protected TestObservationRegistry createObservationRegistryWithMetrics() {
         return observationRegistry;
     }
 
-    protected ObservationRegistry getObservationRegistry() {
+    protected TestObservationRegistry getObservationRegistry() {
         return this.testObservationRegistry;
     }
 
@@ -76,7 +73,7 @@ enum TestType {
         METRICS_VIA_METER_REGISTRY,
 
         /**
-         * Runs the tests by using the Observation API.
+         * Runs the tests by using the Observation API and a MeterObservationHandler.
          */
         METRICS_VIA_OBSERVATIONS_WITH_METRICS_HANDLER
 

micrometer-test/src/test/java/io/micrometer/core/instrument/ApacheHttpClientTimingInstrumentationVerificationTests.java

@@ -16,8 +16,10 @@
 package io.micrometer.core.instrument;
 
 import io.micrometer.common.lang.Nullable;
+import io.micrometer.core.instrument.binder.httpcomponents.ApacheHttpClientObservationDocumentation;
 import io.micrometer.core.instrument.binder.httpcomponents.DefaultUriMapper;
 import io.micrometer.core.instrument.binder.httpcomponents.MicrometerHttpRequestExecutor;
+import io.micrometer.observation.docs.ObservationDocumentation;
 import org.apache.http.client.HttpClient;
 import org.apache.http.client.methods.HttpEntityEnclosingRequestBase;
 import org.apache.http.client.methods.HttpUriRequest;
@@ -50,6 +52,11 @@ protected String timerName() {
         return "httpcomponents.httpclient.request";
     }
 
+    @Override
+    protected ObservationDocumentation observationDocumentation() {
+        return ApacheHttpClientObservationDocumentation.DEFAULT;
+    }
+
     @Override
     protected void sendHttpRequest(HttpClient instrumentedClient, HttpMethod method, @Nullable byte[] body, URI baseUri,
             String templatedPath, String... pathVariables) {

micrometer-test/src/test/java/io/micrometer/core/instrument/JerseyServerTimingInstrumentationVerificationTests.java

@@ -17,8 +17,10 @@
 
 import io.micrometer.common.lang.Nullable;
 import io.micrometer.core.instrument.binder.jersey.server.DefaultJerseyTagsProvider;
+import io.micrometer.core.instrument.binder.jersey.server.JerseyObservationDocumentation;
 import io.micrometer.core.instrument.binder.jersey.server.MetricsApplicationEventListener;
 import io.micrometer.core.instrument.binder.jersey.server.ObservationApplicationEventListener;
+import io.micrometer.observation.docs.ObservationDocumentation;
 import org.glassfish.jersey.server.ResourceConfig;
 import org.glassfish.jersey.test.JerseyTest;
 
@@ -46,6 +48,11 @@ protected URI startInstrumentedWithObservationsServer() throws Exception {
         return setupUri(jerseyTest);
     }
 
+    @Override
+    protected ObservationDocumentation observationDocumentation() {
+        return JerseyObservationDocumentation.DEFAULT;
+    }
+
     private JerseyTest jerseyWithListener(Object listener) {
         return new JerseyTest() {
             @Override

micrometer-test/src/test/java/io/micrometer/core/instrument/OkHttpClientTimingInstrumentationVerificationTests.java

@@ -17,7 +17,9 @@
 
 import io.micrometer.common.lang.Nullable;
 import io.micrometer.core.instrument.binder.okhttp3.OkHttpMetricsEventListener;
+import io.micrometer.core.instrument.binder.okhttp3.OkHttpObservationDocumentation;
 import io.micrometer.core.instrument.binder.okhttp3.OkHttpObservationInterceptor;
+import io.micrometer.observation.docs.ObservationDocumentation;
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.RequestBody;
@@ -55,4 +57,9 @@ protected OkHttpClient clientInstrumentedWithObservations() {
                 .build();
     }
 
+    @Override
+    protected ObservationDocumentation observationDocumentation() {
+        return OkHttpObservationDocumentation.DEFAULT;
+    }
+
 }