Merge pull request #45689 from holly-cummins/better-support-lgtm-cont…

…ainer-reuse Fix incomplete config on re-used LGTM containers
quarkusio · Jan 21, 2025 · 174290c · 174290c
2 parents c2fe311 + 8450e15
commit 174290c
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 28 deletions.
diff --git a/...s/testcontainers/src/main/java/io/quarkus/observability/testcontainers/LgtmContainer.java b/...s/testcontainers/src/main/java/io/quarkus/observability/testcontainers/LgtmContainer.java
@@ -120,18 +120,22 @@ public String getOtlpProtocol() {
     }
 
     public int getOtlpPort() {
-        int port = getOtlpPortInternal();
+        int port = getPrivateOtlpPort();
         return getMappedPort(port);
     }
 
-    private int getOtlpPortInternal() {
+    private int getPrivateOtlpPort() {
+        return getPrivateOtlpPort(getOtlpProtocol());
+    }
+
+    public static int getPrivateOtlpPort(String otlpProtocol) {
         // use ignore-case here; grpc == gRPC
-        if (ContainerConstants.OTEL_GRPC_PROTOCOL.equalsIgnoreCase(getOtlpProtocol())) {
+        if (ContainerConstants.OTEL_GRPC_PROTOCOL.equalsIgnoreCase(otlpProtocol)) {
             return ContainerConstants.OTEL_GRPC_EXPORTER_PORT;
-        } else if (ContainerConstants.OTEL_HTTP_PROTOCOL.equals(getOtlpProtocol())) {
+        } else if (ContainerConstants.OTEL_HTTP_PROTOCOL.equals(otlpProtocol)) {
             return ContainerConstants.OTEL_HTTP_EXPORTER_PORT;
         } else {
-            throw new IllegalArgumentException("Unsupported OTEL protocol: " + getOtlpProtocol());
+            throw new IllegalArgumentException("Unsupported OTEL protocol: " + otlpProtocol);
         }
     }
 

diff --git a/...ommon/src/main/java/io/quarkus/observability/devresource/DevResourceLifecycleManager.java b/...ommon/src/main/java/io/quarkus/observability/devresource/DevResourceLifecycleManager.java
@@ -75,8 +75,8 @@ default Container<T> container(T config, ModulesConfiguration root) {
     }
 
     /**
-     * Deduct current config from params.
-     * If port are too dynamic / configured, it's hard to deduct,
+     * Deduce current config from params.
+     * If port are too dynamic / configured, it's hard to deduce,
      * since configuration is not part of the devservice state.
      * e.g. different ports then usual - Grafana UI is 3000, if you do not use 3000,
      * it's hard or impossible to know which port belongs to certain property.

diff --git a/...evresource-lgtm/src/main/java/io/quarkus/observability/devresource/lgtm/LgtmResource.java b/...evresource-lgtm/src/main/java/io/quarkus/observability/devresource/lgtm/LgtmResource.java
@@ -15,10 +15,13 @@
 public class LgtmResource extends ContainerResource<LgtmContainer, LgtmConfig> {
 
     private ExtensionsCatalog catalog;
+    private LgtmConfig config;
 
     @Override
     public LgtmConfig config(ModulesConfiguration configuration) {
-        return configuration.lgtm();
+        LgtmConfig config = configuration.lgtm();
+        this.config = config;
+        return config;
     }
 
     @Override
@@ -32,17 +35,53 @@ public Container<LgtmConfig> container(LgtmConfig config, ModulesConfiguration r
         return set(new LgtmContainer(config));
     }
 
-    // FIXME consolidate config methods.
+    private int getPrivateOtlpPort() {
+        if (config != null) {
+            return LgtmContainer.getPrivateOtlpPort(config.otlpProtocol());
+        } else {
+            return -1;
+        }
+    }
+
+    private Map<String, String> config(int privatePort, String host) {
+        return config(privatePort, host, container.getMappedPort(privatePort));
+    }
+
     @Override
     public Map<String, String> config(int privatePort, String host, int publicPort) {
+
+        Map<String, String> containerConfigs = new HashMap<>();
+
         switch (privatePort) {
             case ContainerConstants.GRAFANA_PORT:
-                return Map.of("grafana.endpoint", String.format("http://%s:%s", host, publicPort));
-            case ContainerConstants.OTEL_GRPC_EXPORTER_PORT:
+                containerConfigs.put("grafana.endpoint", String.format("http://%s:%s", host, publicPort));
+                break;
             case ContainerConstants.OTEL_HTTP_EXPORTER_PORT:
-                return Map.of("otel-collector.url", String.format("%s:%s", host, publicPort));
+                if (catalog != null && catalog.hasMicrometerOtlp()) {
+
+                    containerConfigs.put("quarkus.micrometer.export.otlp.url",
+                            String.format("http://%s:%s/v1/metrics", host,
+                                    publicPort));
+                }
+                // No break, fall through
+            case ContainerConstants.OTEL_GRPC_EXPORTER_PORT:
+                containerConfigs.put("otel-collector.url", String.format("%s:%s", host, publicPort));
+                break;
+        }
+
+        // The OTLP port is probably one of the ports we already compared against, but at compile-time we don't know which one,
+        // so instead of doing this check as a fallthrough on the switch, do a normal if-check
+        if (catalog != null && catalog.hasOpenTelemetry()) {
+            final int privateOtlpPort = getPrivateOtlpPort();
+            if (privateOtlpPort == privatePort) {
+                containerConfigs.put("quarkus.otel.exporter.otlp.endpoint",
+                        String.format("http://%s:%s", host, publicPort));
+                String otlpProtocol = config.otlpProtocol(); // If we got to this stage, config must be not null
+                containerConfigs.put("quarkus.otel.exporter.otlp.protocol", otlpProtocol);
+            }
+
         }
-        return Map.of();
+        return containerConfigs;
     }
 
     @Override
@@ -53,23 +92,13 @@ protected LgtmContainer defaultContainer() {
     @Override
     public Map<String, String> doStart() {
         String host = container.getHost();
-        int otlpPort = container.getOtlpPort();
-
-        //Set non Quarkus properties for convenience and testing.
         Map<String, String> containerConfigs = new HashMap<>();
-        containerConfigs.put("grafana.endpoint", String.format("http://%s:%s", host, container.getGrafanaPort()));
-        containerConfigs.put("otel-collector.url", String.format("%s:%s", host, otlpPort));
 
-        // set relevant properties for Quarkus extensions directly
-        if (catalog != null && catalog.hasOpenTelemetry()) {
-            containerConfigs.put("quarkus.otel.exporter.otlp.endpoint", String.format("http://%s:%s", host, otlpPort));
-            containerConfigs.put("quarkus.otel.exporter.otlp.protocol", container.getOtlpProtocol());
-        }
-        if (catalog != null && catalog.hasMicrometerOtlp()) {
-            // always use http -- as that's what Micrometer supports
-            containerConfigs.put("quarkus.micrometer.export.otlp.url",
-                    String.format("http://%s:%s/v1/metrics", host,
-                            container.getMappedPort(ContainerConstants.OTEL_HTTP_EXPORTER_PORT)));
+        containerConfigs.putAll(config(ContainerConstants.GRAFANA_PORT, host));
+        containerConfigs.putAll(config(ContainerConstants.OTEL_HTTP_EXPORTER_PORT, host));
+        // Iff GRPC is the OTLP protocol, overwrite the otel-collector.url we just wrote with the correct grpc one, and set up the otlp endpoints
+        if (ContainerConstants.OTEL_GRPC_PROTOCOL.equals(container.getOtlpProtocol())) {
+            containerConfigs.putAll(config(ContainerConstants.OTEL_GRPC_EXPORTER_PORT, host));
         }
         return containerConfigs;
     }