diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fae8763b7..8797b9992a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2297](https://github.com/open-telemetry/opentelemetry-python-contrib/issues/2297)) - Ensure all http.server.duration metrics have the same description ([#2151](https://github.com/open-telemetry/opentelemetry-python-contrib/issues/2298)) +- Avoid losing repeated HTTP headers + ([#2266](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2266)) ## Version 1.23.0/0.44b0 (2024-02-23) 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 d179f4fafa..d28d85f975 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -195,7 +195,7 @@ def client_response_hook(span: Span, message: dict): import urllib from functools import wraps from timeit import default_timer -from typing import Any, Awaitable, Callable, Tuple, cast +from typing import Any, Awaitable, Callable, Tuple from asgiref.compatibility import guarantee_single_callable @@ -347,11 +347,17 @@ def collect_custom_headers_attributes( - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-request-and-response-headers """ # Decode headers before processing. - headers: dict[str, str] = { - _key.decode("utf8"): _value.decode("utf8") - for (_key, _value) in scope_or_response_message.get("headers") - or cast("list[tuple[bytes, bytes]]", []) - } + headers: dict[str, str] = {} + raw_headers = scope_or_response_message.get("headers") + if raw_headers: + for _key, _value in raw_headers: + key = _key.decode().lower() + value = _value.decode() + if key in headers: + headers[key] += f",{value}" + else: + headers[key] = value + return sanitize.sanitize_header_values( headers, header_regexes, diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py index c50839f72f..7a9c91a3e7 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py @@ -40,6 +40,24 @@ async def http_app_with_custom_headers(scope, receive, send): await send({"type": "http.response.body", "body": b"*"}) +async def http_app_with_repeat_headers(scope, receive, send): + message = await receive() + assert scope["type"] == "http" + if message.get("type") == "http.request": + await send( + { + "type": "http.response.start", + "status": 200, + "headers": [ + (b"Content-Type", b"text/plain"), + (b"custom-test-header-1", b"test-header-value-1"), + (b"custom-test-header-1", b"test-header-value-2"), + ], + } + ) + await send({"type": "http.response.body", "body": b"*"}) + + async def websocket_app_with_custom_headers(scope, receive, send): assert scope["type"] == "websocket" while True: @@ -121,6 +139,25 @@ def test_http_custom_request_headers_in_span_attributes(self): if span.kind == SpanKind.SERVER: self.assertSpanHasAttributes(span, expected) + def test_http_repeat_request_headers_in_span_attributes(self): + self.scope["headers"].extend( + [ + (b"custom-test-header-1", b"test-header-value-1"), + (b"custom-test-header-1", b"test-header-value-2"), + ] + ) + self.seed_app(self.app) + self.send_default_request() + self.get_all_output() + span_list = self.exporter.get_finished_spans() + expected = { + "http.request.header.custom_test_header_1": ( + "test-header-value-1,test-header-value-2", + ), + } + span = next(span for span in span_list if span.kind == SpanKind.SERVER) + self.assertSpanHasAttributes(span, expected) + def test_http_custom_request_headers_not_in_span_attributes(self): self.scope["headers"].extend( [ @@ -176,6 +213,24 @@ def test_http_custom_response_headers_in_span_attributes(self): if span.kind == SpanKind.SERVER: self.assertSpanHasAttributes(span, expected) + def test_http_repeat_response_headers_in_span_attributes(self): + self.app = otel_asgi.OpenTelemetryMiddleware( + http_app_with_repeat_headers, + tracer_provider=self.tracer_provider, + **self.constructor_params, + ) + self.seed_app(self.app) + self.send_default_request() + self.get_all_output() + span_list = self.exporter.get_finished_spans() + expected = { + "http.response.header.custom_test_header_1": ( + "test-header-value-1,test-header-value-2", + ), + } + span = next(span for span in span_list if span.kind == SpanKind.SERVER) + self.assertSpanHasAttributes(span, expected) + def test_http_custom_response_headers_not_in_span_attributes(self): self.app = otel_asgi.OpenTelemetryMiddleware( http_app_with_custom_headers, diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py b/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py index 6117521bb9..3c8073f261 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py @@ -88,6 +88,14 @@ def _custom_response_headers(): resp.headers["my-secret-header"] = "my-secret-value" return resp + @staticmethod + def _repeat_custom_response_headers(): + headers = { + "content-type": "text/plain; charset=utf-8", + "my-custom-header": ["my-custom-value-1", "my-custom-header-2"], + } + return flask.Response("test response", headers=headers) + def _common_initialization(self): def excluded_endpoint(): return "excluded" @@ -106,6 +114,9 @@ def excluded2_endpoint(): self.app.route("/test_custom_response_headers")( self._custom_response_headers ) + self.app.route("/test_repeat_custom_response_headers")( + self._repeat_custom_response_headers + ) # pylint: disable=attribute-defined-outside-init self.client = Client(self.app, Response) diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py index 3bd2f74ba8..dec265907f 100644 --- a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py +++ b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py @@ -671,6 +671,22 @@ def test_custom_request_header_added_in_server_span(self): self.assertEqual(span.kind, trace.SpanKind.SERVER) self.assertSpanHasAttributes(span, expected) + def test_repeat_custom_request_header_added_in_server_span(self): + headers = [ + ("Custom-Test-Header-1", "Test Value 1"), + ("Custom-Test-Header-1", "Test Value 2"), + ] + resp = self.client.get("/hello/123", headers=headers) + self.assertEqual(200, resp.status_code) + span = self.memory_exporter.get_finished_spans()[0] + expected = { + "http.request.header.custom_test_header_1": ( + "Test Value 1, Test Value 2", + ), + } + self.assertEqual(span.kind, trace.SpanKind.SERVER) + self.assertSpanHasAttributes(span, expected) + def test_custom_request_header_not_added_in_internal_span(self): tracer = trace.get_tracer(__name__) with tracer.start_as_current_span("test", kind=trace.SpanKind.SERVER): @@ -724,6 +740,21 @@ def test_custom_response_header_added_in_server_span(self): self.assertEqual(span.kind, trace.SpanKind.SERVER) self.assertSpanHasAttributes(span, expected) + def test_repeat_custom_response_header_added_in_server_span(self): + resp = self.client.get("/test_repeat_custom_response_headers") + self.assertEqual(resp.status_code, 200) + span = self.memory_exporter.get_finished_spans()[0] + expected = { + "http.response.header.content_type": ( + "text/plain; charset=utf-8", + ), + "http.response.header.my_custom_header": ( + "my-custom-value-1,my-custom-header-2", + ), + } + self.assertEqual(span.kind, trace.SpanKind.SERVER) + self.assertSpanHasAttributes(span, expected) + def test_custom_response_header_not_added_in_internal_span(self): tracer = trace.get_tracer(__name__) with tracer.start_as_current_span("test", kind=trace.SpanKind.SERVER): diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py b/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py index afa1bf2a55..50d4f03dff 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py @@ -358,7 +358,6 @@ def collect_custom_request_headers_attributes(environ): OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS ) ) - headers = { key[_CARRIER_KEY_PREFIX_LEN:].replace("_", "-"): val for key, val in environ.items() @@ -387,7 +386,12 @@ def collect_custom_response_headers_attributes(response_headers): ) response_headers_dict = {} if response_headers: - response_headers_dict = dict(response_headers) + for key, val in response_headers: + key = key.lower() + if key in response_headers_dict: + response_headers_dict[key] += "," + val + else: + response_headers_dict[key] = val return sanitize.sanitize_header_values( response_headers_dict, diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py index d71e584ca8..f74dd67867 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py +++ b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py @@ -115,6 +115,18 @@ def wsgi_with_custom_response_headers(environ, start_response): return [b"*"] +def wsgi_with_repeat_custom_response_headers(environ, start_response): + assert isinstance(environ, dict) + start_response( + "200 OK", + [ + ("my-custom-header", "my-custom-value-1"), + ("my-custom-header", "my-custom-value-2"), + ], + ) + return [b"*"] + + _expected_metric_names = [ "http.server.active_requests", "http.server.duration", @@ -711,6 +723,26 @@ def test_custom_response_headers_not_added_in_internal_span(self): for key, _ in not_expected.items(): self.assertNotIn(key, span.attributes) + @mock.patch.dict( + "os.environ", + { + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE: "my-custom-header", + }, + ) + def test_repeat_custom_response_headers_added_in_server_span(self): + app = otel_wsgi.OpenTelemetryMiddleware( + wsgi_with_repeat_custom_response_headers + ) + response = app(self.environ, self.start_response) + self.iterate_response(response) + span = self.memory_exporter.get_finished_spans()[0] + expected = { + "http.response.header.my_custom_header": ( + "my-custom-value-1,my-custom-value-2", + ), + } + self.assertSpanHasAttributes(span, expected) + if __name__ == "__main__": unittest.main()