flask: fixed http_server_request_duration metrics being recorded for excluded urls (#3794)

* Fixed an issue #2352 where http_server_request_duration metrics was being recorded for excluded urls.

* Updated CHANGELOG

* Fix indentation

* Fix tests

* Fix assert condition

* Update CHANGELOG.md

Co-authored-by: Riccardo Magliocchetti <[email protected]>

* Addressed comments

* Modified logic for excluded urls

* Fix test name

* Align tests with the same assertion

* Update instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py

---------

Co-authored-by: Riccardo Magliocchetti <[email protected]>

Author: rads-1996

CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `opentelemetry-instrumentation-flask`: Do not record `http.server.duration` metrics for excluded URLs.
+  ([#3794](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3794))
 - `opentelemetry-instrumentation-botocore`: migrate off the deprecated events API to use the logs API
   ([#3624](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3624))
 - `opentelemetry-instrumentation-dbapi`: fix crash retrieving libpq version when enabling commenter with psycopg

instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py

@@ -350,11 +350,12 @@ def _wrapped_app(wrapped_app_environ, start_response):
         active_requests_counter.add(1, active_requests_count_attrs)
         request_route = None
 
+        should_trace = True
+
         def _start_response(status, response_headers, *args, **kwargs):
-            if flask.request and (
-                excluded_urls is None
-                or not excluded_urls.url_disabled(flask.request.url)
-            ):
+            nonlocal should_trace
+            should_trace = _should_trace(excluded_urls)
+            if should_trace:
                 nonlocal request_route
                 request_route = flask.request.url_rule
 
@@ -395,33 +396,43 @@ def _start_response(status, response_headers, *args, **kwargs):
             return start_response(status, response_headers, *args, **kwargs)
 
         result = wsgi_app(wrapped_app_environ, _start_response)
-        duration_s = default_timer() - start
-        if duration_histogram_old:
-            duration_attrs_old = otel_wsgi._parse_duration_attrs(
-                attributes, _StabilityMode.DEFAULT
-            )
+        if should_trace:
+            duration_s = default_timer() - start
+            if duration_histogram_old:
+                duration_attrs_old = otel_wsgi._parse_duration_attrs(
+                    attributes, _StabilityMode.DEFAULT
+                )
 
-            if request_route:
-                # http.target to be included in old semantic conventions
-                duration_attrs_old[HTTP_TARGET] = str(request_route)
+                if request_route:
+                    # http.target to be included in old semantic conventions
+                    duration_attrs_old[HTTP_TARGET] = str(request_route)
 
-            duration_histogram_old.record(
-                max(round(duration_s * 1000), 0), duration_attrs_old
-            )
-        if duration_histogram_new:
-            duration_attrs_new = otel_wsgi._parse_duration_attrs(
-                attributes, _StabilityMode.HTTP
-            )
+                duration_histogram_old.record(
+                    max(round(duration_s * 1000), 0), duration_attrs_old
+                )
+            if duration_histogram_new:
+                duration_attrs_new = otel_wsgi._parse_duration_attrs(
+                    attributes, _StabilityMode.HTTP
+                )
 
-            if request_route:
-                duration_attrs_new[HTTP_ROUTE] = str(request_route)
+                if request_route:
+                    duration_attrs_new[HTTP_ROUTE] = str(request_route)
 
-            duration_histogram_new.record(
-                max(duration_s, 0), duration_attrs_new
-            )
+                duration_histogram_new.record(
+                    max(duration_s, 0), duration_attrs_new
+                )
         active_requests_counter.add(-1, active_requests_count_attrs)
         return result
 
+    def _should_trace(excluded_urls) -> bool:
+        return bool(
+            flask.request
+            and (
+                excluded_urls is None
+                or not excluded_urls.url_disabled(flask.request.url)
+            )
+        )
+
     return _wrapped_app
 
 

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py

@@ -164,7 +164,7 @@ def setUp(self):
         self.env_patch = patch.dict(
             "os.environ",
             {
-                "OTEL_PYTHON_FLASK_EXCLUDED_URLS": "http://localhost/env_excluded_arg/123,env_excluded_noarg",
+                "OTEL_PYTHON_FLASK_EXCLUDED_URLS": "http://localhost/env_excluded_arg/123,env_excluded_noarg,env_excluded_arg/789",
                 OTEL_SEMCONV_STABILITY_OPT_IN: sem_conv_mode,
             },
         )
@@ -736,6 +736,80 @@ def test_metric_uninstrument(self):
                         if isinstance(point, HistogramDataPoint):
                             self.assertEqual(point.count, 1)
 
+    def test_flask_metrics_excluded_urls(self):
+        start = default_timer()
+        self.client.get("/env_excluded_arg/123")
+        self.client.get("/env_excluded_noarg")
+        self.client.get("/env_excluded_arg/789")
+        duration = max(round((default_timer() - start) * 1000), 0)
+        metrics_list = self.memory_metrics_reader.get_metrics_data()
+        number_data_point_seen = False
+        histogram_data_point_seen = False
+        self.assertTrue(len(metrics_list.resource_metrics) != 0)
+        for resource_metric in metrics_list.resource_metrics:
+            self.assertTrue(len(resource_metric.scope_metrics) != 0)
+            for scope_metric in resource_metric.scope_metrics:
+                self.assertTrue(len(scope_metric.metrics) != 0)
+                for metric in scope_metric.metrics:
+                    self.assertIn(metric.name, _expected_metric_names_old)
+                    data_points = list(metric.data.data_points)
+                    self.assertEqual(len(data_points), 1)
+                    for point in data_points:
+                        if isinstance(point, HistogramDataPoint):
+                            self.assertEqual(point.count, 0)
+                            self.assertAlmostEqual(
+                                duration, point.sum, delta=10
+                            )
+                            histogram_data_point_seen = True
+                        if isinstance(point, NumberDataPoint):
+                            number_data_point_seen = True
+                        for attr in point.attributes:
+                            self.assertIn(
+                                attr,
+                                _recommended_metrics_attrs_old[metric.name],
+                            )
+        self.assertTrue(number_data_point_seen)
+        self.assertFalse(histogram_data_point_seen)
+
+    def test_flask_metrics_excluded_urls_new_semconv(self):
+        start = default_timer()
+        self.client.get("/env_excluded_arg/123")
+        self.client.get("/env_excluded_noarg")
+        self.client.get("/env_excluded_arg/789")
+        duration_s = max(default_timer() - start, 0)
+        metrics_list = self.memory_metrics_reader.get_metrics_data()
+        number_data_point_seen = False
+        histogram_data_point_seen = False
+        self.assertTrue(len(metrics_list.resource_metrics) != 0)
+        for resource_metric in metrics_list.resource_metrics:
+            self.assertTrue(len(resource_metric.scope_metrics) != 0)
+            for scope_metric in resource_metric.scope_metrics:
+                self.assertTrue(len(scope_metric.metrics) != 0)
+                for metric in scope_metric.metrics:
+                    self.assertIn(metric.name, _expected_metric_names_new)
+                    data_points = list(metric.data.data_points)
+                    self.assertEqual(len(data_points), 1)
+                    for point in data_points:
+                        if isinstance(point, HistogramDataPoint):
+                            self.assertEqual(point.count, 0)
+                            self.assertAlmostEqual(
+                                duration_s, point.sum, places=1
+                            )
+                            self.assertEqual(
+                                point.explicit_bounds,
+                                HTTP_DURATION_HISTOGRAM_BUCKETS_NEW,
+                            )
+                            histogram_data_point_seen = True
+                        if isinstance(point, NumberDataPoint):
+                            number_data_point_seen = True
+                        for attr in point.attributes:
+                            self.assertIn(
+                                attr,
+                                _recommended_metrics_attrs_new[metric.name],
+                            )
+        self.assertTrue(number_data_point_seen)
+        self.assertFalse(histogram_data_point_seen)
+
 
 class TestProgrammaticHooks(InstrumentationTest, WsgiTestBase):
     def setUp(self):