From 62b65fab92798572accb1f8a1446bf8245dc0dc1 Mon Sep 17 00:00:00 2001
From: Lalit Kumar Bhasin <labhas@microsoft.com>
Date: Thu, 2 Jun 2022 20:30:15 -0700
Subject: [PATCH] [Metrics SDK] Only record non-negative / finite / Non-NAN
 histogram values (#1427)

* validate histogram value

* handle Nan

* add changelog

* divide by 0 error on windows

* fix markdown lint
---
 CHANGELOG.md                              |  2 ++
 sdk/src/metrics/sync_instruments.cc       | 32 +++++++++++++++++++++++
 sdk/test/metrics/sync_instruments_test.cc | 10 +++++--
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5a260fe8be..aa491b1afb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,8 @@ Increment the:
 
 ## [Unreleased]
 
+* [METRICS] Only record non-negative / finite / Non-NAN histogram values([#1427](https://github.com/open-telemetry/opentelemetry-cpp/pull/1427))
+
 ## [1.4.0] 2022-05-17
 
 * [API SDK] Upgrade proto to v0.17.0, update log data model ([#1383](https://github.com/open-telemetry/opentelemetry-cpp/pull/1383))
diff --git a/sdk/src/metrics/sync_instruments.cc b/sdk/src/metrics/sync_instruments.cc
index 5c94ae6f24..7cf9034cdc 100644
--- a/sdk/src/metrics/sync_instruments.cc
+++ b/sdk/src/metrics/sync_instruments.cc
@@ -4,6 +4,9 @@
 #ifndef ENABLE_METRICS_PREVIEW
 #  include "opentelemetry/sdk/metrics/sync_instruments.h"
 #  include "opentelemetry/sdk/metrics/state/metric_storage.h"
+#  include "opentelemetry/sdk_config.h"
+
+#  include <cmath>
 
 OPENTELEMETRY_BEGIN_NAMESPACE
 namespace sdk
@@ -139,11 +142,25 @@ void LongHistogram::Record(long value,
                            const opentelemetry::common::KeyValueIterable &attributes,
                            const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0)
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[LongHistogram::Record(value, attributes)] negative value provided to histogram Name:"
+        << instrument_descriptor_.name_ << " Value:" << value);
+    return;
+  }
   return storage_->RecordLong(value, attributes, context);
 }
 
 void LongHistogram::Record(long value, const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0)
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[LongHistogram::Record(value)] negative value provided to histogram Name:"
+        << instrument_descriptor_.name_ << " Value:" << value);
+    return;
+  }
   return storage_->RecordLong(value, context);
 }
 
@@ -156,11 +173,26 @@ void DoubleHistogram::Record(double value,
                              const opentelemetry::common::KeyValueIterable &attributes,
                              const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0 || std::isnan(value) || std::isinf(value))
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[DoubleHistogram::Record(value, attributes)] negative/nan/infinite value provided to "
+        "histogram Name:"
+        << instrument_descriptor_.name_);
+    return;
+  }
   return storage_->RecordDouble(value, attributes, context);
 }
 
 void DoubleHistogram::Record(double value, const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0 || std::isnan(value) || std::isinf(value))
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[DoubleHistogram::Record(value)] negative/nan/infinite value provided to histogram Name:"
+        << instrument_descriptor_.name_);
+    return;
+  }
   return storage_->RecordDouble(value, context);
 }
 
diff --git a/sdk/test/metrics/sync_instruments_test.cc b/sdk/test/metrics/sync_instruments_test.cc
index e029821d41..1a590d8d26 100644
--- a/sdk/test/metrics/sync_instruments_test.cc
+++ b/sdk/test/metrics/sync_instruments_test.cc
@@ -9,6 +9,8 @@
 #  include "opentelemetry/sdk/metrics/state/multi_metric_storage.h"
 
 #  include <gtest/gtest.h>
+#  include <cmath>
+#  include <limits>
 
 using namespace opentelemetry;
 using namespace opentelemetry::sdk::instrumentationlibrary;
@@ -103,7 +105,7 @@ TEST(SyncInstruments, LongHistogram)
   std::unique_ptr<WritableMetricStorage> metric_storage(new MultiMetricStorage());
   LongHistogram counter(instrument_descriptor, std::move(metric_storage));
   EXPECT_NO_THROW(counter.Record(10l, opentelemetry::context::Context{}));
-  EXPECT_NO_THROW(counter.Record(10l, opentelemetry::context::Context{}));
+  EXPECT_NO_THROW(counter.Record(-10l, opentelemetry::context::Context{}));  // This is ignored
 
   EXPECT_NO_THROW(counter.Record(
       10l, opentelemetry::common::KeyValueIterableView<M>({{"abc", "123"}, {"xyz", "456"}}),
@@ -120,7 +122,11 @@ TEST(SyncInstruments, DoubleHistogram)
   std::unique_ptr<WritableMetricStorage> metric_storage(new MultiMetricStorage());
   DoubleHistogram counter(instrument_descriptor, std::move(metric_storage));
   EXPECT_NO_THROW(counter.Record(10.10, opentelemetry::context::Context{}));
-  EXPECT_NO_THROW(counter.Record(10.10, opentelemetry::context::Context{}));
+  EXPECT_NO_THROW(counter.Record(-10.10, opentelemetry::context::Context{}));  // This is ignored.
+  EXPECT_NO_THROW(counter.Record(std::numeric_limits<double>::quiet_NaN(),
+                                 opentelemetry::context::Context{}));  // This is ignored too
+  EXPECT_NO_THROW(counter.Record(std::numeric_limits<double>::infinity(),
+                                 opentelemetry::context::Context{}));  // This is ignored too
 
   EXPECT_NO_THROW(counter.Record(
       10.10, opentelemetry::common::KeyValueIterableView<M>({{"abc", "123"}, {"xyz", "456"}}),