Skip to content

Commit

Permalink
Drop NaN measurements to metric instruments (#5859)
Browse files Browse the repository at this point in the history
  • Loading branch information
jack-berg authored Oct 2, 2023
1 parent bb4c219 commit 7e67a84
Show file tree
Hide file tree
Showing 9 changed files with 147 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,16 @@ public void recordLong(long value, Attributes attributes, Context context) {

@Override
public void recordDouble(double value, Attributes attributes, Context context) {
if (Double.isNaN(value)) {
logger.log(
Level.FINE,
"Instrument "
+ metricDescriptor.getSourceInstrument().getName()
+ " has recorded measurement Not-a-Number (NaN) value with attributes "
+ attributes
+ ". Dropping measurement.");
return;
}
AggregatorHandle<T, U> handle = getAggregatorHandle(attributes, context);
handle.recordDouble(value, attributes, context);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,16 @@ public void record(double value, Attributes attributes) {
logNoActiveReader();
return;
}
if (Double.isNaN(value)) {
logger.log(
Level.FINE,
"Instrument "
+ instrumentDescriptor.getName()
+ " has recorded measurement Not-a-Number (NaN) value with attributes "
+ attributes
+ ". Dropping measurement.");
return;
}

Measurement measurement;
MemoryMode memoryMode = activeReader.getReader().getMemoryMode();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.metrics.internal.state.DefaultSynchronousMetricStorage;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import io.opentelemetry.sdk.testing.time.TestClock;
Expand Down Expand Up @@ -166,6 +167,14 @@ void doubleCounterAdd_Monotonicity() {
"Counters can only increase. Instrument testCounter has recorded a negative value.");
}

@Test
@SuppressLogger(DefaultSynchronousMetricStorage.class)
void doubleCounterAdd_NaN() {
DoubleCounter doubleCounter = sdkMeter.counterBuilder("testCounter").ofDoubles().build();
doubleCounter.add(Double.NaN);
assertThat(sdkMeterReader.collectAllMetrics()).hasSize(0);
}

@Test
void stressTest() {
DoubleCounter doubleCounter = sdkMeter.counterBuilder("testCounter").ofDoubles().build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
import io.opentelemetry.api.metrics.ObservableDoubleGauge;
import io.opentelemetry.extension.incubator.metrics.DoubleGauge;
import io.opentelemetry.extension.incubator.metrics.ExtendedDoubleGaugeBuilder;
import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.metrics.internal.state.DefaultSynchronousMetricStorage;
import io.opentelemetry.sdk.metrics.internal.state.SdkObservableMeasurement;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import io.opentelemetry.sdk.testing.time.TestClock;
import java.time.Duration;
import java.util.stream.IntStream;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

/** Unit tests for {@link SdkDoubleGauge}. */
Expand Down Expand Up @@ -54,12 +56,20 @@ void set_PreventNullAttributes() {
.hasMessage("attributes");
}

@Test
@SuppressLogger(DefaultSynchronousMetricStorage.class)
void set_NaN() {
DoubleGauge gauge = ((ExtendedDoubleGaugeBuilder) sdkMeter.gaugeBuilder("testGauge")).build();
gauge.set(Double.NaN);
assertThat(cumulativeReader.collectAllMetrics()).hasSize(0);
}

@Test
void observable_RemoveCallback() {
ObservableDoubleGauge gauge =
sdkMeter.gaugeBuilder("testGauge").buildWithCallback(measurement -> measurement.record(10));

Assertions.assertThat(cumulativeReader.collectAllMetrics())
assertThat(cumulativeReader.collectAllMetrics())
.satisfiesExactly(
metric ->
assertThat(metric)
Expand All @@ -69,7 +79,16 @@ void observable_RemoveCallback() {

gauge.close();

Assertions.assertThat(cumulativeReader.collectAllMetrics()).hasSize(0);
assertThat(cumulativeReader.collectAllMetrics()).hasSize(0);
}

@Test
@SuppressLogger(SdkObservableMeasurement.class)
void observable_NaN() {
sdkMeter
.gaugeBuilder("testGauge")
.buildWithCallback(measurement -> measurement.record(Double.NaN));
assertThat(cumulativeReader.collectAllMetrics()).hasSize(0);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.metrics.internal.state.DefaultSynchronousMetricStorage;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import io.opentelemetry.sdk.testing.time.TestClock;
Expand Down Expand Up @@ -266,6 +267,14 @@ void doubleHistogramRecord_NonNegativeCheck() {
"Histograms can only record non-negative values. Instrument testHistogram has recorded a negative value.");
}

@Test
@SuppressLogger(DefaultSynchronousMetricStorage.class)
void doubleHistogramRecord_NaN() {
DoubleHistogram histogram = sdkMeter.histogramBuilder("testHistogram").build();
histogram.record(Double.NaN);
assertThat(sdkMeterReader.collectAllMetrics()).hasSize(0);
}

@Test
void stressTest() {
DoubleHistogram doubleHistogram = sdkMeter.histogramBuilder("testHistogram").build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@
import static io.opentelemetry.api.common.AttributeKey.stringKey;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;
import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.metrics.ObservableDoubleCounter;
import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.metrics.internal.state.SdkObservableMeasurement;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import io.opentelemetry.sdk.testing.time.TestClock;
Expand Down Expand Up @@ -53,6 +56,20 @@ void removeCallback() {
assertThat(sdkMeterReader.collectAllMetrics()).hasSize(0);
}

@Test
@SuppressLogger(SdkObservableMeasurement.class)
void observable_NaN() {
InMemoryMetricReader sdkMeterReader = InMemoryMetricReader.create();
SdkMeterProvider sdkMeterProvider =
sdkMeterProviderBuilder.registerMetricReader(sdkMeterReader).build();
sdkMeterProvider
.get(getClass().getName())
.counterBuilder("testObserver")
.ofDoubles()
.buildWithCallback(measurement -> measurement.record(Double.NaN));
assertThat(sdkMeterReader.collectAllMetrics()).hasSize(0);
}

@Test
void collectMetrics_NoRecords() {
InMemoryMetricReader sdkMeterReader = InMemoryMetricReader.create();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@
import static io.opentelemetry.api.common.AttributeKey.stringKey;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.mock;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.metrics.ObservableDoubleUpDownCounter;
import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.metrics.internal.state.SdkObservableMeasurement;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import io.opentelemetry.sdk.testing.time.TestClock;
Expand Down Expand Up @@ -55,6 +58,20 @@ void removeCallback() {
assertThat(sdkMeterReader.collectAllMetrics()).hasSize(0);
}

@Test
@SuppressLogger(SdkObservableMeasurement.class)
void observable_NaN() {
InMemoryMetricReader sdkMeterReader = InMemoryMetricReader.create();
SdkMeterProvider sdkMeterProvider =
sdkMeterProviderBuilder.registerMetricReader(sdkMeterReader).build();
sdkMeterProvider
.get(getClass().getName())
.upDownCounterBuilder("testCounter")
.ofDoubles()
.buildWithCallback(measurement -> measurement.record(Double.NaN));
assertThat(sdkMeterReader.collectAllMetrics()).hasSize(0);
}

@Test
void collectMetrics_NoRecords() {
InMemoryMetricReader sdkMeterReader = InMemoryMetricReader.create();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@

import static io.opentelemetry.sdk.metrics.data.AggregationTemporality.CUMULATIVE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import io.github.netmikey.logunit.api.LogCapturer;
import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.common.export.MemoryMode;
import io.opentelemetry.sdk.metrics.InstrumentType;
Expand All @@ -21,12 +25,18 @@
import io.opentelemetry.sdk.metrics.internal.export.RegisteredReader;
import io.opentelemetry.sdk.metrics.internal.view.ViewRegistry;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import org.assertj.core.util.Lists;
import java.util.Arrays;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.mockito.ArgumentCaptor;
import org.slf4j.event.Level;

@SuppressWarnings("rawtypes")
public class SdkObservableMeasurementTest {
class SdkObservableMeasurementTest {

@RegisterExtension
final LogCapturer logs =
LogCapturer.create().captureForLogger(SdkObservableMeasurement.class.getName(), Level.DEBUG);

private AsynchronousMetricStorage mockAsyncStorage1;
private RegisteredReader registeredReader1;
Expand Down Expand Up @@ -66,11 +76,11 @@ private void setup(MemoryMode memoryMode) {
SdkObservableMeasurement.create(
instrumentationScopeInfo,
instrumentDescriptor,
Lists.newArrayList(mockAsyncStorage1, mockAsyncStorage2));
Arrays.asList(mockAsyncStorage1, mockAsyncStorage2));
}

@Test
public void testRecordLongImmutableData() {
void recordLong_ImmutableData() {
setup(MemoryMode.IMMUTABLE_DATA);

sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
Expand All @@ -90,7 +100,7 @@ public void testRecordLongImmutableData() {
}

@Test
public void testRecordDoubleReturnImmutableData() {
void recordDouble_ImmutableData() {
setup(MemoryMode.IMMUTABLE_DATA);

sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
Expand All @@ -110,7 +120,7 @@ public void testRecordDoubleReturnImmutableData() {
}

@Test
public void testRecordDoubleReturnReusableData() {
void recordDouble_ReusableData() {
setup(MemoryMode.REUSABLE_DATA);

sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
Expand Down Expand Up @@ -142,7 +152,7 @@ public void testRecordDoubleReturnReusableData() {
}

@Test
public void testRecordLongReturnReusableData() {
void recordLong_ReusableData() {
setup(MemoryMode.REUSABLE_DATA);

sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
Expand Down Expand Up @@ -172,4 +182,16 @@ public void testRecordLongReturnReusableData() {
sdkObservableMeasurement.unsetActiveReader();
}
}

@Test
@SuppressLogger(SdkObservableMeasurement.class)
void recordDouble_NaN() {
setup(MemoryMode.REUSABLE_DATA);
sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
sdkObservableMeasurement.record(Double.NaN);

verify(mockAsyncStorage1, never()).record(any());
logs.assertContains(
"Instrument testCounter has recorded measurement Not-a-Number (NaN) value with attributes {}. Dropping measurement.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand All @@ -25,6 +26,7 @@
import io.opentelemetry.sdk.metrics.data.MetricData;
import io.opentelemetry.sdk.metrics.internal.aggregator.Aggregator;
import io.opentelemetry.sdk.metrics.internal.aggregator.AggregatorFactory;
import io.opentelemetry.sdk.metrics.internal.aggregator.EmptyMetricData;
import io.opentelemetry.sdk.metrics.internal.descriptor.Advice;
import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor;
import io.opentelemetry.sdk.metrics.internal.descriptor.MetricDescriptor;
Expand All @@ -37,6 +39,7 @@
import io.opentelemetry.sdk.testing.time.TestClock;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.slf4j.event.Level;

@SuppressLogger(DefaultSynchronousMetricStorage.class)
public class SynchronousMetricStorageTest {
Expand All @@ -56,7 +59,8 @@ public class SynchronousMetricStorageTest {
private static final int CARDINALITY_LIMIT = 25;

@RegisterExtension
LogCapturer logs = LogCapturer.create().captureForType(DefaultSynchronousMetricStorage.class);
LogCapturer logs =
LogCapturer.create().captureForType(DefaultSynchronousMetricStorage.class, Level.DEBUG);

private final RegisteredReader deltaReader =
RegisteredReader.create(InMemoryMetricReader.createDelta(), ViewRegistry.create());
Expand All @@ -69,6 +73,25 @@ public class SynchronousMetricStorageTest {
.createAggregator(DESCRIPTOR, ExemplarFilter.alwaysOff()));
private final AttributesProcessor attributesProcessor = AttributesProcessor.noop();

@Test
void recordDouble_NaN() {
DefaultSynchronousMetricStorage<?, ?> storage =
new DefaultSynchronousMetricStorage<>(
cumulativeReader,
METRIC_DESCRIPTOR,
aggregator,
attributesProcessor,
CARDINALITY_LIMIT);

storage.recordDouble(Double.NaN, Attributes.empty(), Context.current());

logs.assertContains(
"Instrument name has recorded measurement Not-a-Number (NaN) value with attributes {}. Dropping measurement.");
verify(aggregator, never()).createHandle();
assertThat(storage.collect(RESOURCE, INSTRUMENTATION_SCOPE_INFO, 0, 10))
.isEqualTo(EmptyMetricData.getInstance());
}

@Test
void attributesProcessor_applied() {
Attributes attributes = Attributes.builder().put("K", "V").build();
Expand Down

0 comments on commit 7e67a84

Please sign in to comment.