From 16037ddd85adfa5712f11a341773627b6a44226b Mon Sep 17 00:00:00 2001 From: Marius Volkhart Date: Fri, 30 Oct 2020 15:11:51 +0100 Subject: [PATCH] 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 https://github.com/open-telemetry/opentelemetry-specification/issues/586#issuecomment-669856711 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 --- .../sdk/metrics/MeterSdkProvider.java | 14 ++++++++++++-- .../sdk/metrics/MeterSdkRegistryTest.java | 18 ++++++++++++++++++ .../sdk/trace/TracerSdkProvider.java | 12 ++++++++++-- .../sdk/trace/TracerSdkProviderTest.java | 18 ++++++++++++++++++ 4 files changed, 58 insertions(+), 4 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/MeterSdkProvider.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/MeterSdkProvider.java index dae53e29244..921d3eef866 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/MeterSdkProvider.java +++ b/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()); + private static final String defaultMeterName = "unknown"; private final MeterSdkComponentRegistry registry; private final MetricProducer metricProducer; @@ -41,11 +45,17 @@ 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. See commit + // message for further details. + if (instrumentationName == null || instrumentationName.isEmpty()) { + logger.warning("Meter requested without instrumentation name."); + instrumentationName = defaultMeterName; + } return registry.get(instrumentationName, instrumentationVersion); } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MeterSdkRegistryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MeterSdkRegistryTest.java index fa7dabaa247..4c665f4a0fc 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MeterSdkRegistryTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MeterSdkRegistryTest.java @@ -101,4 +101,22 @@ 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("unknown"); + + meter = meterProvider.get(null, null); + assertThat(meter.getInstrumentationLibraryInfo().getName()).isEqualTo("unknown"); + } + + @Test + void suppliesDefaultMeterForEmptyName() { + MeterSdk meter = meterProvider.get(""); + assertThat(meter.getInstrumentationLibraryInfo().getName()).isEqualTo("unknown"); + + meter = meterProvider.get("", ""); + assertThat(meter.getInstrumentationLibraryInfo().getName()).isEqualTo("unknown"); + } } diff --git a/sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/TracerSdkProvider.java b/sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/TracerSdkProvider.java index 2e9dcce14b1..fb66bf3aea4 100644 --- a/sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/TracerSdkProvider.java +++ b/sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/TracerSdkProvider.java @@ -15,6 +15,7 @@ import io.opentelemetry.sdk.internal.MillisClock; import io.opentelemetry.sdk.resources.Resource; import io.opentelemetry.sdk.trace.config.TraceConfig; +import javax.annotation.Nullable; import java.util.Objects; import java.util.logging.Level; import java.util.logging.Logger; @@ -28,6 +29,7 @@ */ public class TracerSdkProvider implements TracerProvider, TracerSdkManagement { private static final Logger logger = Logger.getLogger(TracerSdkProvider.class.getName()); + private static final String defaultTracerName = "unknown"; private final TracerSharedState sharedState; private final TracerSdkComponentRegistry tracerSdkComponentRegistry; @@ -47,11 +49,17 @@ private TracerSdkProvider(Clock clock, IdsGenerator idsGenerator, Resource resou @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. See commit + // message for further details. + if (instrumentationName == null || instrumentationName.isEmpty()) { + logger.warning("Tracer requested without instrumentation name."); + instrumentationName = defaultTracerName; + } return tracerSdkComponentRegistry.get(instrumentationName, instrumentationVersion); } diff --git a/sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/TracerSdkProviderTest.java b/sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/TracerSdkProviderTest.java index 7885b8b298c..84542d8f865 100644 --- a/sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/TracerSdkProviderTest.java +++ b/sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/TracerSdkProviderTest.java @@ -133,4 +133,22 @@ void returnNoopSpanAfterShutdown() { assertThat(span.getSpanContext().isValid()).isFalse(); span.end(); } + + @Test + void suppliesDefaultTracerForNullName() { + TracerSdk tracer = (TracerSdk) tracerFactory.get(null); + assertThat(tracer.getInstrumentationLibraryInfo().getName()).isEqualTo("unknown"); + + tracer = (TracerSdk) tracerFactory.get(null, null); + assertThat(tracer.getInstrumentationLibraryInfo().getName()).isEqualTo("unknown"); + } + + @Test + void suppliesDefaultTracerForEmptyName() { + TracerSdk tracer = (TracerSdk) tracerFactory.get(""); + assertThat(tracer.getInstrumentationLibraryInfo().getName()).isEqualTo("unknown"); + + tracer = (TracerSdk) tracerFactory.get("", ""); + assertThat(tracer.getInstrumentationLibraryInfo().getName()).isEqualTo("unknown"); + } }