warn if Counter used incorrectly

Author: aabmass

opentelemetry-api/src/opentelemetry/metrics/__init__.py

@@ -66,7 +66,17 @@ def add(self, value: ValueT) -> None:
         """Increases the value of the bound counter by ``value``.
 
         Args:
-            value: The value to add to the bound counter.
+            value: The value to add to the bound counter. Must be positive.
+        """
+
+
+class BoundUpDownCounter:
+    def add(self, value: ValueT) -> None:
+        """Increases the value of the bound counter by ``value``.
+
+        Args:
+            value: The value to add to the bound counter. Can be positive or
+                negative.
         """
 
 
@@ -148,9 +158,9 @@ class UpDownCounter(Metric):
     """A counter type metric that expresses the computation of a sum,
     allowing negative increments."""
 
-    def bind(self, labels: Dict[str, str]) -> "BoundCounter":
-        """Gets a `BoundCounter`."""
-        return BoundCounter()
+    def bind(self, labels: Dict[str, str]) -> "BoundUpDownCounter":
+        """Gets a `BoundUpDownCounter`."""
+        return BoundUpDownCounter()
 
     def add(self, value: ValueT, labels: Dict[str, str]) -> None:
         """Increases the value of the counter by ``value``.

opentelemetry-api/tests/metrics/test_metrics.py

@@ -38,7 +38,7 @@ def test_counter_add(self):
     def test_updowncounter(self):
         counter = metrics.UpDownCounter()
         bound_counter = counter.bind({})
-        self.assertIsInstance(bound_counter, metrics.BoundCounter)
+        self.assertIsInstance(bound_counter, metrics.BoundUpDownCounter)
 
     def test_updowncounter_add(self):
         counter = metrics.Counter()

opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py

@@ -102,6 +102,25 @@ def add(self, value: metrics_api.ValueT) -> None:
         if self._validate_update(value):
             self.update(value)
 
+    def _validate_update(self, value: metrics_api.ValueT) -> bool:
+        if super()._validate_update(value):
+            if value >= 0:
+                return True
+            logger.warning(
+                "Invalid value %s passed to Counter, value must be "
+                "non-negative. For a Counter that can decrease, use "
+                "UpDownCounter.",
+                value,
+            )
+        return False
+
+
+class BoundUpDownCounter(metrics_api.BoundUpDownCounter, BaseBoundInstrument):
+    def add(self, value: metrics_api.ValueT) -> None:
+        """See `opentelemetry.metrics.BoundUpDownCounter.add`."""
+        if self._validate_update(value):
+            self.update(value)
+
 
 class BoundValueRecorder(metrics_api.BoundValueRecorder, BaseBoundInstrument):
     def record(self, value: metrics_api.ValueT) -> None:
@@ -188,7 +207,7 @@ class UpDownCounter(Metric, metrics_api.UpDownCounter):
     """See `opentelemetry.metrics.UpDownCounter`.
     """
 
-    BOUND_INSTR_TYPE = BoundCounter
+    BOUND_INSTR_TYPE = BoundUpDownCounter
 
     def add(self, value: metrics_api.ValueT, labels: Dict[str, str]) -> None:
         """See `opentelemetry.metrics.UpDownCounter.add`."""

opentelemetry-sdk/tests/metrics/test_metrics.py

@@ -305,9 +305,37 @@ def test_add(self):
         metric.add(2, labels)
         self.assertEqual(bound_counter.aggregator.current, 5)
 
+    @mock.patch("opentelemetry.sdk.metrics.logger")
+    def test_add_non_decreasing_int_error(self, logger_mock):
+        meter = metrics.MeterProvider().get_meter(__name__)
+        metric = metrics.Counter("name", "desc", "unit", int, meter, ("key",))
+        labels = {"key": "value"}
+        bound_counter = metric.bind(labels)
+        metric.add(3, labels)
+        metric.add(0, labels)
+        metric.add(-1, labels)
+        self.assertEqual(bound_counter.aggregator.current, 3)
+        self.assertEqual(logger_mock.warning.call_count, 1)
+
+    @mock.patch("opentelemetry.sdk.metrics.logger")
+    def test_add_non_decreasing_float_error(self, logger_mock):
+        meter = metrics.MeterProvider().get_meter(__name__)
+        metric = metrics.Counter(
+            "name", "desc", "unit", float, meter, ("key",)
+        )
+        labels = {"key": "value"}
+        bound_counter = metric.bind(labels)
+        metric.add(3.3, labels)
+        metric.add(0.0, labels)
+        metric.add(0.1, labels)
+        metric.add(-0.1, labels)
+        self.assertEqual(bound_counter.aggregator.current, 3.4)
+        self.assertEqual(logger_mock.warning.call_count, 1)
+
 
 class TestUpDownCounter(unittest.TestCase):
-    def test_add(self):
+    @mock.patch("opentelemetry.sdk.metrics.logger")
+    def test_add(self, logger_mock):
         meter = metrics.MeterProvider().get_meter(__name__)
         metric = metrics.UpDownCounter(
             "name", "desc", "unit", int, meter, ("key",)
@@ -322,6 +350,7 @@ def test_add(self):
         metric.add(-3, labels)
         metric.add(-1, labels)
         self.assertEqual(bound_counter.aggregator.current, 1)
+        self.assertEqual(logger_mock.warning.call_count, 0)
 
 
 class TestValueRecorder(unittest.TestCase):