Merge pull request #45300 from gsmet/fix-opentelemetry-suppress-manag…

…ement

Fix how Vert.x routes are identified in metrics and OpenTelemetry

extensions/opentelemetry/deployment/pom.xml

@@ -145,6 +145,11 @@
             <artifactId>quarkus-security-deployment</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>io.quarkus</groupId>
+            <artifactId>quarkus-vertx-http-dev-ui-tests</artifactId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>io.opentelemetry</groupId>
             <artifactId>opentelemetry-sdk-testing</artifactId>

extensions/opentelemetry/deployment/src/test/java/io/quarkus/opentelemetry/deployment/OpenTelemetrySuppressNonAppUriManagementInterfaceTest.java

@@ -0,0 +1,101 @@
+package io.quarkus.opentelemetry.deployment;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Arrays;
+import java.util.List;
+
+import jakarta.inject.Inject;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.Path;
+
+import org.jboss.shrinkwrap.api.asset.StringAsset;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.opentelemetry.sdk.trace.data.SpanData;
+import io.quarkus.opentelemetry.deployment.common.exporter.TestSpanExporter;
+import io.quarkus.opentelemetry.deployment.common.exporter.TestSpanExporterProvider;
+import io.quarkus.test.QuarkusDevModeTest;
+import io.restassured.RestAssured;
+
+public class OpenTelemetrySuppressNonAppUriManagementInterfaceTest {
+
+    @RegisterExtension
+    final static QuarkusDevModeTest TEST = new QuarkusDevModeTest()
+            .withApplicationRoot((jar) -> jar
+                    .addClasses(HelloResource.class, TestSpanExporter.class, TestSpanExporterProvider.class)
+                    .addAsResource(new StringAsset(TestSpanExporterProvider.class.getCanonicalName()),
+                            "META-INF/services/io.opentelemetry.sdk.autoconfigure.spi.traces.ConfigurableSpanExporterProvider")
+                    .add(new StringAsset(
+                            """
+                                    quarkus.otel.traces.exporter=test-span-exporter
+                                    quarkus.otel.metrics.exporter=none
+                                    quarkus.otel.bsp.export.timeout=1s
+                                    quarkus.otel.bsp.schedule.delay=50
+                                    quarkus.management.enabled=true
+                                    quarkus.management.port=9001
+                                    """),
+                            "application.properties"));
+
+    @Test
+    void test() {
+
+        // Must not be traced
+        RestAssured.given()
+                .get("http://localhost:9001/q/health/")
+                .then()
+                .statusCode(200);
+        RestAssured.given()
+                .get("/q/dev-ui/")
+                .then()
+                .statusCode(200);
+        RestAssured.given()
+                .get("/q/dev-ui/icon/font-awesome.js")
+                .then()
+                .statusCode(200);
+        // Valid trace
+        RestAssured.given()
+                .get("/hello")
+                .then()
+                .statusCode(200);
+        // Get span names
+        List<String> spans = Arrays.asList(
+                RestAssured.given()
+                        .get("/hello/spans")
+                        .then()
+                        .statusCode(200)
+                        .extract().body()
+                        .asString()
+                        .split(";"));
+
+        assertThat(spans).containsExactly("GET /hello");
+    }
+
+    @Path("/hello")
+    public static class HelloResource {
+
+        @Inject
+        TestSpanExporter spanExporter;
+
+        @GET
+        public String greetings() {
+            return "Hello test";
+        }
+
+        /**
+         * Gets a string with the received spans names concatenated by ;
+         *
+         * @return
+         */
+        @GET
+        @Path("/spans")
+        public String greetingsInsertAtLeast() {
+            String spanNames = spanExporter.getFinishedSpanItemsAtLeast(1).stream()
+                    .map(SpanData::getName)
+                    .reduce((s1, s2) -> s1 + ";" + s2).orElse("");
+            System.out.println(spanNames);
+            return spanNames;
+        }
+    }
+}

extensions/opentelemetry/deployment/src/test/java/io/quarkus/opentelemetry/deployment/OpenTelemetrySuppressNonAppUriTest.java

@@ -0,0 +1,99 @@
+package io.quarkus.opentelemetry.deployment;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Arrays;
+import java.util.List;
+
+import jakarta.inject.Inject;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.Path;
+
+import org.jboss.shrinkwrap.api.asset.StringAsset;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.opentelemetry.sdk.trace.data.SpanData;
+import io.quarkus.opentelemetry.deployment.common.exporter.TestSpanExporter;
+import io.quarkus.opentelemetry.deployment.common.exporter.TestSpanExporterProvider;
+import io.quarkus.test.QuarkusDevModeTest;
+import io.restassured.RestAssured;
+
+public class OpenTelemetrySuppressNonAppUriTest {
+
+    @RegisterExtension
+    final static QuarkusDevModeTest TEST = new QuarkusDevModeTest()
+            .withApplicationRoot((jar) -> jar
+                    .addClasses(HelloResource.class, TestSpanExporter.class, TestSpanExporterProvider.class)
+                    .addAsResource(new StringAsset(TestSpanExporterProvider.class.getCanonicalName()),
+                            "META-INF/services/io.opentelemetry.sdk.autoconfigure.spi.traces.ConfigurableSpanExporterProvider")
+                    .add(new StringAsset(
+                            """
+                                    quarkus.otel.traces.exporter=test-span-exporter
+                                    quarkus.otel.metrics.exporter=none
+                                    quarkus.otel.bsp.export.timeout=1s
+                                    quarkus.otel.bsp.schedule.delay=50
+                                    """),
+                            "application.properties"));
+
+    @Test
+    void test() {
+
+        // Must not be traced
+        RestAssured.given()
+                .get("/q/health/")
+                .then()
+                .statusCode(200);
+        RestAssured.given()
+                .get("/q/dev-ui/")
+                .then()
+                .statusCode(200);
+        RestAssured.given()
+                .get("/q/dev-ui/icon/font-awesome.js")
+                .then()
+                .statusCode(200);
+        // Valid trace
+        RestAssured.given()
+                .get("/hello")
+                .then()
+                .statusCode(200);
+        // Get span names
+        List<String> spans = Arrays.asList(
+                RestAssured.given()
+                        .get("/hello/spans")
+                        .then()
+                        .statusCode(200)
+                        .extract().body()
+                        .asString()
+                        .split(";"));
+
+        assertThat(spans).containsExactly("GET /hello");
+    }
+
+    @Path("/hello")
+    public static class HelloResource {
+
+        @Inject
+        TestSpanExporter spanExporter;
+
+        @GET
+        public String greetings() {
+            return "Hello test";
+        }
+
+        /**
+         * Gets a string with the received spans names concatenated by ;
+         *
+         * @return
+         */
+        @GET
+        @Path("/spans")
+        public String greetingsInsertAtLeast() {
+            String spanNames = spanExporter.getFinishedSpanItemsAtLeast(1).stream()
+                    .map(SpanData::getName)
+                    .reduce((s1, s2) -> s1 + ";" + s2).orElse("");
+            System.out.println(spanNames);
+            return spanNames;
+        }
+    }
+}

extensions/opentelemetry/deployment/src/test/java/io/quarkus/opentelemetry/deployment/common/exporter/TestSpanExporter.java

@@ -4,6 +4,7 @@
 import static java.util.stream.Collectors.toList;
 import static org.awaitility.Awaitility.await;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.Collection;
 import java.util.List;
@@ -50,11 +51,27 @@ public List<SpanData> getFinishedSpanItems(int spanCount) {
         return finishedSpanItems.stream().collect(toList());
     }
 
+    /**
+     * Careful when retrieving the list of finished spans. There is a chance when the response is already sent to the
+     * client and Vert.x still writing the end of the spans. This means that a response is available to assert from the
+     * test side but not all spans may be available yet. For this reason, this method requires the number of expected
+     * spans.
+     */
+    public List<SpanData> getFinishedSpanItemsAtLeast(int spanCount) {
+        assertSpanAtLeast(spanCount);
+        return finishedSpanItems;
+    }
+
     public void assertSpanCount(int spanCount) {
-        await().atMost(30, SECONDS).untilAsserted(
+        await().atMost(5, SECONDS).untilAsserted(
                 () -> assertEquals(spanCount, finishedSpanItems.size(), "Spans: " + finishedSpanItems.toString()));
     }
 
+    public void assertSpanAtLeast(int spanCount) {
+        await().atMost(5, SECONDS).untilAsserted(
+                () -> assertTrue(spanCount <= finishedSpanItems.size(), "Spans: " + finishedSpanItems.toString()));
+    }
+
     public void reset() {
         finishedSpanItems.clear();
     }

extensions/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/AutoConfiguredOpenTelemetrySdkBuilderCustomizer.java

@@ -5,8 +5,9 @@
 
 import java.net.InetAddress;
 import java.net.UnknownHostException;
-import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.function.BiFunction;
 import java.util.function.Function;
 import java.util.function.Predicate;
@@ -171,7 +172,7 @@ public Sampler apply(Sampler existingSampler, ConfigProperties configProperties)
                                 .orElse(existingSampler);
 
                         //collect default filtering targets (Needed for all samplers)
-                        List<String> dropTargets = new ArrayList<>();
+                        Set<String> dropTargets = new HashSet<>();
                         if (oTelRuntimeConfig.traces().suppressNonApplicationUris()) {//default is true
                             dropTargets.addAll(TracerRecorder.dropNonApplicationUriTargets);
                         }

extensions/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/tracing/DropTargetsSampler.java

@@ -3,7 +3,10 @@
 import static io.opentelemetry.semconv.UrlAttributes.URL_PATH;
 import static io.opentelemetry.semconv.UrlAttributes.URL_QUERY;
 
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.trace.SpanKind;
@@ -14,12 +17,18 @@
 
 public class DropTargetsSampler implements Sampler {
     private final Sampler sampler;
-    private final List<String> dropTargets;
+    private final Set<String> dropTargetsExact;
+    private final Set<String> dropTargetsWildcard;
 
     public DropTargetsSampler(final Sampler sampler,
-            final List<String> dropTargets) {
+            final Set<String> dropTargets) {
         this.sampler = sampler;
-        this.dropTargets = dropTargets;
+        this.dropTargetsExact = dropTargets.stream().filter(s -> !s.endsWith("*"))
+                .collect(Collectors.toCollection(HashSet::new));
+        this.dropTargetsWildcard = dropTargets.stream()
+                .filter(s -> s.endsWith("*"))
+                .map(s -> s.substring(0, s.length() - 1))
+                .collect(Collectors.toCollection(HashSet::new));
     }
 
     @Override
@@ -48,26 +57,24 @@ private boolean shouldDrop(String target) {
         if ((target == null) || target.isEmpty()) {
             return false;
         }
-        if (safeContains(target)) { // check exact match
+        if (containsExactly(target)) { // check exact match
             return true;
         }
         if (target.charAt(target.length() - 1) == '/') { // check if the path without the ending slash matched
-            if (safeContains(target.substring(0, target.length() - 1))) {
+            if (containsExactly(target.substring(0, target.length() - 1))) {
                 return true;
             }
         }
-        int lastSlashIndex = target.lastIndexOf('/');
-        if (lastSlashIndex != -1) {
-            if (safeContains(target.substring(0, lastSlashIndex) + "*")
-                    || safeContains(target.substring(0, lastSlashIndex) + "/*")) { // check if a wildcard matches
+        for (String dropTargetsWildcard : dropTargetsWildcard) {
+            if (target.startsWith(dropTargetsWildcard)) {
                 return true;
             }
         }
         return false;
     }
 
-    private boolean safeContains(String target) {
-        return dropTargets.contains(target);
+    private boolean containsExactly(String target) {
+        return dropTargetsExact.contains(target);
     }
 
     @Override

extensions/opentelemetry/runtime/src/test/java/io/quarkus/opentelemetry/runtime/tracing/DropTargetsSamplerTest.java

@@ -1,10 +1,10 @@
 package io.quarkus.opentelemetry.runtime.tracing;
 
 import static io.opentelemetry.semconv.UrlAttributes.URL_PATH;
-import static io.quarkus.opentelemetry.runtime.OpenTelemetryUtil.*;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 
 import org.junit.jupiter.api.Test;
@@ -21,7 +21,7 @@ class DropTargetsSamplerTest {
     @Test
     void testDropTargets() {
         CountingSampler countingSampler = new CountingSampler();
-        var sut = new DropTargetsSampler(countingSampler, List.of("/q/swagger-ui", "/q/swagger-ui*"));
+        var sut = new DropTargetsSampler(countingSampler, Set.of("/q/swagger-ui", "/q/swagger-ui*"));
 
         assertEquals(SamplingResult.recordAndSample(), getShouldSample(sut, "/other"));
         assertEquals(1, countingSampler.count.get());
@@ -39,6 +39,33 @@ void testDropTargets() {
         assertEquals(2, countingSampler.count.get());
     }
 
+    @Test
+    void testDropTargetsWildcards() {
+        CountingSampler countingSampler = new CountingSampler();
+        var sut = new DropTargetsSampler(countingSampler, Set.of("/q/dev-ui", "/q/dev-ui/*"));
+
+        assertEquals(SamplingResult.recordAndSample(), getShouldSample(sut, "/other"));
+        assertEquals(1, countingSampler.count.get());
+
+        assertEquals(SamplingResult.recordAndSample(), getShouldSample(sut, "/q/dev-ui-test"));
+        assertEquals(2, countingSampler.count.get());
+
+        assertEquals(SamplingResult.drop(), getShouldSample(sut, "/q/dev-ui"));
+        assertEquals(2, countingSampler.count.get());
+
+        assertEquals(SamplingResult.drop(), getShouldSample(sut, "/q/dev-ui/"));
+        assertEquals(2, countingSampler.count.get());
+
+        assertEquals(SamplingResult.drop(), getShouldSample(sut, "/q/dev-ui/whatever"));
+        assertEquals(2, countingSampler.count.get());
+
+        assertEquals(SamplingResult.drop(), getShouldSample(sut, "/q/dev-ui/whatever/wherever/whenever"));
+        assertEquals(2, countingSampler.count.get());
+
+        assertEquals(SamplingResult.recordAndSample(), getShouldSample(sut, "/q/test"));
+        assertEquals(3, countingSampler.count.get());
+    }
+
     private static SamplingResult getShouldSample(DropTargetsSampler sut, String target) {
         return sut.shouldSample(null, null, null, SpanKind.SERVER,
                 Attributes.of(URL_PATH, target), null);