From dc42bdd986cd136559fda6a3032061d4eed7e517 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Wed, 19 Jun 2024 22:37:09 +0200 Subject: [PATCH 1/6] Add http.route to Django duration metrics --- .../django/middleware/otel_middleware.py | 45 ++++++------------- .../src/opentelemetry/util/http/__init__.py | 1 + 2 files changed, 15 insertions(+), 31 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py index 1b747fd2c0..f9d1e45198 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py @@ -168,23 +168,23 @@ class _DjangoMiddleware(MiddlewareMixin): ) @staticmethod - def _get_span_name(request): + def _get_span_name_and_route(request): + span_name = request.method + route = None try: if getattr(request, "resolver_match"): match = request.resolver_match else: match = resolve(request.path) - if hasattr(match, "route") and match.route: - return f"{request.method} {match.route}" - - if hasattr(match, "url_name") and match.url_name: - return f"{request.method} {match.url_name}" - - return request.method - + if route := getattr(match, "route", None): + span_name += f" {route}" + elif url_name := getattr(match, "url_name", None): + span_name += f" {url_name}" except Resolver404: - return request.method + pass + + return span_name, route # pylint: disable=too-many-locals def process_request(self, request): @@ -213,9 +213,12 @@ def process_request(self, request): collect_request_attributes = wsgi_collect_request_attributes attributes = collect_request_attributes(carrier) + span_name, route = self._get_span_name_and_route(request) + if route: + attributes[SpanAttributes.HTTP_ROUTE] = route span, token = _start_internal_or_server_span( tracer=self._tracer, - span_name=self._get_span_name(request), + span_name=span_name, start_time=request_meta.get( "opentelemetry-instrumentor-django.starttime_key" ), @@ -290,26 +293,6 @@ def process_request(self, request): span, request ) - # pylint: disable=unused-argument - def process_view(self, request, view_func, *args, **kwargs): - # Process view is executed before the view function, here we get the - # route template from request.resolver_match. It is not set yet in process_request - if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): - return - - if ( - self._environ_activation_key in request.META.keys() - and self._environ_span_key in request.META.keys() - ): - span = request.META[self._environ_span_key] - - if span.is_recording(): - match = getattr(request, "resolver_match", None) - if match: - route = getattr(match, "route", None) - if route: - span.set_attribute(SpanAttributes.HTTP_ROUTE, route) - def process_exception(self, request, exception): if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): return diff --git a/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py b/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py index e8a2cf2034..1d3a967c62 100644 --- a/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py +++ b/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py @@ -45,6 +45,7 @@ SpanAttributes.HTTP_STATUS_CODE, SpanAttributes.HTTP_FLAVOR, SpanAttributes.HTTP_SERVER_NAME, + SpanAttributes.HTTP_ROUTE, SpanAttributes.NET_HOST_NAME, SpanAttributes.NET_HOST_PORT, } From 259531b2790f369533d16feb822867b4da2b8815 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 2 Jul 2024 14:24:45 +0200 Subject: [PATCH 2/6] Update fastapi and starlette tests --- .../tests/test_fastapi_instrumentation.py | 1 + .../tests/test_starlette_instrumentation.py | 1 + 2 files changed, 2 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index a7cd5045ee..5aa0956072 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -235,6 +235,7 @@ def test_basic_metric_success(self): "net.host.port": 443, "http.status_code": 200, "http.target": "/foobar", + "http.route": "/foobar", } expected_requests_count_attributes = { "http.method": "GET", diff --git a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py index 1e768982b5..904edbaae2 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py @@ -163,6 +163,7 @@ def test_basic_post_request_metric_success(self): "http.flavor": "1.1", "http.host": "testserver", "http.method": "POST", + "http.route": "/foobar", "http.scheme": "http", "http.server_name": "testserver", "http.status_code": 405, From 6a7c0a15d00670e626219d28250e24878f0e30c1 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 16 Jul 2024 13:00:18 +0200 Subject: [PATCH 3/6] Set http.target instead of http.route --- .../instrumentation/django/middleware/otel_middleware.py | 2 ++ .../tests/test_middleware.py | 3 ++- .../tests/test_fastapi_instrumentation.py | 1 - .../tests/test_starlette_instrumentation.py | 1 - .../src/opentelemetry/util/http/__init__.py | 1 - 5 files changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py index f9d1e45198..6627328bda 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py @@ -231,6 +231,8 @@ def process_request(self, request): attributes ) duration_attrs = _parse_duration_attrs(attributes) + if route: + duration_attrs[SpanAttributes.HTTP_TARGET] = route request.META[self._environ_active_request_attr_key] = ( active_requests_count_attrs diff --git a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py index 63af1e6b86..335182f9e7 100644 --- a/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py @@ -454,7 +454,8 @@ def test_wsgi_metrics(self): ] _recommended_attrs = { "http.server.active_requests": _active_requests_count_attrs, - "http.server.duration": _duration_attrs, + "http.server.duration": _duration_attrs + | {SpanAttributes.HTTP_TARGET}, } start = default_timer() for _ in range(3): diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 79642b3d14..26d9e743a8 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -236,7 +236,6 @@ def test_basic_metric_success(self): "net.host.port": 443, "http.status_code": 200, "http.target": "/foobar", - "http.route": "/foobar", } expected_requests_count_attributes = { "http.method": "GET", diff --git a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py index 904edbaae2..1e768982b5 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py @@ -163,7 +163,6 @@ def test_basic_post_request_metric_success(self): "http.flavor": "1.1", "http.host": "testserver", "http.method": "POST", - "http.route": "/foobar", "http.scheme": "http", "http.server_name": "testserver", "http.status_code": 405, diff --git a/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py b/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py index eb968e1743..f5dacf0fff 100644 --- a/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py +++ b/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py @@ -46,7 +46,6 @@ SpanAttributes.HTTP_STATUS_CODE, SpanAttributes.HTTP_FLAVOR, SpanAttributes.HTTP_SERVER_NAME, - SpanAttributes.HTTP_ROUTE, SpanAttributes.NET_HOST_NAME, SpanAttributes.NET_HOST_PORT, } From 00bf613794ca0428a58f638f1cf3b06860c364b2 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 16 Jul 2024 13:03:21 +0200 Subject: [PATCH 4/6] Revert changes to getting route --- .../django/middleware/otel_middleware.py | 51 +++++++++++++------ 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py index c74e11c08f..da5e8b8c01 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py @@ -168,23 +168,23 @@ class _DjangoMiddleware(MiddlewareMixin): ) @staticmethod - def _get_span_name_and_route(request): - span_name = request.method - route = None + def _get_span_name(request): try: if getattr(request, "resolver_match"): match = request.resolver_match else: match = resolve(request.path) - if route := getattr(match, "route", None): - span_name += f" {route}" - elif url_name := getattr(match, "url_name", None): - span_name += f" {url_name}" - except Resolver404: - pass + if hasattr(match, "route") and match.route: + return f"{request.method} {match.route}" + + if hasattr(match, "url_name") and match.url_name: + return f"{request.method} {match.url_name}" - return span_name, route + return request.method + + except Resolver404: + return request.method # pylint: disable=too-many-locals # pylint: disable=too-many-branches @@ -214,12 +214,9 @@ def process_request(self, request): collect_request_attributes = wsgi_collect_request_attributes attributes = collect_request_attributes(carrier) - span_name, route = self._get_span_name_and_route(request) - if route: - attributes[SpanAttributes.HTTP_ROUTE] = route span, token = _start_internal_or_server_span( tracer=self._tracer, - span_name=span_name, + span_name=self._get_span_name(request), start_time=request_meta.get( "opentelemetry-instrumentor-django.starttime_key" ), @@ -232,8 +229,6 @@ def process_request(self, request): attributes ) duration_attrs = _parse_duration_attrs(attributes) - if route: - duration_attrs[SpanAttributes.HTTP_TARGET] = route request.META[self._environ_active_request_attr_key] = ( active_requests_count_attrs @@ -301,6 +296,30 @@ def process_request(self, request): # would not be called. Log the exception instead. _logger.exception("Exception raised by request_hook") + # pylint: disable=unused-argument + def process_view(self, request, view_func, *args, **kwargs): + # Process view is executed before the view function, here we get the + # route template from request.resolver_match. It is not set yet in process_request + if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): + return + + if ( + self._environ_activation_key in request.META.keys() + and self._environ_span_key in request.META.keys() + ): + span = request.META[self._environ_span_key] + + if span.is_recording(): + match = getattr(request, "resolver_match", None) + if match: + route = getattr(match, "route", None) + if route: + span.set_attribute(SpanAttributes.HTTP_ROUTE, route) + duration_attrs = request.META[ + self._environ_duration_attr_key + ] + duration_attrs[SpanAttributes.HTTP_TARGET] = route + def process_exception(self, request, exception): if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): return From 26d9110b37f2168ecbc68c12febb0d25a9d4e445 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 16 Jul 2024 13:05:49 +0200 Subject: [PATCH 5/6] comment --- .../instrumentation/django/middleware/otel_middleware.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py index da5e8b8c01..11e92c9757 100644 --- a/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py @@ -318,6 +318,8 @@ def process_view(self, request, view_func, *args, **kwargs): duration_attrs = request.META[ self._environ_duration_attr_key ] + # Metrics currently use the 1.11.0 schema, which puts the route in `http.target`. + # TODO: use `http.route` when the user sets `OTEL_SEMCONV_STABILITY_OPT_IN`. duration_attrs[SpanAttributes.HTTP_TARGET] = route def process_exception(self, request, exception): From 6cda892f9e61b2f9b099e9db007884ab5eff7bb0 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 16 Jul 2024 13:42:50 +0200 Subject: [PATCH 6/6] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4573e4b44..57447ccf97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2652](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2652)) - `opentelemetry-instrumentation-aiohttp-client` Implement new semantic convention opt-in migration ([#2673](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2673)) +- `opentelemetry-instrumentation-django` Add `http.target` to Django duration metric attributes + ([#2624](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2624)) ### Breaking changes