Change Spring starter default otlp protocol from gRPC to http/protobuf (#10212)

jeanbisutti · web-flow · commit 32ea07c1bc6d · 2024-01-11T13:09:17.000-08:00
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpExporterUtil.java b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpExporterUtil.java
@@ -8,14 +8,17 @@
 import io.opentelemetry.exporter.otlp.internal.OtlpConfigUtil;
 import java.time.Duration;
 import java.util.Map;
-import java.util.Objects;
 import java.util.function.BiConsumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 class OtlpExporterUtil {
   private OtlpExporterUtil() {}
 
+  private static final Logger logger = LoggerFactory.getLogger(OtlpExporterUtil.class);
+
   static <G, H, E> E applySignalProperties(
       String dataType,
       OtlpExporterProperties properties,
@@ -39,7 +42,18 @@ static <G, H, E> E applySignalProperties(
     G grpcBuilder = newGrpcBuilder.get();
     H httpBuilder = newHttpBuilder.get();
 
-    boolean isHttpProtobuf = Objects.equals(protocol, OtlpConfigUtil.PROTOCOL_HTTP_PROTOBUF);
+    boolean isHttpProtobuf = !"grpc".equals(protocol);
+
+    if (protocol != null
+        && !OtlpConfigUtil.PROTOCOL_GRPC.equals(protocol)
+        && !OtlpConfigUtil.PROTOCOL_HTTP_PROTOBUF.equals(protocol)) {
+      logger.warn(
+          "Unknown OTLP protocol '"
+              + protocol
+              + "', using '"
+              + OtlpConfigUtil.PROTOCOL_HTTP_PROTOBUF
+              + "'.");
+    }
 
     String endpoint = signalProperties.getEndpoint();
     if (endpoint == null) {
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/MetricExporterAutoConfigurationTest.java b/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/MetricExporterAutoConfigurationTest.java
@@ -8,7 +8,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import io.opentelemetry.exporter.logging.LoggingMetricExporter;
-import io.opentelemetry.exporter.otlp.metrics.OtlpGrpcMetricExporter;
+import io.opentelemetry.exporter.otlp.http.metrics.OtlpHttpMetricExporter;
 import io.opentelemetry.instrumentation.spring.autoconfigure.exporters.logging.LoggingMetricExporterAutoConfiguration;
 import io.opentelemetry.instrumentation.spring.autoconfigure.exporters.otlp.OtlpMetricExporterAutoConfiguration;
 import org.junit.jupiter.api.Test;
@@ -28,7 +28,7 @@ class MetricExporterAutoConfigurationTest {
   void defaultConfiguration() {
     contextRunner.run(
         context -> {
-          assertThat(context.getBean("otelOtlpMetricExporter", OtlpGrpcMetricExporter.class))
+          assertThat(context.getBean("otelOtlpMetricExporter", OtlpHttpMetricExporter.class))
               .as("OTLP exporter is enabled by default")
               .isNotNull();
           assertThat(context.containsBean("otelLoggingMetricExporter"))
@@ -43,7 +43,7 @@ void loggingEnabledByConfiguration() {
         .withPropertyValues("otel.exporter.logging.enabled=true")
         .run(
             context -> {
-              assertThat(context.getBean("otelOtlpMetricExporter", OtlpGrpcMetricExporter.class))
+              assertThat(context.getBean("otelOtlpMetricExporter", OtlpHttpMetricExporter.class))
                   .as("OTLP exporter is present even with logging enabled")
                   .isNotNull();
               assertThat(context.getBean("otelLoggingMetricExporter", LoggingMetricExporter.class))
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/SpanExporterAutoConfigurationTest.java b/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/SpanExporterAutoConfigurationTest.java
@@ -8,7 +8,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import io.opentelemetry.exporter.logging.LoggingSpanExporter;
-import io.opentelemetry.exporter.otlp.trace.OtlpGrpcSpanExporter;
+import io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporter;
 import io.opentelemetry.instrumentation.spring.autoconfigure.exporters.logging.LoggingSpanExporterAutoConfiguration;
 import io.opentelemetry.instrumentation.spring.autoconfigure.exporters.otlp.OtlpSpanExporterAutoConfiguration;
 import org.junit.jupiter.api.Test;
@@ -28,7 +28,7 @@ class SpanExporterAutoConfigurationTest {
   void defaultConfiguration() {
     contextRunner.run(
         context -> {
-          assertThat(context.getBean("otelOtlpSpanExporter", OtlpGrpcSpanExporter.class))
+          assertThat(context.getBean("otelOtlpSpanExporter", OtlpHttpSpanExporter.class))
               .as("OTLP exporter is enabled by default")
               .isNotNull();
           assertThat(context.containsBean("otelLoggingSpanExporter"))
@@ -43,7 +43,7 @@ void loggingEnabledByConfiguration() {
         .withPropertyValues("otel.exporter.logging.enabled=true")
         .run(
             context -> {
-              assertThat(context.getBean("otelOtlpSpanExporter", OtlpGrpcSpanExporter.class))
+              assertThat(context.getBean("otelOtlpSpanExporter", OtlpHttpSpanExporter.class))
                   .as("OTLP exporter is present even with logging enabled")
                   .isNotNull();
               assertThat(context.getBean("otelLoggingSpanExporter", LoggingSpanExporter.class))
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpLogExporterAutoConfigurationTest.java b/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpLogExporterAutoConfigurationTest.java
@@ -7,9 +7,11 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import io.opentelemetry.exporter.otlp.http.logs.OtlpHttpLogRecordExporter;
 import io.opentelemetry.exporter.otlp.logs.OtlpGrpcLogRecordExporter;
 import io.opentelemetry.instrumentation.spring.autoconfigure.OpenTelemetryAutoConfiguration;
 import io.opentelemetry.sdk.logs.export.LogRecordExporter;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.springframework.boot.autoconfigure.AutoConfigurations;
 import org.springframework.boot.test.context.runner.ApplicationContextRunner;
@@ -64,11 +66,37 @@ void otlpLogsDisabled() {
   }
 
   @Test
-  void loggerPresentByDefault() {
+  void otlpHttpUsedByDefault() {
     runner.run(
         context ->
             assertThat(
-                    context.getBean("otelOtlpLogRecordExporter", OtlpGrpcLogRecordExporter.class))
+                    context.getBean("otelOtlpLogRecordExporter", OtlpHttpLogRecordExporter.class))
                 .isNotNull());
   }
+
+  @Test
+  @DisplayName("use grpc when protocol set")
+  void useGrpc() {
+    runner
+        .withPropertyValues("otel.exporter.otlp.protocol=grpc")
+        .run(
+            context ->
+                assertThat(
+                        context.getBean(
+                            "otelOtlpLogRecordExporter", OtlpGrpcLogRecordExporter.class))
+                    .isNotNull());
+  }
+
+  @Test
+  @DisplayName("use http when unknown protocol set")
+  void useHttpWhenAnUnknownProtocolIsSet() {
+    runner
+        .withPropertyValues("otel.exporter.otlp.protocol=unknown")
+        .run(
+            context ->
+                assertThat(
+                        context.getBean(
+                            "otelOtlpLogRecordExporter", OtlpHttpLogRecordExporter.class))
+                    .isNotNull());
+  }
 }
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpMetricExporterAutoConfigurationTest.java b/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpMetricExporterAutoConfigurationTest.java
@@ -7,8 +7,10 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import io.opentelemetry.exporter.otlp.http.metrics.OtlpHttpMetricExporter;
 import io.opentelemetry.exporter.otlp.metrics.OtlpGrpcMetricExporter;
 import io.opentelemetry.instrumentation.spring.autoconfigure.OpenTelemetryAutoConfiguration;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.springframework.boot.autoconfigure.AutoConfigurations;
 import org.springframework.boot.test.context.runner.ApplicationContextRunner;
@@ -25,19 +27,41 @@ class OtlpMetricExporterAutoConfigurationTest {
   void otlpEnabled() {
     runner
         .withPropertyValues("otel.exporter.otlp.enabled=true")
+        .run(
+            context ->
+                assertThat(context.getBean("otelOtlpMetricExporter", OtlpHttpMetricExporter.class))
+                    .isNotNull());
+  }
+
+  @Test
+  @DisplayName("use grpc when protocol set")
+  void useGrpc() {
+    runner
+        .withPropertyValues("otel.exporter.otlp.protocol=grpc")
         .run(
             context ->
                 assertThat(context.getBean("otelOtlpMetricExporter", OtlpGrpcMetricExporter.class))
                     .isNotNull());
   }
 
+  @Test
+  @DisplayName("use http when unknown protocol set")
+  void useHttpWhenAnUnknownProtocolIsSet() {
+    runner
+        .withPropertyValues("otel.exporter.otlp.protocol=unknown")
+        .run(
+            context ->
+                assertThat(context.getBean("otelOtlpMetricExporter", OtlpHttpMetricExporter.class))
+                    .isNotNull());
+  }
+
   @Test
   void otlpMetricsEnabled() {
     runner
         .withPropertyValues("otel.exporter.otlp.metrics.enabled=true")
         .run(
             context ->
-                assertThat(context.getBean("otelOtlpMetricExporter", OtlpGrpcMetricExporter.class))
+                assertThat(context.getBean("otelOtlpMetricExporter", OtlpHttpMetricExporter.class))
                     .isNotNull());
   }
 
@@ -63,10 +87,10 @@ void otlpMetricsDisabled() {
   }
 
   @Test
-  void exporterPresentByDefault() {
+  void otlpHttpUsedByDefault() {
     runner.run(
         context ->
-            assertThat(context.getBean("otelOtlpMetricExporter", OtlpGrpcMetricExporter.class))
+            assertThat(context.getBean("otelOtlpMetricExporter", OtlpHttpMetricExporter.class))
                 .isNotNull());
   }
 }
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpSpanExporterAutoConfigurationTest.java b/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpSpanExporterAutoConfigurationTest.java
@@ -8,6 +8,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import io.opentelemetry.exporter.logging.LoggingSpanExporter;
+import io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporter;
 import io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporterBuilder;
 import io.opentelemetry.exporter.otlp.trace.OtlpGrpcSpanExporter;
 import io.opentelemetry.instrumentation.spring.autoconfigure.MapConverterTestAutoConfiguration;
@@ -31,17 +32,16 @@ class OtlpSpanExporterAutoConfigurationTest {
               AutoConfigurations.of(
                   OpenTelemetryAutoConfiguration.class,
                   OtlpSpanExporterAutoConfiguration.class,
-                  MapConverterTestAutoConfiguration.class))
-          .withBean(OtlpHttpSpanExporterBuilder.class, () -> otlpHttpSpanExporterBuilder);
+                  MapConverterTestAutoConfiguration.class));
 
   @Test
-  @DisplayName("when exporters are ENABLED should initialize OtlpGrpcSpanExporter bean")
+  @DisplayName("when exporters are ENABLED should initialize OtlpHttpSpanExporter bean")
   void otlpEnabled() {
     this.contextRunner
         .withPropertyValues("otel.exporter.otlp.enabled=true")
         .run(
             context ->
-                assertThat(context.getBean("otelOtlpSpanExporter", OtlpGrpcSpanExporter.class))
+                assertThat(context.getBean("otelOtlpSpanExporter", OtlpHttpSpanExporter.class))
                     .isNotNull());
 
     Mockito.verifyNoMoreInteractions(otlpHttpSpanExporterBuilder);
@@ -53,7 +53,7 @@ void otlpTracesEnabled() {
         .withPropertyValues("otel.exporter.otlp.traces.enabled=true")
         .run(
             context ->
-                assertThat(context.getBean("otelOtlpSpanExporter", OtlpGrpcSpanExporter.class))
+                assertThat(context.getBean("otelOtlpSpanExporter", OtlpHttpSpanExporter.class))
                     .isNotNull());
   }
 
@@ -80,18 +80,19 @@ void otlpTracesDisabled() {
   }
 
   @Test
-  @DisplayName("when otlp enabled property is MISSING should initialize OtlpGrpcSpanExporter bean")
+  @DisplayName("when otlp enabled property is MISSING should initialize OtlpHttpSpanExporter bean")
   void exporterPresentByDefault() {
     this.contextRunner.run(
         context ->
-            assertThat(context.getBean("otelOtlpSpanExporter", OtlpGrpcSpanExporter.class))
+            assertThat(context.getBean("otelOtlpSpanExporter", OtlpHttpSpanExporter.class))
                 .isNotNull());
   }
 
   @Test
   @DisplayName("use http/protobuf when protocol set")
   void useHttp() {
     this.contextRunner
+        .withBean(OtlpHttpSpanExporterBuilder.class, () -> otlpHttpSpanExporterBuilder)
         .withPropertyValues(
             "otel.exporter.otlp.enabled=true",
             "otel.exporter.otlp.protocol=http/protobuf",
@@ -113,6 +114,7 @@ void useHttp() {
   @DisplayName("use http/protobuf with environment variables for headers using the MapConverter")
   void useHttpWithEnv() {
     this.contextRunner
+        .withBean(OtlpHttpSpanExporterBuilder.class, () -> otlpHttpSpanExporterBuilder)
         .withPropertyValues(
             "otel.exporter.otlp.enabled=true", "otel.exporter.otlp.protocol=http/protobuf")
         // are similar to environment variables in that they use the same converters
@@ -125,6 +127,30 @@ void useHttpWithEnv() {
     Mockito.verifyNoMoreInteractions(otlpHttpSpanExporterBuilder);
   }
 
+  @Test
+  @DisplayName("use grpc when protocol set")
+  void useGrpc() {
+    this.contextRunner
+        .withPropertyValues("otel.exporter.otlp.protocol=grpc")
+        .run(
+            context ->
+                assertThat(context.getBean(OtlpGrpcSpanExporter.class))
+                    .as("Should contain the gRPC span exporter when grpc is set")
+                    .isNotNull());
+  }
+
+  @Test
+  @DisplayName("use http when unknown protocol set")
+  void useHttpWhenAnUnknownProtocolIsSet() {
+    this.contextRunner
+        .withPropertyValues("otel.exporter.otlp.protocol=unknown")
+        .run(
+            context ->
+                assertThat(context.getBean(OtlpHttpSpanExporter.class))
+                    .as("Should contain the http span exporter when an unknown is set")
+                    .isNotNull());
+  }
+
   @Test
   @DisplayName("logging exporter can still be configured")
   void loggingExporter() {
