Drop NaN measurements to metric instruments (#5859)

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DefaultSynchronousMetricStorage.java

@@ -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);
   }

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/SdkObservableMeasurement.java

@@ -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();

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

@@ -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;
@@ -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();

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

@@ -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}. */
@@ -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)
@@ -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

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

@@ -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;
@@ -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();

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

@@ -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;
@@ -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();

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

@@ -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;
@@ -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();

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/state/SdkObservableMeasurementTest.java

@@ -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;
@@ -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;
@@ -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);
@@ -90,7 +100,7 @@ public void testRecordLongImmutableData() {
   }
 
   @Test
-  public void testRecordDoubleReturnImmutableData() {
+  void recordDouble_ImmutableData() {
     setup(MemoryMode.IMMUTABLE_DATA);
 
     sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
@@ -110,7 +120,7 @@ public void testRecordDoubleReturnImmutableData() {
   }
 
   @Test
-  public void testRecordDoubleReturnReusableData() {
+  void recordDouble_ReusableData() {
     setup(MemoryMode.REUSABLE_DATA);
 
     sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
@@ -142,7 +152,7 @@ public void testRecordDoubleReturnReusableData() {
   }
 
   @Test
-  public void testRecordLongReturnReusableData() {
+  void recordLong_ReusableData() {
     setup(MemoryMode.REUSABLE_DATA);
 
     sdkObservableMeasurement.setActiveReader(registeredReader1, 0, 10);
@@ -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.");
+  }
 }

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/state/SynchronousMetricStorageTest.java

@@ -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;
@@ -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;
@@ -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 {
@@ -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());
@@ -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();