diff --git a/CHANGELOG.md b/CHANGELOG.md index 9814367242..e77a7c1ba2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py b/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py index 9a22383e1b..57f0f75ca3 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py +++ b/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 diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py index 60f08c7cd2..2da579adda 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py +++ b/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):