From 6bdc5fd20ddf4394eb4630d8bb6847f7e399a817 Mon Sep 17 00:00:00 2001
From: Leighton Chen <lechen@microsoft.com>
Date: Thu, 20 Jan 2022 12:58:45 -0800
Subject: [PATCH] Validate add/record operations for instruments (#2394)

---
 .../opentelemetry/sdk/_metrics/instrument.py  | 19 ++++++--
 .../tests/metrics/test_instrument.py          | 45 +++++++++++++++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py b/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py
index 3b2db188354..d9da9a067a0 100644
--- a/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py
+++ b/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py
@@ -14,6 +14,7 @@
 
 # pylint: disable=too-many-ancestors
 
+import logging
 from typing import Dict, Generator, Iterable, Union
 
 from opentelemetry._metrics.instrument import CallbackT
@@ -33,6 +34,8 @@
 from opentelemetry.sdk._metrics.measurement_consumer import MeasurementConsumer
 from opentelemetry.sdk.util.instrumentation import InstrumentationInfo
 
+_logger = logging.getLogger(__name__)
+
 
 class _Synchronous:
     def __init__(
@@ -87,8 +90,10 @@ def add(
         self, amount: Union[int, float], attributes: Dict[str, str] = None
     ):
         if amount < 0:
-            raise Exception("amount must be non negative")
-
+            _logger.warning(
+                "Add amount must be non-negative on Counter %s.", self.name
+            )
+            return
         self._measurement_consumer.consume_measurement(
             Measurement(amount, attributes)
         )
@@ -112,7 +117,15 @@ class ObservableUpDownCounter(_Asynchronous, APIObservableUpDownCounter):
 
 
 class Histogram(_Synchronous, APIHistogram):
-    def record(self, amount, attributes=None):
+    def record(
+        self, amount: Union[int, float], attributes: Dict[str, str] = None
+    ):
+        if amount < 0:
+            _logger.warning(
+                "Record amount must be non-negative on Histogram %s.",
+                self.name,
+            )
+            return
         self._measurement_consumer.consume_measurement(
             Measurement(amount, attributes)
         )
diff --git a/opentelemetry-sdk/tests/metrics/test_instrument.py b/opentelemetry-sdk/tests/metrics/test_instrument.py
index 64d20d68628..5504fdb686b 100644
--- a/opentelemetry-sdk/tests/metrics/test_instrument.py
+++ b/opentelemetry-sdk/tests/metrics/test_instrument.py
@@ -16,12 +16,43 @@
 from unittest.mock import Mock
 
 from opentelemetry.sdk._metrics.instrument import (
+    Counter,
+    Histogram,
     ObservableCounter,
     ObservableGauge,
     ObservableUpDownCounter,
+    UpDownCounter,
 )
 
 
+class TestCounter(TestCase):
+    def test_add(self):
+        mc = Mock()
+        counter = Counter("name", Mock(), mc)
+        counter.add(1.0)
+        mc.consume_measurement.assert_called_once()
+
+    def test_add_non_monotonic(self):
+        mc = Mock()
+        counter = Counter("name", Mock(), mc)
+        counter.add(-1.0)
+        mc.consume_measurement.assert_not_called()
+
+
+class TestUpDownCounter(TestCase):
+    def test_add(self):
+        mc = Mock()
+        counter = UpDownCounter("name", Mock(), mc)
+        counter.add(1.0)
+        mc.consume_measurement.assert_called_once()
+
+    def test_add_non_monotonic(self):
+        mc = Mock()
+        counter = UpDownCounter("name", Mock(), mc)
+        counter.add(-1.0)
+        mc.consume_measurement.assert_called_once()
+
+
 class TestObservableGauge(TestCase):
     def test_callable_callback(self):
         def callback():
@@ -82,3 +113,17 @@ def callback():
         )
 
         self.assertEqual(observable_up_down_counter.callback(), [1, 2, 3])
+
+
+class TestHistogram(TestCase):
+    def test_record(self):
+        mc = Mock()
+        hist = Histogram("name", Mock(), mc)
+        hist.record(1.0)
+        mc.consume_measurement.assert_called_once()
+
+    def test_record_non_monotonic(self):
+        mc = Mock()
+        hist = Histogram("name", Mock(), mc)
+        hist.record(-1.0)
+        mc.consume_measurement.assert_not_called()