Stop validating instrument unit (#5279)

api/all/src/main/java/io/opentelemetry/api/internal/ApiUsageLogger.java

@@ -16,12 +16,10 @@
  */
 public final class ApiUsageLogger {
 
-  public static final String LOGGER_NAME = "io.opentelemetry.ApiUsageLogger";
-
-  private static final Logger API_USAGE_LOGGER = Logger.getLogger(LOGGER_NAME);
+  private static final Logger API_USAGE_LOGGER = Logger.getLogger(ApiUsageLogger.class.getName());
 
   /**
-   * Log the {@code message} to the {@link #LOGGER_NAME API Usage Logger}.
+   * Log the {@code message} to the {@link #API_USAGE_LOGGER API Usage Logger}.
    *
    * <p>Log at {@link Level#FINEST} and include a stack trace.
    */
@@ -30,7 +28,7 @@ public static void log(String message) {
   }
 
   /**
-   * Log the {@code message} to the {@link #LOGGER_NAME API Usage Logger}.
+   * Log the {@code message} to the {@link #API_USAGE_LOGGER API Usage Logger}.
    *
    * <p>Log includes a stack trace.
    */

api/all/src/test/java/io/opentelemetry/api/internal/ApiUsageLoggerTest.java

@@ -8,13 +8,15 @@
 import static java.util.logging.Level.WARNING;
 
 import io.github.netmikey.logunit.api.LogCapturer;
+import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
+@SuppressLogger(ApiUsageLogger.class)
 class ApiUsageLoggerTest {
 
   @RegisterExtension
-  LogCapturer apiUsageLogs = LogCapturer.create().captureForLogger(ApiUsageLogger.LOGGER_NAME);
+  LogCapturer apiUsageLogs = LogCapturer.create().captureForLogger(ApiUsageLogger.class.getName());
 
   @Test
   void log() {

api/all/src/test/java/io/opentelemetry/api/metrics/DefaultMeterTest.java

@@ -6,21 +6,16 @@
 package io.opentelemetry.api.metrics;
 
 import static io.opentelemetry.api.common.AttributeKey.stringKey;
-import static io.opentelemetry.api.internal.ApiUsageLogger.LOGGER_NAME;
 
-import io.github.netmikey.logunit.api.LogCapturer;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
 
-@SuppressLogger(loggerName = LOGGER_NAME)
+@SuppressLogger()
 public class DefaultMeterTest {
   private static final Meter METER = DefaultMeter.getInstance();
 
-  @RegisterExtension LogCapturer apiUsageLogs = LogCapturer.create().captureForLogger(LOGGER_NAME);
-
   @Test
   void noopLongCounter_doesNotThrow() {
     LongCounter counter =

exporters/common/src/test/java/io/opentelemetry/exporter/internal/TlsConfigHelperTest.java

@@ -16,6 +16,7 @@
 
 import com.linecorp.armeria.testing.junit5.server.SelfSignedCertificateExtension;
 import io.github.netmikey.logunit.api.LogCapturer;
+import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
 import java.util.concurrent.atomic.AtomicReference;
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLSocketFactory;
@@ -28,6 +29,7 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 
 @ExtendWith(MockitoExtension.class)
+@SuppressLogger(TlsConfigHelper.class)
 class TlsConfigHelperTest {
   @RegisterExtension
   static final SelfSignedCertificateExtension serverTls = new SelfSignedCertificateExtension();

sdk/logs/src/test/java/io/opentelemetry/sdk/logs/SdkLoggerTest.java

@@ -9,7 +9,6 @@
 import static io.opentelemetry.api.common.AttributeKey.doubleArrayKey;
 import static io.opentelemetry.api.common.AttributeKey.longArrayKey;
 import static io.opentelemetry.api.common.AttributeKey.stringArrayKey;
-import static io.opentelemetry.api.internal.ApiUsageLogger.LOGGER_NAME;
 import static io.opentelemetry.sdk.testing.assertj.LogAssertions.assertThat;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
@@ -18,7 +17,6 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
-import io.github.netmikey.logunit.api.LogCapturer;
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
@@ -32,12 +30,9 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
 
 class SdkLoggerTest {
 
-  @RegisterExtension LogCapturer apiUsageLogs = LogCapturer.create().captureForLogger(LOGGER_NAME);
-
   @Test
   void logRecordBuilder() {
     LoggerSharedState state = mock(LoggerSharedState.class);

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

@@ -5,8 +5,6 @@
 
 package io.opentelemetry.sdk.metrics;
 
-import static java.util.logging.Level.WARNING;
-
 import io.opentelemetry.api.metrics.ObservableDoubleMeasurement;
 import io.opentelemetry.api.metrics.ObservableLongMeasurement;
 import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor;
@@ -15,18 +13,14 @@
 import io.opentelemetry.sdk.metrics.internal.state.MeterSharedState;
 import io.opentelemetry.sdk.metrics.internal.state.SdkObservableMeasurement;
 import io.opentelemetry.sdk.metrics.internal.state.WriteableMetricStorage;
-import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.function.BiFunction;
 import java.util.function.Consumer;
-import java.util.logging.Logger;
 
 /** Helper to make implementing builders easier. */
 abstract class AbstractInstrumentBuilder<BuilderT extends AbstractInstrumentBuilder<?>> {
 
   static final String DEFAULT_UNIT = "";
-  public static final String LOGGER_NAME = "io.opentelemetry.sdk.metrics.AbstractInstrumentBuilder";
-  private static final Logger LOGGER = Logger.getLogger(LOGGER_NAME);
 
   private final MeterProviderSharedState meterProviderSharedState;
   private final InstrumentType type;
@@ -57,13 +51,7 @@ abstract class AbstractInstrumentBuilder<BuilderT extends AbstractInstrumentBuil
   protected abstract BuilderT getThis();
 
   public BuilderT setUnit(String unit) {
-    if (!checkValidInstrumentUnit(
-        unit,
-        " Using \"" + DEFAULT_UNIT + "\" for instrument " + this.instrumentName + " instead.")) {
-      this.unit = DEFAULT_UNIT;
-    } else {
-      this.unit = unit;
-    }
+    this.unit = unit;
     return getThis();
   }
 
@@ -120,34 +108,6 @@ public String toString() {
         + "}";
   }
 
-  /** Check if the instrument unit is valid. If invalid, log a warning. */
-  static boolean checkValidInstrumentUnit(String unit) {
-    return checkValidInstrumentUnit(unit, "");
-  }
-
-  /**
-   * Check if the instrument unit is valid. If invalid, log a warning with the {@code logSuffix}
-   * appended. This method should be removed and unit validation should not happen.
-   */
-  static boolean checkValidInstrumentUnit(String unit, String logSuffix) {
-    if (unit != null
-        && !unit.equals("")
-        && unit.length() < 64
-        && StandardCharsets.US_ASCII.newEncoder().canEncode(unit)) {
-      return true;
-    }
-    if (LOGGER.isLoggable(WARNING)) {
-      LOGGER.log(
-          WARNING,
-          "Unit \""
-              + unit
-              + "\" is invalid. Instrument unit must be 63 or fewer ASCII characters."
-              + logSuffix,
-          new AssertionError());
-    }
-    return false;
-  }
-
   @FunctionalInterface
   protected interface SwapBuilder<T> {
     T newBuilder(

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

@@ -5,8 +5,6 @@
 
 package io.opentelemetry.sdk.metrics;
 
-import static io.opentelemetry.sdk.metrics.internal.InstrumentNameValidator.checkValidInstrumentName;
-
 import io.opentelemetry.api.metrics.BatchCallback;
 import io.opentelemetry.api.metrics.DoubleGaugeBuilder;
 import io.opentelemetry.api.metrics.DoubleHistogramBuilder;
@@ -30,17 +28,26 @@
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.regex.Pattern;
 
 /** {@link SdkMeter} is SDK implementation of {@link Meter}. */
 final class SdkMeter implements Meter {
 
   private static final Logger logger = Logger.getLogger(SdkMeter.class.getName());
 
   /**
-   * Message appended to warnings when {@link
-   * InstrumentNameValidator#checkValidInstrumentName(String, String)} is {@code false}.
+   * Instrument names MUST conform to the following syntax.
+   *
+   * <ul>
+   *   <li>They are not null or empty strings.
+   *   <li>They are case-insensitive, ASCII strings.
+   *   <li>The first character must be an alphabetic character.
+   *   <li>Subsequent characters must belong to the alphanumeric characters, '_', '.', and '-'.
+   *   <li>They can have a maximum length of 63 characters.
+   * </ul>
    */
-  private static final String NOOP_INSTRUMENT_WARNING = " Returning noop instrument.";
+  private static final Pattern VALID_INSTRUMENT_NAME_PATTERN =
+      Pattern.compile("([A-Za-z]){1}([A-Za-z0-9\\_\\-\\.]){0,62}");
 
   private static final Meter NOOP_METER = MeterProvider.noop().get("noop");
   private static final String NOOP_INSTRUMENT_NAME = "noop";
@@ -75,31 +82,31 @@ void resetForTest() {
 
   @Override
   public LongCounterBuilder counterBuilder(String name) {
-    return !checkValidInstrumentName(name, NOOP_INSTRUMENT_WARNING)
+    return !checkValidInstrumentName(name)
         ? NOOP_METER.counterBuilder(NOOP_INSTRUMENT_NAME)
         : new SdkLongCounter.SdkLongCounterBuilder(
             meterProviderSharedState, meterSharedState, name);
   }
 
   @Override
   public LongUpDownCounterBuilder upDownCounterBuilder(String name) {
-    return !checkValidInstrumentName(name, NOOP_INSTRUMENT_WARNING)
+    return !checkValidInstrumentName(name)
         ? NOOP_METER.upDownCounterBuilder(NOOP_INSTRUMENT_NAME)
         : new SdkLongUpDownCounter.SdkLongUpDownCounterBuilder(
             meterProviderSharedState, meterSharedState, name);
   }
 
   @Override
   public DoubleHistogramBuilder histogramBuilder(String name) {
-    return !checkValidInstrumentName(name, NOOP_INSTRUMENT_WARNING)
+    return !checkValidInstrumentName(name)
         ? NOOP_METER.histogramBuilder(NOOP_INSTRUMENT_NAME)
         : new SdkDoubleHistogram.SdkDoubleHistogramBuilder(
             meterProviderSharedState, meterSharedState, name);
   }
 
   @Override
   public DoubleGaugeBuilder gaugeBuilder(String name) {
-    return !checkValidInstrumentName(name, NOOP_INSTRUMENT_WARNING)
+    return !checkValidInstrumentName(name)
         ? NOOP_METER.gaugeBuilder(NOOP_INSTRUMENT_NAME)
         : new SdkDoubleGaugeBuilder(meterProviderSharedState, meterSharedState, name);
   }
@@ -143,4 +150,22 @@ public BatchCallback batchCallback(
   public String toString() {
     return "SdkMeter{instrumentationScopeInfo=" + instrumentationScopeInfo + "}";
   }
+
+  /** Check if the instrument name is valid. If invalid, log a warning. */
+  // Visible for testing
+  static boolean checkValidInstrumentName(String name) {
+    if (name != null && VALID_INSTRUMENT_NAME_PATTERN.matcher(name).matches()) {
+      return true;
+    }
+    if (logger.isLoggable(Level.WARNING)) {
+      logger.log(
+          Level.WARNING,
+          "Instrument name \""
+              + name
+              + "\" is invalid, returning noop instrument. Instrument names must consist of 63 or fewer characters including alphanumeric, _, ., -, and start with a letter.",
+          new AssertionError());
+    }
+
+    return false;
+  }
 }