From 50168db1562de23d2e0c036dff59fa230bd15603 Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Thu, 10 Nov 2022 12:23:58 -0300 Subject: [PATCH] revert: feat(instrumentation/asgi): add target to metrics Unfortunately I made a mistake in https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1323 where I assumed that the scope was laready being processed by the framework at the moment of reporting, but of course that's not true as the framework has not even seen the request at that point. Is for that reason that we are not able to extract any route information in the middleware to report what the target is. Sorry for the noise, and be happy to help if anyone has any idea how we could fix this. --- .../instrumentation/asgi/__init__.py | 35 ---------- .../tests/test_asgi_middleware.py | 64 ------------------- 2 files changed, 99 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py index 608aeade7f..d730dc5119 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -425,33 +425,6 @@ def get_default_span_details(scope: dict) -> Tuple[str, dict]: return span_name, {} - -def _collect_target_attribute( - scope: typing.Dict[str, typing.Any] -) -> typing.Optional[str]: - """ - Returns the target path as defined by the Semantic Conventions. - - This value is suitable to use in metrics as it should replace concrete - values with a parameterized name. Example: /api/users/{user_id} - - Refer to the specification - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#parameterized-attributes - - Note: this function requires specific code for each framework, as there's no - standard attribute to use. - """ - # FastAPI - root_path = scope.get("root_path", "") - - route = scope.get("route") - path_format = getattr(route, "path_format", None) - if path_format: - return f"{root_path}{path_format}" - - return None - - class OpenTelemetryMiddleware: """The ASGI application middleware. @@ -473,7 +446,6 @@ class OpenTelemetryMiddleware: the current globally configured one is used. """ - # pylint: disable=too-many-branches def __init__( self, app, @@ -542,11 +514,6 @@ async def __call__(self, scope, receive, send): ) duration_attrs = _parse_duration_attrs(attributes) - target = _collect_target_attribute(scope) - if target: - active_requests_count_attrs[SpanAttributes.HTTP_TARGET] = target - duration_attrs[SpanAttributes.HTTP_TARGET] = target - if scope["type"] == "http": self.active_requests_counter.add(1, active_requests_count_attrs) try: @@ -589,8 +556,6 @@ async def __call__(self, scope, receive, send): if token: context.detach(token) - # pylint: enable=too-many-branches - def _get_otel_receive(self, server_span_name, scope, receive): @wraps(receive) async def otel_receive(): diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index 7582ffb998..13492762e7 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -12,8 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -# pylint: disable=too-many-lines - import sys import unittest from timeit import default_timer @@ -585,37 +583,6 @@ def test_basic_metric_success(self): ) self.assertEqual(point.value, 0) - def test_metric_target_attribute(self): - expected_target = "/api/user/{id}" - - class TestRoute: - path_format = expected_target - - self.scope["route"] = TestRoute() - app = otel_asgi.OpenTelemetryMiddleware(simple_asgi) - self.seed_app(app) - self.send_default_request() - - metrics_list = self.memory_metrics_reader.get_metrics_data() - assertions = 0 - for resource_metric in metrics_list.resource_metrics: - for scope_metrics in resource_metric.scope_metrics: - for metric in scope_metrics.metrics: - for point in metric.data.data_points: - if isinstance(point, HistogramDataPoint): - self.assertEqual( - point.attributes["http.target"], - expected_target, - ) - assertions += 1 - elif isinstance(point, NumberDataPoint): - self.assertEqual( - point.attributes["http.target"], - expected_target, - ) - assertions += 1 - self.assertEqual(assertions, 2) - def test_no_metric_for_websockets(self): self.scope = { "type": "websocket", @@ -709,37 +676,6 @@ def test_credential_removal(self): attrs[SpanAttributes.HTTP_URL], "http://httpbin.org/status/200" ) - def test_collect_target_attribute_missing(self): - self.assertIsNone(otel_asgi._collect_target_attribute(self.scope)) - - def test_collect_target_attribute_fastapi(self): - class TestRoute: - path_format = "/api/users/{user_id}" - - self.scope["route"] = TestRoute() - self.assertEqual( - otel_asgi._collect_target_attribute(self.scope), - "/api/users/{user_id}", - ) - - def test_collect_target_attribute_fastapi_mounted(self): - class TestRoute: - path_format = "/users/{user_id}" - - self.scope["route"] = TestRoute() - self.scope["root_path"] = "/api/v2" - self.assertEqual( - otel_asgi._collect_target_attribute(self.scope), - "/api/v2/users/{user_id}", - ) - - def test_collect_target_attribute_fastapi_starlette_invalid(self): - self.scope["route"] = object() - self.assertIsNone( - otel_asgi._collect_target_attribute(self.scope), - "HTTP_TARGET values is not None", - ) - class TestWrappedApplication(AsgiTestBase): def test_mark_span_internal_in_presence_of_span_from_other_framework(self):