Refactor metrics instrument (#2297)

* Refactor instrument

Fixes #2294

* Add Callback type back to API

* Create instrument with meter

* Make default_aggregation public

* Use right import

* Remove default aggregations

Author: ocelotl

opentelemetry-api/src/opentelemetry/_metrics/instrument.py

@@ -17,7 +17,6 @@
 # type: ignore
 
 from abc import ABC, abstractmethod
-from collections import abc as collections_abc
 from logging import getLogger
 from typing import (
     Callable,
@@ -33,10 +32,11 @@
 from opentelemetry import _metrics as metrics
 from opentelemetry._metrics.measurement import Measurement
 
-_TInstrumentCallback = Callable[[], Iterable[Measurement]]
-_TInstrumentCallbackGenerator = Generator[Iterable[Measurement], None, None]
-TCallback = Union[_TInstrumentCallback, _TInstrumentCallbackGenerator]
 InstrumentT = TypeVar("InstrumentT", bound="Instrument")
+CallbackT = Union[
+    Callable[[], Iterable[Measurement]],
+    Generator[Iterable[Measurement], None, None],
+]
 
 
 _logger = getLogger(__name__)
@@ -87,45 +87,11 @@ class Asynchronous(Instrument):
     def __init__(
         self,
         name,
-        callback: TCallback,
-        *args,
+        callback,
         unit="",
         description="",
-        **kwargs
     ):
-        super().__init__(
-            name, *args, unit=unit, description=description, **kwargs
-        )
-
-        if isinstance(callback, collections_abc.Callable):
-            self._callback = callback
-        elif isinstance(callback, collections_abc.Generator):
-            self._callback = self._wrap_generator_callback(callback)
-        # FIXME check that callback is a callable or generator
-
-    @staticmethod
-    def _wrap_generator_callback(
-        generator_callback: _TInstrumentCallbackGenerator,
-    ) -> _TInstrumentCallback:
-        """Wraps a generator style callback into a callable one"""
-        has_items = True
-
-        def inner() -> Iterable[Measurement]:
-            nonlocal has_items
-            if not has_items:
-                return []
-
-            try:
-                return next(generator_callback)
-            except StopIteration:
-                has_items = False
-                # FIXME handle the situation where the callback generator has
-                # run out of measurements
-                return []
-
-        return inner
-
-    # FIXME check that callbacks return an iterable of Measurements
+        super().__init__(name, unit=unit, description=description)
 
 
 class _Adding(Instrument):

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py

@@ -12,13 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# pylint: disable=function-redefined
-# pylint: disable=dangerous-default-value
-# Classes in this module use dictionaries as default arguments. This is
-# considered dangerous by pylint because the default dictionary is shared by
-# all instances. Implementations of these classes must not make any change to
-# this default dictionary in __init__.
+# pylint: disable=too-many-ancestors
 
+from typing import Dict, Generator, Iterable, Union
+
+from opentelemetry._metrics.instrument import CallbackT
 from opentelemetry._metrics.instrument import Counter as APICounter
 from opentelemetry._metrics.instrument import Histogram as APIHistogram
 from opentelemetry._metrics.instrument import (
@@ -30,131 +28,77 @@
 from opentelemetry._metrics.instrument import (
     ObservableUpDownCounter as APIObservableUpDownCounter,
 )
-from opentelemetry._metrics.instrument import TCallback
 from opentelemetry._metrics.instrument import UpDownCounter as APIUpDownCounter
+from opentelemetry.sdk._metrics.measurement import Measurement
 from opentelemetry.sdk.util.instrumentation import InstrumentationInfo
 
 
-class _Instrument:
+class _Synchronous:
     def __init__(
         self,
         instrumentation_info: InstrumentationInfo,
         name: str,
-        *args,
         unit: str = "",
         description: str = "",
     ):
         self._instrumentation_info = instrumentation_info
-        super().__init__(name, *args, unit=unit, description=description)
+        super().__init__(name, unit=unit, description=description)
 
 
-class Counter(_Instrument, APICounter):
+class _Asynchronous:
     def __init__(
         self,
         instrumentation_info: InstrumentationInfo,
         name: str,
+        callback: CallbackT,
         unit: str = "",
         description: str = "",
     ):
-        super().__init__(
-            instrumentation_info,
-            name,
-            unit=unit,
-            description=description,
-        )
-
-    def add(self, amount, attributes=None):
-        # FIXME check that the amount is non negative
-        pass
 
+        self._instrumentation_info = instrumentation_info
+        super().__init__(name, callback, unit=unit, description=description)
 
-class UpDownCounter(_Instrument, APIUpDownCounter):
-    def __init__(
-        self,
-        instrumentation_info: InstrumentationInfo,
-        name: str,
-        unit: str = "",
-        description: str = "",
-    ):
-        super().__init__(
-            instrumentation_info,
-            name,
-            unit=unit,
-            description=description,
-        )
-
-    def add(self, amount, attributes=None):
-        pass
+        self._callback = callback
 
+        if isinstance(callback, Generator):
 
-class ObservableCounter(_Instrument, APIObservableCounter):
-    def __init__(
-        self,
-        instrumentation_info: InstrumentationInfo,
-        name: str,
-        callback: TCallback,
-        unit: str = "",
-        description: str = "",
-    ):
-        super().__init__(
-            instrumentation_info,
-            name,
-            callback,
-            unit=unit,
-            description=description,
-        )
+            def inner() -> Iterable[Measurement]:
+                return next(callback)
 
+            self._callback = inner
 
-class ObservableUpDownCounter(_Instrument, APIObservableUpDownCounter):
-    def __init__(
-        self,
-        instrumentation_info: InstrumentationInfo,
-        name: str,
-        callback: TCallback,
-        unit: str = "",
-        description: str = "",
+    @property
+    def callback(self) -> CallbackT:
+        return self._callback
+
+
+class Counter(_Synchronous, APICounter):
+    def add(
+        self, amount: Union[int, float], attributes: Dict[str, str] = None
     ):
-        super().__init__(
-            instrumentation_info,
-            name,
-            callback,
-            unit=unit,
-            description=description,
-        )
+        if amount < 0:
+            raise Exception("amount must be non negative")
 
 
-class Histogram(_Instrument, APIHistogram):
-    def __init__(
-        self,
-        instrumentation_info: InstrumentationInfo,
-        name: str,
-        unit: str = "",
-        description: str = "",
+class UpDownCounter(_Synchronous, APIUpDownCounter):
+    def add(
+        self, amount: Union[int, float], attributes: Dict[str, str] = None
     ):
-        super().__init__(
-            instrumentation_info,
-            name,
-            unit=unit,
-            description=description,
-        )
+        pass
+
 
+class ObservableCounter(_Asynchronous, APIObservableCounter):
+    pass
+
+
+class ObservableUpDownCounter(_Asynchronous, APIObservableUpDownCounter):
+    pass
+
+
+class Histogram(_Synchronous, APIHistogram):
     def record(self, amount, attributes=None):
         pass
 
 
-class ObservableGauge(_Instrument, APIObservableGauge):
-    def __init__(
-        self,
-        instrumentation_info: InstrumentationInfo,
-        name: str,
-        callback: TCallback,
-        unit: str = "",
-        description: str = "",
-    ):
-        super().__init__(
-            instrumentation_info,
-            name,
-            callback,
-            unit=unit,
-            description=description,
-        )
+class ObservableGauge(_Asynchronous, APIObservableGauge):
+    pass

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/measurement.py

@@ -0,0 +1,17 @@
+# Copyright The OpenTelemetry Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+class Measurement:
+    pass

opentelemetry-api/tests/metrics/integration_test/test_cpu_time.py renamed to opentelemetry-sdk/tests/metrics/integration_test/test_cpu_time.py

@@ -17,8 +17,8 @@
 from typing import Generator, Iterable
 from unittest import TestCase
 
-from opentelemetry._metrics import _DefaultMeter
 from opentelemetry._metrics.measurement import Measurement
+from opentelemetry.sdk._metrics import MeterProvider
 
 # FIXME Test that the instrument methods can be called concurrently safely.
 
@@ -61,8 +61,6 @@ class TestCpuTimeIntegration(TestCase):
     ]
 
     def test_cpu_time_callback(self):
-        meter = _DefaultMeter("foo")
-
         def cpu_time_callback() -> Iterable[Measurement]:
             procstat = io.StringIO(self.procstat_str)
             procstat.readline()  # skip the first line
@@ -98,18 +96,17 @@ def cpu_time_callback() -> Iterable[Measurement]:
                     int(states[8]) // 100, {"cpu": cpu, "state": "guest_nice"}
                 )
 
+        meter = MeterProvider().get_meter("name")
         observable_counter = meter.create_observable_counter(
             "system.cpu.time",
-            callback=cpu_time_callback,
+            cpu_time_callback,
             unit="s",
             description="CPU time",
         )
         measurements = list(observable_counter._callback())
         self.assertEqual(measurements, self.measurements_expected)
 
     def test_cpu_time_generator(self):
-        meter = _DefaultMeter("foo")
-
         def cpu_time_generator() -> Generator[
             Iterable[Measurement], None, None
         ]:
@@ -176,6 +173,7 @@ def cpu_time_generator() -> Generator[
                     )
                 yield measurements
 
+        meter = MeterProvider().get_meter("name")
         observable_counter = meter.create_observable_counter(
             "system.cpu.time",
             callback=cpu_time_generator(),