Prevent SDK from throwing on null instrumentation name (#1941)

CHANGELOG: the SDK will no longer throw an exception with a null instrumentation name on tracers or meters, but this is still not recommended usage.

* Provide a default Tracer and Meter for invalid instrumentation names

According to the spec, https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#tracerprovider-operations both a null and empty instrumentation name are invalid and a default implementation should be provided.

However, the spec is vague on what default means. See open-telemetry/opentelemetry-specification#586 (comment)

This change interprets "default" as meaning, "it should still work, but it's not ideal". Accordingly, a real Tracer or Meter implementation from the SDK is used. It meets the requirements of exporters and such by using a valid instrumentation name. It meets the requirements of application developers by producing valid tracing data. It also warns library and application developers by logging at the WARN level that an invalid value has been given.

The rational for this interpretation is that things should always work for the application developer. If an app developer doesn't provide a name, they should be chided, but they are instrumenting their own app, and likely don't care about the lack of name. If an app developer incorporates a library that doesn't specify the instrumentation name, the instrumentation should still be made available to application devs, but they should be able to turn if off (hence a valid name) and be made aware of something not being right (hence the logging) so they can report it to the library maintainers.

Fies #1879

* Mark instrumentation version nullable in MeterProvider and TracerProvider

The spec dictates this to be optional, and the implementations treat it as nullable already.

* Remove ComponentRegistry#get(String) overload

This overload is not no longer used by our code.

* fixup! Mark instrumentation version nullable in MeterProvider and TracerProvider

* fixup! Provide a default Tracer and Meter for invalid instrumentation names

* fixup! Provide a default Tracer and Meter for invalid instrumentation names

* fixup! Remove ComponentRegistry#get(String) overload

This reverts commit db56814

* Revert "Mark instrumentation version nullable in MeterProvider and TracerProvider"

This reverts commit 7e92b39

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/MeterSdkProvider.java

@@ -19,7 +19,9 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
+import java.util.logging.Logger;
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 /**
  * {@code Meter} provider implementation for {@link MeterProvider}.
@@ -29,6 +31,8 @@
  */
 public final class MeterSdkProvider implements MeterProvider {
 
+  private static final Logger LOGGER = Logger.getLogger(MeterSdkProvider.class.getName());
+  static final String DEFAULT_METER_NAME = "unknown";
   private final MeterSdkComponentRegistry registry;
   private final MetricProducer metricProducer;
 
@@ -41,11 +45,16 @@ private MeterSdkProvider(Clock clock, Resource resource) {
 
   @Override
   public MeterSdk get(String instrumentationName) {
-    return registry.get(instrumentationName);
+    return get(instrumentationName, null);
   }
 
   @Override
-  public MeterSdk get(String instrumentationName, String instrumentationVersion) {
+  public MeterSdk get(String instrumentationName, @Nullable String instrumentationVersion) {
+    // Per the spec, both null and empty are "invalid" and a "default" should be used.
+    if (instrumentationName == null || instrumentationName.isEmpty()) {
+      LOGGER.fine("Meter requested without instrumentation name.");
+      instrumentationName = DEFAULT_METER_NAME;
+    }
     return registry.get(instrumentationName, instrumentationVersion);
   }
 

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MeterSdkRegistryTest.java

@@ -101,4 +101,26 @@ void metricProducer_GetAllMetrics() {
                 Collections.singletonList(
                     LongPoint.create(testClock.now(), testClock.now(), Labels.empty(), 10))));
   }
+
+  @Test
+  void suppliesDefaultMeterForNullName() {
+    MeterSdk meter = meterProvider.get(null);
+    assertThat(meter.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(MeterSdkProvider.DEFAULT_METER_NAME);
+
+    meter = meterProvider.get(null, null);
+    assertThat(meter.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(MeterSdkProvider.DEFAULT_METER_NAME);
+  }
+
+  @Test
+  void suppliesDefaultMeterForEmptyName() {
+    MeterSdk meter = meterProvider.get("");
+    assertThat(meter.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(MeterSdkProvider.DEFAULT_METER_NAME);
+
+    meter = meterProvider.get("", "");
+    assertThat(meter.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(MeterSdkProvider.DEFAULT_METER_NAME);
+  }
 }

sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/TracerSdkProvider.java

@@ -18,6 +18,7 @@
 import java.util.Objects;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import javax.annotation.Nullable;
 
 /**
  * {@code Tracer} provider implementation for {@link TracerProvider}.
@@ -28,6 +29,7 @@
  */
 public class TracerSdkProvider implements TracerProvider, TracerSdkManagement {
   private static final Logger logger = Logger.getLogger(TracerSdkProvider.class.getName());
+  static final String DEFAULT_TRACER_NAME = "unknown";
   private final TracerSharedState sharedState;
   private final TracerSdkComponentRegistry tracerSdkComponentRegistry;
 
@@ -47,11 +49,16 @@ private TracerSdkProvider(Clock clock, IdGenerator idsGenerator, Resource resour
 
   @Override
   public Tracer get(String instrumentationName) {
-    return tracerSdkComponentRegistry.get(instrumentationName);
+    return get(instrumentationName, null);
   }
 
   @Override
-  public Tracer get(String instrumentationName, String instrumentationVersion) {
+  public Tracer get(String instrumentationName, @Nullable String instrumentationVersion) {
+    // Per the spec, both null and empty are "invalid" and a "default" should be used.
+    if (instrumentationName == null || instrumentationName.isEmpty()) {
+      logger.fine("Tracer requested without instrumentation name.");
+      instrumentationName = DEFAULT_TRACER_NAME;
+    }
     return tracerSdkComponentRegistry.get(instrumentationName, instrumentationVersion);
   }
 

sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/TracerSdkProviderTest.java

@@ -134,4 +134,26 @@ void returnNoopSpanAfterShutdown() {
     assertThat(span.getSpanContext().isValid()).isFalse();
     span.end();
   }
+
+  @Test
+  void suppliesDefaultTracerForNullName() {
+    TracerSdk tracer = (TracerSdk) tracerFactory.get(null);
+    assertThat(tracer.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(TracerSdkProvider.DEFAULT_TRACER_NAME);
+
+    tracer = (TracerSdk) tracerFactory.get(null, null);
+    assertThat(tracer.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(TracerSdkProvider.DEFAULT_TRACER_NAME);
+  }
+
+  @Test
+  void suppliesDefaultTracerForEmptyName() {
+    TracerSdk tracer = (TracerSdk) tracerFactory.get("");
+    assertThat(tracer.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(TracerSdkProvider.DEFAULT_TRACER_NAME);
+
+    tracer = (TracerSdk) tracerFactory.get("", "");
+    assertThat(tracer.getInstrumentationLibraryInfo().getName())
+        .isEqualTo(TracerSdkProvider.DEFAULT_TRACER_NAME);
+  }
 }