Fix kwargs in proxy metric instruments, preventing warning about duplicate histograms (#1149)

Author: alexmojaki

logfire/_internal/metrics.py

@@ -79,7 +79,7 @@ def shutdown(self, timeout_millis: float = 30_000) -> None:
             if isinstance(self.provider, SDKMeterProvider):
                 self.provider.shutdown(timeout_millis)
 
-    def force_flush(self, timeout_millis: float = 30_000) -> None:  # pragma: no cover
+    def force_flush(self, timeout_millis: float = 30_000) -> None:
         with self.lock:
             if isinstance(self.provider, SDKMeterProvider):  # pragma: no branch
                 self.provider.force_flush(timeout_millis)
@@ -113,29 +113,27 @@ def set_meter(self, meter_provider: MeterProvider) -> None:
             for instrument in self._instruments:
                 instrument.on_meter_set(real_meter)
 
+    def _add_proxy_instrument(self, instrument_type: type[_ProxyInstrument[InstrumentT]], **kwargs: Any) -> InstrumentT:
+        with self._lock:
+            proxy = instrument_type(self._meter, **kwargs)
+            self._instruments.add(proxy)
+            return proxy  # type: ignore
+
     def create_counter(
         self,
         name: str,
         unit: str = '',
         description: str = '',
     ) -> Counter:
-        with self._lock:
-            proxy = _ProxyCounter(self._meter.create_counter(name, unit, description), name, unit, description)
-            self._instruments.add(proxy)
-            return proxy
+        return self._add_proxy_instrument(_ProxyCounter, name=name, unit=unit, description=description)
 
     def create_up_down_counter(
         self,
         name: str,
         unit: str = '',
         description: str = '',
     ) -> UpDownCounter:
-        with self._lock:
-            proxy = _ProxyUpDownCounter(
-                self._meter.create_up_down_counter(name, unit, description), name, unit, description
-            )
-            self._instruments.add(proxy)
-            return proxy
+        return self._add_proxy_instrument(_ProxyUpDownCounter, name=name, unit=unit, description=description)
 
     def create_observable_counter(
         self,
@@ -144,16 +142,9 @@ def create_observable_counter(
         unit: str = '',
         description: str = '',
     ) -> ObservableCounter:
-        with self._lock:
-            proxy = _ProxyObservableCounter(
-                self._meter.create_observable_counter(name, callbacks, unit, description),
-                name,
-                callbacks,
-                unit,
-                description,
-            )
-            self._instruments.add(proxy)
-            return proxy
+        return self._add_proxy_instrument(
+            _ProxyObservableCounter, name=name, unit=unit, description=description, callbacks=callbacks
+        )
 
     def create_histogram(
         self,
@@ -162,12 +153,7 @@ def create_histogram(
         description: str = '',
         **kwargs: Any,
     ) -> Histogram:
-        with self._lock:
-            proxy = _ProxyHistogram(
-                self._meter.create_histogram(name, unit, description, **kwargs), name, unit, description
-            )
-            self._instruments.add(proxy)
-            return proxy
+        return self._add_proxy_instrument(_ProxyHistogram, name=name, unit=unit, description=description, **kwargs)
 
     def create_gauge(
         self,
@@ -182,10 +168,7 @@ def create_gauge(
                 'You should upgrade to 1.23.0 or newer:\n'
                 '   pip install opentelemetry-sdk>=1.23.0'
             )
-        with self._lock:
-            proxy = _ProxyGauge(self._meter.create_gauge(name, unit, description), name, unit, description)
-            self._instruments.add(proxy)
-            return proxy
+        return self._add_proxy_instrument(_ProxyGauge, name=name, unit=unit, description=description)
 
     def create_observable_gauge(
         self,
@@ -194,16 +177,9 @@ def create_observable_gauge(
         unit: str = '',
         description: str = '',
     ) -> ObservableGauge:
-        with self._lock:
-            proxy = _ProxyObservableGauge(
-                self._meter.create_observable_gauge(name, callbacks, unit, description),
-                name,
-                callbacks,
-                unit,
-                description,
-            )
-            self._instruments.add(proxy)
-            return proxy
+        return self._add_proxy_instrument(
+            _ProxyObservableGauge, name=name, unit=unit, description=description, callbacks=callbacks
+        )
 
     def create_observable_up_down_counter(
         self,
@@ -212,33 +188,18 @@ def create_observable_up_down_counter(
         unit: str = '',
         description: str = '',
     ) -> ObservableUpDownCounter:
-        with self._lock:
-            proxy = _ProxyObservableUpDownCounter(
-                self._meter.create_observable_up_down_counter(name, callbacks, unit, description),
-                name,
-                callbacks,
-                unit,
-                description,
-            )
-            self._instruments.add(proxy)
-            return proxy
+        return self._add_proxy_instrument(
+            _ProxyObservableUpDownCounter, name=name, unit=unit, description=description, callbacks=callbacks
+        )
 
 
 InstrumentT = TypeVar('InstrumentT', bound=Instrument)
 
 
 class _ProxyInstrument(ABC, Generic[InstrumentT]):
-    def __init__(
-        self,
-        instrument: InstrumentT,
-        name: str,
-        unit: str,
-        description: str,
-    ) -> None:
-        self._name = name
-        self._unit = unit
-        self._description = description
-        self._instrument = instrument
+    def __init__(self, meter: Meter, **kwargs: Any) -> None:
+        self._kwargs = kwargs
+        self._instrument = self._create_real_instrument(meter)
 
     def on_meter_set(self, meter: Meter) -> None:
         """Called when a real meter is set on the creating _ProxyMeter."""
@@ -255,20 +216,7 @@ def _create_real_instrument(self, meter: Meter) -> InstrumentT:
     def _increment_span_metric(self, amount: float, attributes: Attributes | None = None):
         span = get_current_span()
         if isinstance(span, _LogfireWrappedSpan):
-            span.increment_metric(self._name, attributes or {}, amount)
-
-
-class _ProxyAsynchronousInstrument(_ProxyInstrument[InstrumentT], ABC):
-    def __init__(
-        self,
-        instrument: InstrumentT,
-        name: str,
-        callbacks: Sequence[CallbackT] | None,
-        unit: str,
-        description: str,
-    ) -> None:
-        super().__init__(instrument, name, unit, description)
-        self._callbacks = callbacks
+            span.increment_metric(self._kwargs['name'], attributes or {}, amount)
 
 
 class _ProxyCounter(_ProxyInstrument[Counter], Counter):
@@ -285,7 +233,7 @@ def add(
         self._instrument.add(amount, attributes, *args, **kwargs)
 
     def _create_real_instrument(self, meter: Meter) -> Counter:
-        return meter.create_counter(self._name, self._unit, self._description)
+        return meter.create_counter(**self._kwargs)
 
 
 class _ProxyHistogram(_ProxyInstrument[Histogram], Histogram):
@@ -300,28 +248,22 @@ def record(
         self._instrument.record(amount, attributes, *args, **kwargs)
 
     def _create_real_instrument(self, meter: Meter) -> Histogram:
-        return meter.create_histogram(self._name, self._unit, self._description)
+        return meter.create_histogram(**self._kwargs)
 
 
-class _ProxyObservableCounter(_ProxyAsynchronousInstrument[ObservableCounter], ObservableCounter):
-    def _create_real_instrument(self, meter: Meter) -> ObservableCounter:  # pragma: no cover
-        return meter.create_observable_counter(self._name, self._callbacks, self._unit, self._description)
+class _ProxyObservableCounter(_ProxyInstrument[ObservableCounter], ObservableCounter):
+    def _create_real_instrument(self, meter: Meter) -> ObservableCounter:
+        return meter.create_observable_counter(**self._kwargs)
 
 
-class _ProxyObservableGauge(
-    _ProxyAsynchronousInstrument[ObservableGauge],
-    ObservableGauge,
-):
-    def _create_real_instrument(self, meter: Meter) -> ObservableGauge:  # pragma: no cover
-        return meter.create_observable_gauge(self._name, self._callbacks, self._unit, self._description)
+class _ProxyObservableGauge(_ProxyInstrument[ObservableGauge], ObservableGauge):
+    def _create_real_instrument(self, meter: Meter) -> ObservableGauge:
+        return meter.create_observable_gauge(**self._kwargs)
 
 
-class _ProxyObservableUpDownCounter(
-    _ProxyAsynchronousInstrument[ObservableUpDownCounter],
-    ObservableUpDownCounter,
-):
-    def _create_real_instrument(self, meter: Meter) -> ObservableUpDownCounter:  # pragma: no cover
-        return meter.create_observable_up_down_counter(self._name, self._callbacks, self._unit, self._description)
+class _ProxyObservableUpDownCounter(_ProxyInstrument[ObservableUpDownCounter], ObservableUpDownCounter):
+    def _create_real_instrument(self, meter: Meter) -> ObservableUpDownCounter:
+        return meter.create_observable_up_down_counter(**self._kwargs)
 
 
 class _ProxyUpDownCounter(_ProxyInstrument[UpDownCounter], UpDownCounter):
@@ -335,7 +277,7 @@ def add(
         self._instrument.add(amount, attributes, *args, **kwargs)
 
     def _create_real_instrument(self, meter: Meter) -> UpDownCounter:
-        return meter.create_up_down_counter(self._name, self._unit, self._description)
+        return meter.create_up_down_counter(**self._kwargs)
 
 
 if Gauge is not None:  # pragma: no branch
@@ -347,10 +289,10 @@ def set(
             attributes: Attributes | None = None,
             *args: Any,
             **kwargs: Any,
-        ) -> None:  # pragma: no cover
+        ) -> None:
             self._instrument.set(amount, attributes, *args, **kwargs)
 
-        def _create_real_instrument(self, meter: Meter):  # pragma: no cover
-            return meter.create_gauge(self._name, self._unit, self._description)
+        def _create_real_instrument(self, meter: Meter):
+            return meter.create_gauge(**self._kwargs)
 else:  # pragma: no cover
     _ProxyGauge = None  # type: ignore

tests/test_metrics.py

@@ -488,3 +488,15 @@ def test_metrics_in_non_recording_spans(exporter: TestExporter, config_kwargs: d
             }
         ]
     )
+
+
+def test_reconfigure(caplog: pytest.LogCaptureFixture):
+    for _ in range(3):
+        logfire.configure(send_to_logfire=False, console=False)
+        meter.create_histogram('foo', unit='x', description='bar', explicit_bucket_boundaries_advisory=[1, 2, 3])
+    # Previously a bug caused a warning to be logged when reconfiguring the metrics
+    assert not caplog.messages
+
+    # For comparison, this logs a warning because the advisory is different (unset)
+    meter.create_histogram('foo', unit='x', description='bar')
+    assert caplog.messages