From 086213c41844c3767471bd4a295f015969993e5a Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Sat, 10 Sep 2022 02:04:43 -0300 Subject: [PATCH 1/7] feat(instrumentation/asgi): add target to metrics This PR adds the target information for metrics reported by instrumentation/asgi. Unfortunately, there's no ASGI standard to reliably get this information, and I was only able to get it for FastAPI. I also tried to get the info with Sanic and Starlette (https://github.com/encode/starlette/issues/685), but there's nothing in the scope allowing to recreate the route. Besides the included unit tests, the logic was tested using the following app: ```python import io import fastapi app = fastapi.FastAPI() def dump_scope(scope): b = io.StringIO() print(scope, file=b) return b.getvalue() @app.get("/test/{id}") def test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} sub_app = fastapi.FastAPI() @sub_app.get("/test/{id}") def sub_test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} app.mount("/sub", sub_app) ``` Partially fixes #1116 Note to reviewers: I tried to touch as less as possible, so that we don;t require a refactor before this change. However, we could consider changing `collect_request_attributes` so that it returns both a trace attributes and a metrics attributes. Wihout that change we cannot add the `HTTP_TARGET` attribute to the list of metric atttributes, because it will be present but with high cardinality. --- .../instrumentation/asgi/__init__.py | 34 +++++++++++ .../tests/test_asgi_middleware.py | 61 +++++++++++++++++++ 2 files changed, 95 insertions(+) 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 21e8a189b5..fab9bbafd9 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -365,6 +365,34 @@ 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") + if not route: + return None + path_format = getattr(route, "path_format", None) + if path_format: + return f"{root_path}{path_format}" + + return None + + class OpenTelemetryMiddleware: """The ASGI application middleware. @@ -448,6 +476,12 @@ async def __call__(self, scope, receive, send): attributes ) 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: diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index e6b75d7125..04e211bbd3 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -612,6 +612,37 @@ 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", @@ -705,6 +736,36 @@ 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), None + ) + class TestWrappedApplication(AsgiTestBase): def test_mark_span_internal_in_presence_of_span_from_other_framework(self): From c411055b6fe62544b017fb36b85e2e80bd8ca152 Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Sat, 10 Sep 2022 18:49:58 -0300 Subject: [PATCH 2/7] Add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10d6d27c21..0c1183da6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Flask sqlalchemy psycopg2 integration ([#1224](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1224)) +- `opentelemetry-instrumentation-asgi` metrics record target attribute (FastAPI only) + ([#1323](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1323)) ### Fixed From 6bc84c998aee6f670c0ec99f29342b6575e3731e Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Sun, 9 Oct 2022 18:09:08 -0300 Subject: [PATCH 3/7] Update instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py Co-authored-by: Srikanth Chekuri --- .../tests/test_asgi_middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index 04e211bbd3..d5f1616ab8 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -763,7 +763,7 @@ class TestRoute: def test_collect_target_attribute_fastapi_starlette_invalid(self): self.scope["route"] = object() self.assertIsNone( - otel_asgi._collect_target_attribute(self.scope), None + otel_asgi._collect_target_attribute(self.scope), "HTTP_TARGET values is not None" ) From 97b7d0ebe9c249080e96069f1c63b3312d089201 Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Sun, 9 Oct 2022 18:09:32 -0300 Subject: [PATCH 4/7] Update instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py Co-authored-by: Srikanth Chekuri --- .../src/opentelemetry/instrumentation/asgi/__init__.py | 2 -- 1 file changed, 2 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 d28ed2a77e..de3075a0c3 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -384,8 +384,6 @@ def _collect_target_attribute( root_path = scope.get("root_path", "") route = scope.get("route") - if not route: - return None path_format = getattr(route, "path_format", None) if path_format: return f"{root_path}{path_format}" From e3f8de5b51287110b75ebe6272904e0c7275eb08 Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Fri, 21 Oct 2022 16:17:58 -0300 Subject: [PATCH 5/7] format code --- .../tests/test_asgi_middleware.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index 1fa336aa0e..c227b99a99 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -777,7 +777,8 @@ class TestRoute: 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" + otel_asgi._collect_target_attribute(self.scope), + "HTTP_TARGET values is not None", ) From feb46b1d51ea534ff09104bfb878af1322137528 Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Sat, 22 Oct 2022 09:13:09 -0300 Subject: [PATCH 6/7] fix pylint --- .../src/opentelemetry/instrumentation/asgi/__init__.py | 2 ++ .../tests/test_asgi_middleware.py | 2 ++ 2 files changed, 4 insertions(+) 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 76fc76ec9d..b06387e25c 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -413,6 +413,7 @@ class OpenTelemetryMiddleware: the current globally configured one is used. """ + # pylint: disable=too-many-branches def __init__( self, app, @@ -527,6 +528,7 @@ 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) diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index c227b99a99..a74e658438 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py @@ -12,6 +12,8 @@ # 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 From 96782ea1810a5432ad90159134e5e07b4294c499 Mon Sep 17 00:00:00 2001 From: Sebastian Kreft Date: Sat, 22 Oct 2022 09:13:49 -0300 Subject: [PATCH 7/7] format --- .../src/opentelemetry/instrumentation/asgi/__init__.py | 1 + 1 file changed, 1 insertion(+) 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 b06387e25c..dade87c4f4 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -528,6 +528,7 @@ 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):