Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop validating instrument unit #5279

Merged
merged 1 commit into from
Mar 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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";
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
}

This file was deleted.

Loading