Add test and improve code

Author: fcollonval

exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/__init__.py

@@ -89,9 +89,10 @@
     OTEL_EXPORTER_PROMETHEUS_PORT,
     OTEL_PYTHON_EXPERIMENTAL_DISABLE_PROMETHEUS_UNIT_NORMALIZATION,
 )
-from opentelemetry.sdk.metrics import Counter
-from opentelemetry.sdk.metrics import Histogram as HistogramInstrument
 from opentelemetry.sdk.metrics import (
+    Counter,
+    Exemplar,
+    Histogram as HistogramInstrument,
     ObservableCounter,
     ObservableGauge,
     ObservableUpDownCounter,
@@ -107,7 +108,8 @@
     Sum,
 )
 from opentelemetry.util.types import Attributes
-from opentelemetry.sdk.metrics._internal import Exemplar
+from opentelemetry.trace import format_span_id, format_trace_id
+
 from prometheus_client.samples import Exemplar as PrometheusExemplar
 
 
@@ -118,29 +120,33 @@
 
 
 def _convert_buckets(
-    bucket_counts: Sequence[int], explicit_bounds: Sequence[float], exemplars: Sequence[Optional[PrometheusExemplar]] = None
+    bucket_counts: Sequence[int],
+    explicit_bounds: Sequence[float],
+    exemplars: Optional[Sequence[PrometheusExemplar]] = None,
 ) -> Sequence[Tuple[str, int, Optional[Exemplar]]]:
     buckets = []
     total_count = 0
-    previous_bound = float('-inf')
+    previous_bound = float("-inf")
+
+    exemplars = list(reversed(exemplars or []))
+    exemplar = exemplars.pop() if exemplars else None
 
     for upper_bound, count in zip(
         chain(explicit_bounds, ["+Inf"]),
         bucket_counts,
     ):
         total_count += count
-        buckets.append((f"{upper_bound}", total_count, None))
-    
-    # assigning exemplars to their corresponding values
-    if exemplars:
-        for i, (upper_bound, _, _) in enumerate(buckets):
-            for exemplar in exemplars:
-                if previous_bound <= exemplar.value < float(upper_bound):
-                        # Assign the exemplar to the current bucket if it's the first valid one found
-                        _, current_count, current_exemplar = buckets[i]
-                        if current_exemplar is None:  # Only assign if no exemplar has been assigned yet
-                            buckets[i] = (upper_bound, current_count, exemplar)
-            previous_bound = float(upper_bound)
+        current_exemplar = None
+        upper_bound_f = float(upper_bound)
+        while exemplar and previous_bound <= exemplar.value < upper_bound_f:
+            if current_exemplar is None:
+                # Assign the exemplar to the current bucket if it's the first valid one found
+                current_exemplar = exemplar
+            exemplar = exemplars.pop() if exemplars else None
+        previous_bound = upper_bound_f
+
+        buckets.append((f"{upper_bound}", total_count, current_exemplar))
+
     return buckets
 
 
@@ -266,10 +272,10 @@ def _translate_to_prometheus(
                 label_keys = []
                 label_values = []
                 exemplars = [
-                    self._convert_exemplar(ex) for ex in number_data_point.exemplars
+                    self._convert_exemplar(ex)
+                    for ex in number_data_point.exemplars
                 ]
 
-
                 for key, value in sorted(number_data_point.attributes.items()):
                     label_keys.append(sanitize_attribute(key))
                     label_values.append(self._check_value(value))
@@ -322,7 +328,6 @@ def _translate_to_prometheus(
                     isinstance(metric.data, Sum)
                     and not should_convert_sum_to_gauge
                 ):
-
                     metric_family_id = "|".join(
                         [pre_metric_family_id, CounterMetricFamily.__name__]
                     )
@@ -343,7 +348,6 @@ def _translate_to_prometheus(
                     isinstance(metric.data, Gauge)
                     or should_convert_sum_to_gauge
                 ):
-
                     metric_family_id = "|".join(
                         [pre_metric_family_id, GaugeMetricFamily.__name__]
                     )
@@ -364,12 +368,11 @@ def _translate_to_prometheus(
                         metric_family_id
                     ].add_metric(labels=label_values, value=value)
                 elif isinstance(metric.data, Histogram):
-
                     metric_family_id = "|".join(
                         [pre_metric_family_id, HistogramMetricFamily.__name__]
                     )
 
-                    if ( 
+                    if (
                         metric_family_id
                         not in metric_family_id_metric_family.keys()
                     ):
@@ -378,15 +381,17 @@ def _translate_to_prometheus(
                                 name=metric_name,
                                 documentation=metric_description,
                                 labels=label_keys,
-                               unit=metric_unit,
+                                unit=metric_unit,
                             )
                         )
                     metric_family_id_metric_family[
                         metric_family_id
                     ].add_metric(
                         labels=label_values,
                         buckets=_convert_buckets(
-                            value["bucket_counts"], value["explicit_bounds"], value["exemplars"]
+                            value["bucket_counts"],
+                            value["explicit_bounds"],
+                            value["exemplars"],
                         ),
                         sum_value=value["sum"],
                     )
@@ -414,7 +419,7 @@ def _create_info_metric(
         info = InfoMetricFamily(name, description, labels=attributes)
         info.add_metric(labels=list(attributes.keys()), value=attributes)
         return info
-    
+
     def _convert_exemplar(self, exemplar_data: Exemplar) -> PrometheusExemplar:
         """
         Converts the SDK exemplar into a Prometheus Exemplar, including proper time conversion.
@@ -426,18 +431,27 @@ def _convert_exemplar(self, exemplar_data: Exemplar) -> PrometheusExemplar:
         Returns:
         - Exemplar: A Prometheus Exemplar object with correct labeling and timing.
         """
-        labels = {self._sanitize_label(key): str(value) for key, value in exemplar_data.filtered_attributes.items()}
-    
+        labels = {
+            sanitize_attribute(key): str(value)
+            for key, value in exemplar_data.filtered_attributes.items()
+        }
+
         # Add trace_id and span_id to labels only if they are valid and not None
-        if exemplar_data.trace_id and exemplar_data.span_id:
-            labels['trace_id'] = exemplar_data.trace_id
-            labels['span_id'] = exemplar_data.span_id
-    
+        if (
+            exemplar_data.trace_id is not None
+            and exemplar_data.span_id is not None
+        ):
+            labels["trace_id"] = format_trace_id(exemplar_data.trace_id)
+            labels["span_id"] = format_span_id(exemplar_data.span_id)
+
         # Convert time from nanoseconds to seconds
         timestamp_seconds = exemplar_data.time_unix_nano / 1e9
-        prom_exemplar = PrometheusExemplar(labels, exemplar_data.value, timestamp_seconds)
+        prom_exemplar = PrometheusExemplar(
+            labels, exemplar_data.value, timestamp_seconds
+        )
         return prom_exemplar
 
+
 class _AutoPrometheusMetricReader(PrometheusMetricReader):
     """Thin wrapper around PrometheusMetricReader used for the opentelemetry_metrics_exporter entry point.
 

exporter/opentelemetry-exporter-prometheus/tests/test_prometheus_exporter.py

@@ -23,6 +23,9 @@
     GaugeMetricFamily,
     InfoMetricFamily,
 )
+from prometheus_client.openmetrics.exposition import (
+    generate_latest as openmetrics_generate_latest,
+)
 
 from opentelemetry.exporter.prometheus import (
     PrometheusMetricReader,
@@ -31,7 +34,7 @@
 from opentelemetry.sdk.environment_variables import (
     OTEL_PYTHON_EXPERIMENTAL_DISABLE_PROMETHEUS_UNIT_NORMALIZATION,
 )
-from opentelemetry.sdk.metrics import MeterProvider
+from opentelemetry.sdk.metrics import MeterProvider, Exemplar
 from opentelemetry.sdk.metrics.export import (
     AggregationTemporality,
     Histogram,
@@ -42,6 +45,7 @@
     ScopeMetrics,
 )
 from opentelemetry.sdk.resources import Resource
+from opentelemetry.trace import format_span_id, format_trace_id
 from opentelemetry.test.metrictestutil import (
     _generate_gauge,
     _generate_histogram,
@@ -59,7 +63,10 @@ def setUp(self):
         )
 
     def verify_text_format(
-        self, metric: Metric, expect_prometheus_text: str
+        self,
+        metric: Metric,
+        expect_prometheus_text: str,
+        openmetrics_generator: bool = False,
     ) -> None:
         metrics_data = MetricsData(
             resource_metrics=[
@@ -79,7 +86,11 @@ def verify_text_format(
 
         collector = _CustomCollector(disable_target_info=True)
         collector.add_metrics_data(metrics_data)
-        result_bytes = generate_latest(collector)
+        result_bytes = (
+            openmetrics_generate_latest(collector)
+            if openmetrics_generator
+            else generate_latest(collector)
+        )
         result = result_bytes.decode("utf-8")
         self.assertEqual(result, expect_prometheus_text)
 
@@ -135,6 +146,67 @@ def test_histogram_to_prometheus(self):
             ),
         )
 
+    def test_histogram_with_exemplar_to_prometheus(self):
+        span_id = 10217189687419569865
+        trace_id = 67545097771067222548457157018666467027
+        metric = Metric(
+            name="test@name",
+            description="foo",
+            unit="s",
+            data=Histogram(
+                data_points=[
+                    HistogramDataPoint(
+                        attributes={"histo": 1},
+                        start_time_unix_nano=1641946016139533244,
+                        time_unix_nano=1641946016139533244,
+                        exemplars=[
+                            Exemplar(
+                                {"filtered": "banana"},
+                                305.0,
+                                1641946016139533244,
+                                span_id,
+                                trace_id,
+                            ),
+                            # Will be ignored as part of the same buckets
+                            Exemplar(
+                                {"filtered": "banana"},
+                                298.0,
+                                1641946016139533400,
+                                span_id,
+                                trace_id,
+                            ),
+                        ],
+                        count=6,
+                        sum=579.0,
+                        bucket_counts=[1, 3, 2],
+                        explicit_bounds=[123.0, 456.0],
+                        min=1,
+                        max=457,
+                    )
+                ],
+                aggregation_temporality=AggregationTemporality.DELTA,
+            ),
+        )
+        span_str = format_span_id(span_id)
+        trace_str = format_trace_id(trace_id)
+        self.verify_text_format(
+            metric,
+            dedent(
+                f"""\
+                # HELP test_name_seconds foo
+                # TYPE test_name_seconds histogram
+                # UNIT test_name_seconds seconds
+                test_name_seconds_bucket{{histo="1",le="123.0"}} 1.0
+                test_name_seconds_bucket{{histo="1",le="456.0"}} 4.0 # {{filtered="banana",span_id="{span_str}",trace_id="{trace_str}"}} 305.0 1641946016.1395333
+                test_name_seconds_bucket{{histo="1",le="+Inf"}} 6.0
+                test_name_seconds_count{{histo="1"}} 6.0
+                test_name_seconds_sum{{histo="1"}} 579.0
+                # EOF
+                """
+            ),
+            openmetrics_generator=True,
+        )
+
     def test_monotonic_sum_to_prometheus(self):
         labels = {"environment@": "staging", "os": "Windows"}
         metric = _generate_sum(
@@ -321,7 +393,6 @@ def test_list_labels(self):
             self.assertEqual(prometheus_metric.samples[0].labels["os"], "Unix")
 
     def test_check_value(self):
-
         collector = _CustomCollector()
 
         self.assertEqual(collector._check_value(1), "1")
@@ -335,7 +406,6 @@ def test_check_value(self):
         self.assertEqual(collector._check_value(None), "null")
 
     def test_multiple_collection_calls(self):
-
         metric_reader = PrometheusMetricReader()
         provider = MeterProvider(metric_readers=[metric_reader])
         meter = provider.get_meter("getting-started", "0.1.2")