From 19dcc140cea5b06becc7b427d4525890db60cfef Mon Sep 17 00:00:00 2001 From: dh Date: Sun, 28 Apr 2024 18:36:14 +0200 Subject: [PATCH 1/7] fix(opentelemetry-instrumentation-asgi): Correct http.target attribute generation even with sub apps (fixes #2476) - modify the instrumentation logic - add unittests on starlette instrumentation - add unittests on fastapi instrumentation --- CHANGELOG.md | 3 + .../instrumentation/asgi/__init__.py | 14 +- .../tests/test_fastapi_instrumentation.py | 155 +++++++++++++++ .../tests/test_starlette_instrumentation.py | 180 +++++++++++++++++- 4 files changed, 349 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50f290300c..9a1874d80f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-dbapi` Fix compatibility with Psycopg3 to extract libpq build version ([#2500](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2500)) +- `opentelemetry-instrumentation-asgi` Fix generation of `http.target` and `http.url` attributes for ASGI apps + using sub apps + ([#2477](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2477)) - `opentelemetry-instrumentation-grpc` AioClientInterceptor should propagate with a Metadata object ([#2363](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2363)) - `opentelemetry-instrumentation-boto3sqs` Instrument Session and resource 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 f24160fcb6..1f84ada47a 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -367,7 +367,19 @@ def get_host_port_url_tuple(scope): server = scope.get("server") or ["0.0.0.0", 80] port = server[1] server_host = server[0] + (":" + str(port) if str(port) != "80" else "") - full_path = scope.get("root_path", "") + scope.get("path", "") + # To get the correct virtual url path within the hosting application (e.g also in a subapplication scenario) + # we have to remove the root_path from the path + # see: + # - https://asgi.readthedocs.io/en/latest/specs/www.html#http-connection-scope (see: root_path and path) + # - https://asgi.readthedocs.io/en/latest/specs/www.html#wsgi-compatibility (see: PATH_INFO) + # PATH_INFO can be derived by stripping root_path from path + # -> that means that the path should contain the root_path already, so prefixing it again is not necessary + # - https://wsgi.readthedocs.io/en/latest/definitions.html#envvar-PATH_INFO + # + # From investigation it seems (that at least for fastapi), the path is already correctly set. That means + # that root_path is already included in the path, so we can use it directly for full path. + # old way: full_path = scope.get("root_path", "") + scope.get("path", "") + full_path = scope.get("path", "") http_url = scope.get("scheme", "http") + "://" + server_host + full_path return server_host, port, http_url diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 26d9e743a8..226fa8e7de 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -11,6 +11,8 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +# pylint: disable=too-many-lines + import unittest from collections.abc import Mapping from timeit import default_timer @@ -319,9 +321,56 @@ def test_metric_uninstrument(self): if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) + def test_sub_app_fastapi_call(self): + """ + This test is to ensure that a span in case of a sub app targeted contains the correct server url + + As this test case covers manual instrumentation, we won't see any additional spans for the sub app. + In this case all generated spans might suffice the requirements for the attributes already + (as the testcase is not setting a root_path for the outer app here) + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + self.assertIn("GET /sub", span.name) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # We expect only one span to have the HTTP attributes set (the SERVER span from the app itself) + # the sub app is not instrumented with manual instrumentation tests. + self.assertEqual(1, len(spans_with_http_attributes)) + + for span in spans_with_http_attributes: + self.assertEqual( + "/sub/home", span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "https://testserver:443/sub/home", + span.attributes[SpanAttributes.HTTP_URL], + ) + @staticmethod def _create_fastapi_app(): app = fastapi.FastAPI() + sub_app = fastapi.FastAPI() + + @sub_app.get("/home") + async def _(): + return {"message": "sub hi"} @app.get("/foobar") async def _(): @@ -339,6 +388,8 @@ async def _(param: str): async def _(): return {"message": "ok"} + app.mount("/sub", app=sub_app) + return app @@ -453,6 +504,58 @@ def tearDown(self): self._instrumentor.uninstrument() super().tearDown() + def test_sub_app_fastapi_call(self): + """ + !!! Attention: we need to override this testcase for the auto-instrumented variant + The reason is, that with auto instrumentation, the sub app is instrumented as well + and therefore we would see the spans for the sub app as well + + This test is to ensure that a span in case of a sub app targeted contains the correct server url + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 6) + + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + # -> the outer app is not aware of the sub_apps internal routes + sub_in = "GET /sub" in span.name + # The sub app spans are named GET /home as from the sub app perspective the request targets /home + # -> the sub app is technically not aware of the /sub prefix + home_in = "GET /home" in span.name + + # We expect the spans to be either from the outer app or the sub app + self.assertTrue( + sub_in or home_in, + f"Span {span.name} does not have /sub or /home in its name", + ) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # We now expect spans with attributes from both the app and its sub app + self.assertEqual(2, len(spans_with_http_attributes)) + + for span in spans_with_http_attributes: + self.assertEqual( + "/sub/home", span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "https://testserver:443/sub/home", + span.attributes[SpanAttributes.HTTP_URL], + ) + class TestAutoInstrumentationHooks(TestFastAPIManualInstrumentationHooks): """ @@ -494,6 +597,58 @@ def tearDown(self): self._instrumentor.uninstrument() super().tearDown() + def test_sub_app_fastapi_call(self): + """ + !!! Attention: we need to override this testcase for the auto-instrumented variant + The reason is, that with auto instrumentation, the sub app is instrumented as well + and therefore we would see the spans for the sub app as well + + This test is to ensure that a span in case of a sub app targeted contains the correct server url + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 6) + + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + # -> the outer app is not aware of the sub_apps internal routes + sub_in = "GET /sub" in span.name + # The sub app spans are named GET /home as from the sub app perspective the request targets /home + # -> the sub app is technically not aware of the /sub prefix + home_in = "GET /home" in span.name + + # We expect the spans to be either from the outer app or the sub app + self.assertTrue( + sub_in or home_in, + f"Span {span.name} does not have /sub or /home in its name", + ) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # We now expect spans with attributes from both the app and its sub app + self.assertEqual(2, len(spans_with_http_attributes)) + + for span in spans_with_http_attributes: + self.assertEqual( + "/sub/home", span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "https://testserver:443/sub/home", + span.attributes[SpanAttributes.HTTP_URL], + ) + class TestAutoInstrumentationLogic(unittest.TestCase): def test_instrumentation(self): diff --git a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py index 1e768982b5..eed1a75c44 100644 --- a/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py @@ -18,7 +18,7 @@ from starlette import applications from starlette.responses import PlainTextResponse -from starlette.routing import Route +from starlette.routing import Mount, Route from starlette.testclient import TestClient from starlette.websockets import WebSocket @@ -103,6 +103,43 @@ def test_basic_starlette_call(self): "opentelemetry.instrumentation.starlette", ) + def test_sub_app_starlette_call(self): + """ + This test is to ensure that a span in case of a sub app targeted contains the correct server url + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + self.assertIn("GET /sub", span.name) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # expect only one span to have the attributes + self.assertEqual(1, len(spans_with_http_attributes)) + + for span in spans_with_http_attributes: + self.assertEqual( + "/sub/home", span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "http://testserver/sub/home", + span.attributes[SpanAttributes.HTTP_URL], + ) + def test_starlette_route_attribute_added(self): """Ensure that starlette routes are used as the span name.""" self._client.get("/user/123") @@ -250,13 +287,20 @@ def home(_): def health(_): return PlainTextResponse("ok") + def sub_home(_): + return PlainTextResponse("sub hi") + + sub_app = applications.Starlette(routes=[Route("/home", sub_home)]) + app = applications.Starlette( routes=[ Route("/foobar", home), Route("/user/{username}", home), Route("/healthzz", health), - ] + Mount("/sub", app=sub_app), + ], ) + return app @@ -354,6 +398,72 @@ def test_uninstrument(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) + def test_sub_app_starlette_call(self): + """ + !!! Attention: we need to override this testcase for the auto-instrumented variant + The reason is, that with auto instrumentation, the sub app is instrumented as well + and therefore we would see the spans for the sub app as well + + This test is to ensure that a span in case of a sub app targeted contains the correct server url + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 6) + + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + # -> the outer app is not aware of the sub_apps internal routes + sub_in = "GET /sub" in span.name + # The sub app spans are named GET /home as from the sub app perspective the request targets /home + # -> the sub app is technically not aware of the /sub prefix + home_in = "GET /home" in span.name + + # We expect the spans to be either from the outer app or the sub app + self.assertTrue( + sub_in or home_in, + f"Span {span.name} does not have /sub or /home in its name", + ) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # We now expect spans with attributes from both the app and its sub app + self.assertEqual(2, len(spans_with_http_attributes)) + + # Due to a potential bug in starlettes handling of sub app mounts, we can + # check only the server kind spans for the correct attributes + # The internal one generated by the sub app is not yet producing the correct attributes + server_span = next( + ( + span + for span in spans_with_http_attributes + if span.kind == SpanKind.SERVER + ), + None, + ) + + self.assertIsNotNone(server_span) + # As soon as the bug is fixed for starlette, we can iterate over spans_with_http_attributes here + # to verify the correctness of the attributes for the internal span as well + self.assertEqual( + "/sub/home", server_span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "http://testserver/sub/home", + server_span.attributes[SpanAttributes.HTTP_URL], + ) + class TestAutoInstrumentationHooks(TestStarletteManualInstrumentationHooks): """ @@ -374,6 +484,72 @@ def tearDown(self): self._instrumentor.uninstrument() super().tearDown() + def test_sub_app_starlette_call(self): + """ + !!! Attention: we need to override this testcase for the auto-instrumented variant + The reason is, that with auto instrumentation, the sub app is instrumented as well + and therefore we would see the spans for the sub app as well + + This test is to ensure that a span in case of a sub app targeted contains the correct server url + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 6) + + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + # -> the outer app is not aware of the sub_apps internal routes + sub_in = "GET /sub" in span.name + # The sub app spans are named GET /home as from the sub app perspective the request targets /home + # -> the sub app is technically not aware of the /sub prefix + home_in = "GET /home" in span.name + + # We expect the spans to be either from the outer app or the sub app + self.assertTrue( + sub_in or home_in, + f"Span {span.name} does not have /sub or /home in its name", + ) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # We now expect spans with attributes from both the app and its sub app + self.assertEqual(2, len(spans_with_http_attributes)) + + # Due to a potential bug in starlettes handling of sub app mounts, we can + # check only the server kind spans for the correct attributes + # The internal one generated by the sub app is not yet producing the correct attributes + server_span = next( + ( + span + for span in spans_with_http_attributes + if span.kind == SpanKind.SERVER + ), + None, + ) + + self.assertIsNotNone(server_span) + # As soon as the bug is fixed for starlette, we can iterate over spans_with_http_attributes here + # to verify the correctness of the attributes for the internal span as well + self.assertEqual( + "/sub/home", server_span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "http://testserver/sub/home", + server_span.attributes[SpanAttributes.HTTP_URL], + ) + class TestAutoInstrumentationLogic(unittest.TestCase): def test_instrumentation(self): From c2bd6f559abe5a3d7f2e878a859268bb2a3d06fa Mon Sep 17 00:00:00 2001 From: dh Date: Tue, 30 Apr 2024 00:08:16 +0200 Subject: [PATCH 2/7] split fastapi instrumentation tests, to prevent pylint to complain (too-many-lines) --- .../tests/test_fastapi_instrumentation.py | 7 - ..._fastapi_instrumentation_custom_headers.py | 357 ++++++++++++++++++ 2 files changed, 357 insertions(+), 7 deletions(-) create mode 100644 instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 226fa8e7de..87a237c4cd 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -11,8 +11,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -# pylint: disable=too-many-lines - import unittest from collections.abc import Mapping from timeit import default_timer @@ -21,7 +19,6 @@ import fastapi from fastapi.middleware.httpsredirect import HTTPSRedirectMiddleware -from fastapi.responses import JSONResponse from fastapi.testclient import TestClient import opentelemetry.instrumentation.fastapi as otel_fastapi @@ -33,12 +30,8 @@ ) from opentelemetry.sdk.resources import Resource from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.test.globals_test import reset_trace_globals from opentelemetry.test.test_base import TestBase from opentelemetry.util.http import ( - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS, - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST, - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE, _active_requests_count_attrs, _duration_attrs, get_excluded_urls, diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py new file mode 100644 index 0000000000..c309e4fe4d --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py @@ -0,0 +1,357 @@ +from unittest.mock import patch + +import fastapi +from starlette.responses import JSONResponse +from starlette.testclient import TestClient + +import opentelemetry.instrumentation.fastapi as otel_fastapi +from opentelemetry import trace +from opentelemetry.test.globals_test import reset_trace_globals +from opentelemetry.test.test_base import TestBase +from opentelemetry.util.http import ( + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS, + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST, + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE, +) + + +@patch.dict( + "os.environ", + { + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS: ".*my-secret.*", + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,Regex-Test-Header-.*,Regex-Invalid-Test-Header-.*,.*my-secret.*", + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,my-custom-regex-header-.*,invalid-regex-header-.*,.*my-secret.*", + }, +) +class TestHTTPAppWithCustomHeaders(TestBase): + def setUp(self): + super().setUp() + self.app = self._create_app() + otel_fastapi.FastAPIInstrumentor().instrument_app(self.app) + self.client = TestClient(self.app) + + def tearDown(self) -> None: + super().tearDown() + with self.disable_logging(): + otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app) + + @staticmethod + def _create_app(): + app = fastapi.FastAPI() + + @app.get("/foobar") + async def _(): + headers = { + "custom-test-header-1": "test-header-value-1", + "custom-test-header-2": "test-header-value-2", + "my-custom-regex-header-1": "my-custom-regex-value-1,my-custom-regex-value-2", + "My-Custom-Regex-Header-2": "my-custom-regex-value-3,my-custom-regex-value-4", + "My-Secret-Header": "My Secret Value", + } + content = {"message": "hello world"} + return JSONResponse(content=content, headers=headers) + + return app + + def test_http_custom_request_headers_in_span_attributes(self): + expected = { + "http.request.header.custom_test_header_1": ( + "test-header-value-1", + ), + "http.request.header.custom_test_header_2": ( + "test-header-value-2", + ), + "http.request.header.regex_test_header_1": ("Regex Test Value 1",), + "http.request.header.regex_test_header_2": ( + "RegexTestValue2,RegexTestValue3", + ), + "http.request.header.my_secret_header": ("[REDACTED]",), + } + resp = self.client.get( + "/foobar", + headers={ + "custom-test-header-1": "test-header-value-1", + "custom-test-header-2": "test-header-value-2", + "Regex-Test-Header-1": "Regex Test Value 1", + "regex-test-header-2": "RegexTestValue2,RegexTestValue3", + "My-Secret-Header": "My Secret Value", + }, + ) + self.assertEqual(200, resp.status_code) + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 3) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + + self.assertSpanHasAttributes(server_span, expected) + + def test_http_custom_request_headers_not_in_span_attributes(self): + not_expected = { + "http.request.header.custom_test_header_3": ( + "test-header-value-3", + ), + } + resp = self.client.get( + "/foobar", + headers={ + "custom-test-header-1": "test-header-value-1", + "custom-test-header-2": "test-header-value-2", + "Regex-Test-Header-1": "Regex Test Value 1", + "regex-test-header-2": "RegexTestValue2,RegexTestValue3", + "My-Secret-Header": "My Secret Value", + }, + ) + self.assertEqual(200, resp.status_code) + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 3) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + + for key, _ in not_expected.items(): + self.assertNotIn(key, server_span.attributes) + + def test_http_custom_response_headers_in_span_attributes(self): + expected = { + "http.response.header.custom_test_header_1": ( + "test-header-value-1", + ), + "http.response.header.custom_test_header_2": ( + "test-header-value-2", + ), + "http.response.header.my_custom_regex_header_1": ( + "my-custom-regex-value-1,my-custom-regex-value-2", + ), + "http.response.header.my_custom_regex_header_2": ( + "my-custom-regex-value-3,my-custom-regex-value-4", + ), + "http.response.header.my_secret_header": ("[REDACTED]",), + } + resp = self.client.get("/foobar") + self.assertEqual(200, resp.status_code) + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 3) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + self.assertSpanHasAttributes(server_span, expected) + + def test_http_custom_response_headers_not_in_span_attributes(self): + not_expected = { + "http.response.header.custom_test_header_3": ( + "test-header-value-3", + ), + } + resp = self.client.get("/foobar") + self.assertEqual(200, resp.status_code) + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 3) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + + for key, _ in not_expected.items(): + self.assertNotIn(key, server_span.attributes) + + +@patch.dict( + "os.environ", + { + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS: ".*my-secret.*", + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,Regex-Test-Header-.*,Regex-Invalid-Test-Header-.*,.*my-secret.*", + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,my-custom-regex-header-.*,invalid-regex-header-.*,.*my-secret.*", + }, +) +class TestWebSocketAppWithCustomHeaders(TestBase): + def setUp(self): + super().setUp() + self.app = self._create_app() + otel_fastapi.FastAPIInstrumentor().instrument_app(self.app) + self.client = TestClient(self.app) + + def tearDown(self) -> None: + super().tearDown() + with self.disable_logging(): + otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app) + + @staticmethod + def _create_app(): + app = fastapi.FastAPI() + + @app.websocket("/foobar_web") + async def _(websocket: fastapi.WebSocket): + message = await websocket.receive() + if message.get("type") == "websocket.connect": + await websocket.send( + { + "type": "websocket.accept", + "headers": [ + (b"custom-test-header-1", b"test-header-value-1"), + (b"custom-test-header-2", b"test-header-value-2"), + (b"Regex-Test-Header-1", b"Regex Test Value 1"), + ( + b"regex-test-header-2", + b"RegexTestValue2,RegexTestValue3", + ), + (b"My-Secret-Header", b"My Secret Value"), + ], + } + ) + await websocket.send_json({"message": "hello world"}) + await websocket.close() + if message.get("type") == "websocket.disconnect": + pass + + return app + + def test_web_socket_custom_request_headers_in_span_attributes(self): + expected = { + "http.request.header.custom_test_header_1": ( + "test-header-value-1", + ), + "http.request.header.custom_test_header_2": ( + "test-header-value-2", + ), + } + + with self.client.websocket_connect( + "/foobar_web", + headers={ + "custom-test-header-1": "test-header-value-1", + "custom-test-header-2": "test-header-value-2", + }, + ) as websocket: + data = websocket.receive_json() + self.assertEqual(data, {"message": "hello world"}) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 5) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + + self.assertSpanHasAttributes(server_span, expected) + + @patch.dict( + "os.environ", + { + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS: ".*my-secret.*", + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,Regex-Test-Header-.*,Regex-Invalid-Test-Header-.*,.*my-secret.*", + }, + ) + def test_web_socket_custom_request_headers_not_in_span_attributes(self): + not_expected = { + "http.request.header.custom_test_header_3": ( + "test-header-value-3", + ), + } + + with self.client.websocket_connect( + "/foobar_web", + headers={ + "custom-test-header-1": "test-header-value-1", + "custom-test-header-2": "test-header-value-2", + }, + ) as websocket: + data = websocket.receive_json() + self.assertEqual(data, {"message": "hello world"}) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 5) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + + for key, _ in not_expected.items(): + self.assertNotIn(key, server_span.attributes) + + def test_web_socket_custom_response_headers_in_span_attributes(self): + expected = { + "http.response.header.custom_test_header_1": ( + "test-header-value-1", + ), + "http.response.header.custom_test_header_2": ( + "test-header-value-2", + ), + } + + with self.client.websocket_connect("/foobar_web") as websocket: + data = websocket.receive_json() + self.assertEqual(data, {"message": "hello world"}) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 5) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + + self.assertSpanHasAttributes(server_span, expected) + + def test_web_socket_custom_response_headers_not_in_span_attributes(self): + not_expected = { + "http.response.header.custom_test_header_3": ( + "test-header-value-3", + ), + } + + with self.client.websocket_connect("/foobar_web") as websocket: + data = websocket.receive_json() + self.assertEqual(data, {"message": "hello world"}) + + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 5) + + server_span = [ + span for span in span_list if span.kind == trace.SpanKind.SERVER + ][0] + + for key, _ in not_expected.items(): + self.assertNotIn(key, server_span.attributes) + + +@patch.dict( + "os.environ", + { + OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3", + }, +) +class TestNonRecordingSpanWithCustomHeaders(TestBase): + def setUp(self): + super().setUp() + self.app = fastapi.FastAPI() + + @self.app.get("/foobar") + async def _(): + return {"message": "hello world"} + + reset_trace_globals() + tracer_provider = trace.NoOpTracerProvider() + trace.set_tracer_provider(tracer_provider=tracer_provider) + + self._instrumentor = otel_fastapi.FastAPIInstrumentor() + self._instrumentor.instrument_app(self.app) + self.client = TestClient(self.app) + + def tearDown(self) -> None: + super().tearDown() + with self.disable_logging(): + self._instrumentor.uninstrument_app(self.app) + + def test_custom_header_not_present_in_non_recording_span(self): + resp = self.client.get( + "/foobar", + headers={ + "custom-test-header-1": "test-header-value-1", + }, + ) + self.assertEqual(200, resp.status_code) + span_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(span_list), 0) From 9c6b06b709ab627acefb676ee5eff4cb465e0874 Mon Sep 17 00:00:00 2001 From: dh Date: Wed, 22 May 2024 00:08:45 +0200 Subject: [PATCH 3/7] code comments adjusted as proposed in review --- .../src/opentelemetry/instrumentation/asgi/__init__.py | 8 +------- 1 file changed, 1 insertion(+), 7 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 1f84ada47a..b3cd9025c7 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -367,18 +367,12 @@ def get_host_port_url_tuple(scope): server = scope.get("server") or ["0.0.0.0", 80] port = server[1] server_host = server[0] + (":" + str(port) if str(port) != "80" else "") - # To get the correct virtual url path within the hosting application (e.g also in a subapplication scenario) - # we have to remove the root_path from the path - # see: + # using the scope path is enough, see: # - https://asgi.readthedocs.io/en/latest/specs/www.html#http-connection-scope (see: root_path and path) # - https://asgi.readthedocs.io/en/latest/specs/www.html#wsgi-compatibility (see: PATH_INFO) # PATH_INFO can be derived by stripping root_path from path # -> that means that the path should contain the root_path already, so prefixing it again is not necessary # - https://wsgi.readthedocs.io/en/latest/definitions.html#envvar-PATH_INFO - # - # From investigation it seems (that at least for fastapi), the path is already correctly set. That means - # that root_path is already included in the path, so we can use it directly for full path. - # old way: full_path = scope.get("root_path", "") + scope.get("path", "") full_path = scope.get("path", "") http_url = scope.get("scheme", "http") + "://" + server_host + full_path return server_host, port, http_url From 1d8a8b742991f7ff8ccb88b05cdbe2e24cad5bf4 Mon Sep 17 00:00:00 2001 From: dh Date: Thu, 27 Jun 2024 13:14:23 +0200 Subject: [PATCH 4/7] refactor structure --- .../tests/test_fastapi_instrumentation.py | 593 ++++-------------- ..._fastapi_instrumentation_custom_headers.py | 42 +- .../test_fastapi_instrumentation_wrapped.py | 64 ++ 3 files changed, 232 insertions(+), 467 deletions(-) create mode 100644 instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_wrapped.py diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 87a237c4cd..debc3d1118 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -12,9 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import unittest -from collections.abc import Mapping from timeit import default_timer -from typing import Tuple from unittest.mock import patch import fastapi @@ -22,7 +20,6 @@ from fastapi.testclient import TestClient import opentelemetry.instrumentation.fastapi as otel_fastapi -from opentelemetry import trace from opentelemetry.instrumentation.asgi import OpenTelemetryMiddleware from opentelemetry.sdk.metrics.export import ( HistogramDataPoint, @@ -57,7 +54,7 @@ } -class TestFastAPIManualInstrumentation(TestBase): +class BaseFastAPI: def _create_app(self): app = self._create_fastapi_app() self._instrumentor.instrument_app( @@ -105,6 +102,137 @@ def tearDown(self): self._instrumentor.uninstrument() self._instrumentor.uninstrument_app(self._app) + @staticmethod + def _create_fastapi_app(): + app = fastapi.FastAPI() + sub_app = fastapi.FastAPI() + + @sub_app.get("/home") + async def _(): + return {"message": "sub hi"} + + @app.get("/foobar") + async def _(): + return {"message": "hello world"} + + @app.get("/user/{username}") + async def _(username: str): + return {"message": username} + + @app.get("/exclude/{param}") + async def _(param: str): + return {"message": param} + + @app.get("/healthzz") + async def _(): + return {"message": "ok"} + + app.mount("/sub", app=sub_app) + + return app + + +class BaseManualFastAPI(BaseFastAPI): + + def test_sub_app_fastapi_call(self): + """ + This test is to ensure that a span in case of a sub app targeted contains the correct server url + + As this test case covers manual instrumentation, we won't see any additional spans for the sub app. + In this case all generated spans might suffice the requirements for the attributes already + (as the testcase is not setting a root_path for the outer app here) + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + self.assertIn("GET /sub", span.name) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # We expect only one span to have the HTTP attributes set (the SERVER span from the app itself) + # the sub app is not instrumented with manual instrumentation tests. + self.assertEqual(1, len(spans_with_http_attributes)) + + for span in spans_with_http_attributes: + self.assertEqual( + "/sub/home", span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "https://testserver:443/sub/home", + span.attributes[SpanAttributes.HTTP_URL], + ) + + +class BaseAutoFastAPI(BaseFastAPI): + + def test_sub_app_fastapi_call(self): + """ + This test is to ensure that a span in case of a sub app targeted contains the correct server url + + As this test case covers auto instrumentation, we will see additional spans for the sub app. + In this case all generated spans might suffice the requirements for the attributes already + (as the testcase is not setting a root_path for the outer app here) + """ + + self._client.get("/sub/home") + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 6) + + for span in spans: + # As we are only looking to the "outer" app, we would see only the "GET /sub" spans + # -> the outer app is not aware of the sub_apps internal routes + sub_in = "GET /sub" in span.name + # The sub app spans are named GET /home as from the sub app perspective the request targets /home + # -> the sub app is technically not aware of the /sub prefix + home_in = "GET /home" in span.name + + # We expect the spans to be either from the outer app or the sub app + self.assertTrue( + sub_in or home_in, + f"Span {span.name} does not have /sub or /home in its name", + ) + + # We now want to specifically test all spans including the + # - HTTP_TARGET + # - HTTP_URL + # attributes to be populated with the expected values + spans_with_http_attributes = [ + span + for span in spans + if ( + SpanAttributes.HTTP_URL in span.attributes + or SpanAttributes.HTTP_TARGET in span.attributes + ) + ] + + # We now expect spans with attributes from both the app and its sub app + self.assertEqual(2, len(spans_with_http_attributes)) + + for span in spans_with_http_attributes: + self.assertEqual( + "/sub/home", span.attributes[SpanAttributes.HTTP_TARGET] + ) + self.assertEqual( + "https://testserver:443/sub/home", + span.attributes[SpanAttributes.HTTP_URL], + ) + + +class TestFastAPIManualInstrumentation(BaseManualFastAPI, TestBase): def test_instrument_app_with_instrument(self): if not isinstance(self, TestAutoInstrumentation): self._instrumentor.instrument() @@ -314,48 +442,6 @@ def test_metric_uninstrument(self): if isinstance(point, NumberDataPoint): self.assertEqual(point.value, 0) - def test_sub_app_fastapi_call(self): - """ - This test is to ensure that a span in case of a sub app targeted contains the correct server url - - As this test case covers manual instrumentation, we won't see any additional spans for the sub app. - In this case all generated spans might suffice the requirements for the attributes already - (as the testcase is not setting a root_path for the outer app here) - """ - - self._client.get("/sub/home") - spans = self.memory_exporter.get_finished_spans() - self.assertEqual(len(spans), 3) - for span in spans: - # As we are only looking to the "outer" app, we would see only the "GET /sub" spans - self.assertIn("GET /sub", span.name) - - # We now want to specifically test all spans including the - # - HTTP_TARGET - # - HTTP_URL - # attributes to be populated with the expected values - spans_with_http_attributes = [ - span - for span in spans - if ( - SpanAttributes.HTTP_URL in span.attributes - or SpanAttributes.HTTP_TARGET in span.attributes - ) - ] - - # We expect only one span to have the HTTP attributes set (the SERVER span from the app itself) - # the sub app is not instrumented with manual instrumentation tests. - self.assertEqual(1, len(spans_with_http_attributes)) - - for span in spans_with_http_attributes: - self.assertEqual( - "/sub/home", span.attributes[SpanAttributes.HTTP_TARGET] - ) - self.assertEqual( - "https://testserver:443/sub/home", - span.attributes[SpanAttributes.HTTP_URL], - ) - @staticmethod def _create_fastapi_app(): app = fastapi.FastAPI() @@ -386,7 +472,7 @@ async def _(): return app -class TestFastAPIManualInstrumentationHooks(TestFastAPIManualInstrumentation): +class TestFastAPIManualInstrumentationHooks(BaseManualFastAPI, TestBase): _server_request_hook = None _client_request_hook = None _client_response_hook = None @@ -436,7 +522,7 @@ def client_response_hook(send_span, scope, message): ) -class TestAutoInstrumentation(TestFastAPIManualInstrumentation): +class TestAutoInstrumentation(BaseAutoFastAPI, TestBase): """Test the auto-instrumented variant Extending the manual instrumentation as most test cases apply @@ -550,7 +636,7 @@ def test_sub_app_fastapi_call(self): ) -class TestAutoInstrumentationHooks(TestFastAPIManualInstrumentationHooks): +class TestAutoInstrumentationHooks(BaseAutoFastAPI, TestBase): """ Test the auto-instrumented variant for request and response hooks @@ -659,412 +745,3 @@ def test_instrumentation(self): should_be_original = fastapi.FastAPI self.assertIs(original, should_be_original) - - -class TestWrappedApplication(TestBase): - def setUp(self): - super().setUp() - - self.app = fastapi.FastAPI() - - @self.app.get("/foobar") - async def _(): - return {"message": "hello world"} - - otel_fastapi.FastAPIInstrumentor().instrument_app(self.app) - self.client = TestClient(self.app) - self.tracer = self.tracer_provider.get_tracer(__name__) - - def tearDown(self) -> None: - super().tearDown() - with self.disable_logging(): - otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app) - - def test_mark_span_internal_in_presence_of_span_from_other_framework(self): - with self.tracer.start_as_current_span( - "test", kind=trace.SpanKind.SERVER - ) as parent_span: - resp = self.client.get("/foobar") - self.assertEqual(200, resp.status_code) - - span_list = self.memory_exporter.get_finished_spans() - for span in span_list: - print(str(span.__class__) + ": " + str(span.__dict__)) - - # there should be 4 spans - single SERVER "test" and three INTERNAL "FastAPI" - self.assertEqual(trace.SpanKind.INTERNAL, span_list[0].kind) - self.assertEqual(trace.SpanKind.INTERNAL, span_list[1].kind) - # main INTERNAL span - child of test - self.assertEqual(trace.SpanKind.INTERNAL, span_list[2].kind) - self.assertEqual( - parent_span.context.span_id, span_list[2].parent.span_id - ) - # SERVER "test" - self.assertEqual(trace.SpanKind.SERVER, span_list[3].kind) - self.assertEqual( - parent_span.context.span_id, span_list[3].context.span_id - ) - - -class MultiMapping(Mapping): - - def __init__(self, *items: Tuple[str, str]): - self._items = items - - def __len__(self): - return len(self._items) - - def __getitem__(self, __key): - raise NotImplementedError("use .items() instead") - - def __iter__(self): - raise NotImplementedError("use .items() instead") - - def items(self): - return self._items - - -@patch.dict( - "os.environ", - { - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS: ".*my-secret.*", - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,Regex-Test-Header-.*,Regex-Invalid-Test-Header-.*,.*my-secret.*", - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,my-custom-regex-header-.*,invalid-regex-header-.*,.*my-secret.*", - }, -) -class TestHTTPAppWithCustomHeaders(TestBase): - def setUp(self): - super().setUp() - self.app = self._create_app() - otel_fastapi.FastAPIInstrumentor().instrument_app(self.app) - self.client = TestClient(self.app) - - def tearDown(self) -> None: - super().tearDown() - with self.disable_logging(): - otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app) - - @staticmethod - def _create_app(): - app = fastapi.FastAPI() - - @app.get("/foobar") - async def _(): - headers = MultiMapping( - ("custom-test-header-1", "test-header-value-1"), - ("custom-test-header-2", "test-header-value-2"), - ("my-custom-regex-header-1", "my-custom-regex-value-1"), - ("my-custom-regex-header-1", "my-custom-regex-value-2"), - ("My-Custom-Regex-Header-2", "my-custom-regex-value-3"), - ("My-Custom-Regex-Header-2", "my-custom-regex-value-4"), - ("My-Secret-Header", "My Secret Value"), - ) - content = {"message": "hello world"} - return JSONResponse(content=content, headers=headers) - - return app - - def test_http_custom_request_headers_in_span_attributes(self): - expected = { - "http.request.header.custom_test_header_1": ( - "test-header-value-1", - ), - "http.request.header.custom_test_header_2": ( - "test-header-value-2", - ), - "http.request.header.regex_test_header_1": ("Regex Test Value 1",), - "http.request.header.regex_test_header_2": ( - "RegexTestValue2,RegexTestValue3", - ), - "http.request.header.my_secret_header": ("[REDACTED]",), - } - resp = self.client.get( - "/foobar", - headers={ - "custom-test-header-1": "test-header-value-1", - "custom-test-header-2": "test-header-value-2", - "Regex-Test-Header-1": "Regex Test Value 1", - "regex-test-header-2": "RegexTestValue2,RegexTestValue3", - "My-Secret-Header": "My Secret Value", - }, - ) - self.assertEqual(200, resp.status_code) - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 3) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - - self.assertSpanHasAttributes(server_span, expected) - - def test_http_custom_request_headers_not_in_span_attributes(self): - not_expected = { - "http.request.header.custom_test_header_3": ( - "test-header-value-3", - ), - } - resp = self.client.get( - "/foobar", - headers={ - "custom-test-header-1": "test-header-value-1", - "custom-test-header-2": "test-header-value-2", - "Regex-Test-Header-1": "Regex Test Value 1", - "regex-test-header-2": "RegexTestValue2,RegexTestValue3", - "My-Secret-Header": "My Secret Value", - }, - ) - self.assertEqual(200, resp.status_code) - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 3) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - - for key, _ in not_expected.items(): - self.assertNotIn(key, server_span.attributes) - - def test_http_custom_response_headers_in_span_attributes(self): - expected = { - "http.response.header.custom_test_header_1": ( - "test-header-value-1", - ), - "http.response.header.custom_test_header_2": ( - "test-header-value-2", - ), - "http.response.header.my_custom_regex_header_1": ( - "my-custom-regex-value-1", - "my-custom-regex-value-2", - ), - "http.response.header.my_custom_regex_header_2": ( - "my-custom-regex-value-3", - "my-custom-regex-value-4", - ), - "http.response.header.my_secret_header": ("[REDACTED]",), - } - resp = self.client.get("/foobar") - self.assertEqual(200, resp.status_code) - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 3) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - self.assertSpanHasAttributes(server_span, expected) - - def test_http_custom_response_headers_not_in_span_attributes(self): - not_expected = { - "http.response.header.custom_test_header_3": ( - "test-header-value-3", - ), - } - resp = self.client.get("/foobar") - self.assertEqual(200, resp.status_code) - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 3) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - - for key, _ in not_expected.items(): - self.assertNotIn(key, server_span.attributes) - - -@patch.dict( - "os.environ", - { - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS: ".*my-secret.*", - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,Regex-Test-Header-.*,Regex-Invalid-Test-Header-.*,.*my-secret.*", - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,my-custom-regex-header-.*,invalid-regex-header-.*,.*my-secret.*", - }, -) -class TestWebSocketAppWithCustomHeaders(TestBase): - def setUp(self): - super().setUp() - self.app = self._create_app() - otel_fastapi.FastAPIInstrumentor().instrument_app(self.app) - self.client = TestClient(self.app) - - def tearDown(self) -> None: - super().tearDown() - with self.disable_logging(): - otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app) - - @staticmethod - def _create_app(): - app = fastapi.FastAPI() - - @app.websocket("/foobar_web") - async def _(websocket: fastapi.WebSocket): - message = await websocket.receive() - if message.get("type") == "websocket.connect": - await websocket.send( - { - "type": "websocket.accept", - "headers": [ - (b"custom-test-header-1", b"test-header-value-1"), - (b"custom-test-header-2", b"test-header-value-2"), - (b"Regex-Test-Header-1", b"Regex Test Value 1"), - ( - b"regex-test-header-2", - b"RegexTestValue2,RegexTestValue3", - ), - (b"My-Secret-Header", b"My Secret Value"), - ], - } - ) - await websocket.send_json({"message": "hello world"}) - await websocket.close() - if message.get("type") == "websocket.disconnect": - pass - - return app - - def test_web_socket_custom_request_headers_in_span_attributes(self): - expected = { - "http.request.header.custom_test_header_1": ( - "test-header-value-1", - ), - "http.request.header.custom_test_header_2": ( - "test-header-value-2", - ), - } - - with self.client.websocket_connect( - "/foobar_web", - headers={ - "custom-test-header-1": "test-header-value-1", - "custom-test-header-2": "test-header-value-2", - }, - ) as websocket: - data = websocket.receive_json() - self.assertEqual(data, {"message": "hello world"}) - - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 5) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - - self.assertSpanHasAttributes(server_span, expected) - - @patch.dict( - "os.environ", - { - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS: ".*my-secret.*", - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3,Regex-Test-Header-.*,Regex-Invalid-Test-Header-.*,.*my-secret.*", - }, - ) - def test_web_socket_custom_request_headers_not_in_span_attributes(self): - not_expected = { - "http.request.header.custom_test_header_3": ( - "test-header-value-3", - ), - } - - with self.client.websocket_connect( - "/foobar_web", - headers={ - "custom-test-header-1": "test-header-value-1", - "custom-test-header-2": "test-header-value-2", - }, - ) as websocket: - data = websocket.receive_json() - self.assertEqual(data, {"message": "hello world"}) - - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 5) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - - for key, _ in not_expected.items(): - self.assertNotIn(key, server_span.attributes) - - def test_web_socket_custom_response_headers_in_span_attributes(self): - expected = { - "http.response.header.custom_test_header_1": ( - "test-header-value-1", - ), - "http.response.header.custom_test_header_2": ( - "test-header-value-2", - ), - } - - with self.client.websocket_connect("/foobar_web") as websocket: - data = websocket.receive_json() - self.assertEqual(data, {"message": "hello world"}) - - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 5) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - - self.assertSpanHasAttributes(server_span, expected) - - def test_web_socket_custom_response_headers_not_in_span_attributes(self): - not_expected = { - "http.response.header.custom_test_header_3": ( - "test-header-value-3", - ), - } - - with self.client.websocket_connect("/foobar_web") as websocket: - data = websocket.receive_json() - self.assertEqual(data, {"message": "hello world"}) - - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 5) - - server_span = [ - span for span in span_list if span.kind == trace.SpanKind.SERVER - ][0] - - for key, _ in not_expected.items(): - self.assertNotIn(key, server_span.attributes) - - -@patch.dict( - "os.environ", - { - OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST: "Custom-Test-Header-1,Custom-Test-Header-2,Custom-Test-Header-3", - }, -) -class TestNonRecordingSpanWithCustomHeaders(TestBase): - def setUp(self): - super().setUp() - self.app = fastapi.FastAPI() - - @self.app.get("/foobar") - async def _(): - return {"message": "hello world"} - - reset_trace_globals() - tracer_provider = trace.NoOpTracerProvider() - trace.set_tracer_provider(tracer_provider=tracer_provider) - - self._instrumentor = otel_fastapi.FastAPIInstrumentor() - self._instrumentor.instrument_app(self.app) - self.client = TestClient(self.app) - - def tearDown(self) -> None: - super().tearDown() - with self.disable_logging(): - self._instrumentor.uninstrument_app(self.app) - - def test_custom_header_not_present_in_non_recording_span(self): - resp = self.client.get( - "/foobar", - headers={ - "custom-test-header-1": "test-header-value-1", - }, - ) - self.assertEqual(200, resp.status_code) - span_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(span_list), 0) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py index c309e4fe4d..e7adca735c 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_custom_headers.py @@ -1,3 +1,5 @@ +from collections.abc import Mapping +from typing import Tuple from unittest.mock import patch import fastapi @@ -15,6 +17,24 @@ ) +class MultiMapping(Mapping): + + def __init__(self, *items: Tuple[str, str]): + self._items = items + + def __len__(self): + return len(self._items) + + def __getitem__(self, __key): + raise NotImplementedError("use .items() instead") + + def __iter__(self): + raise NotImplementedError("use .items() instead") + + def items(self): + return self._items + + @patch.dict( "os.environ", { @@ -41,13 +61,15 @@ def _create_app(): @app.get("/foobar") async def _(): - headers = { - "custom-test-header-1": "test-header-value-1", - "custom-test-header-2": "test-header-value-2", - "my-custom-regex-header-1": "my-custom-regex-value-1,my-custom-regex-value-2", - "My-Custom-Regex-Header-2": "my-custom-regex-value-3,my-custom-regex-value-4", - "My-Secret-Header": "My Secret Value", - } + headers = MultiMapping( + ("custom-test-header-1", "test-header-value-1"), + ("custom-test-header-2", "test-header-value-2"), + ("my-custom-regex-header-1", "my-custom-regex-value-1"), + ("my-custom-regex-header-1", "my-custom-regex-value-2"), + ("My-Custom-Regex-Header-2", "my-custom-regex-value-3"), + ("My-Custom-Regex-Header-2", "my-custom-regex-value-4"), + ("My-Secret-Header", "My Secret Value"), + ) content = {"message": "hello world"} return JSONResponse(content=content, headers=headers) @@ -123,10 +145,12 @@ def test_http_custom_response_headers_in_span_attributes(self): "test-header-value-2", ), "http.response.header.my_custom_regex_header_1": ( - "my-custom-regex-value-1,my-custom-regex-value-2", + "my-custom-regex-value-1", + "my-custom-regex-value-2", ), "http.response.header.my_custom_regex_header_2": ( - "my-custom-regex-value-3,my-custom-regex-value-4", + "my-custom-regex-value-3", + "my-custom-regex-value-4", ), "http.response.header.my_secret_header": ("[REDACTED]",), } diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_wrapped.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_wrapped.py new file mode 100644 index 0000000000..0b17173ac6 --- /dev/null +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation_wrapped.py @@ -0,0 +1,64 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import fastapi +from starlette.testclient import TestClient + +import opentelemetry.instrumentation.fastapi as otel_fastapi +from opentelemetry import trace +from opentelemetry.test.test_base import TestBase + + +class TestWrappedApplication(TestBase): + def setUp(self): + super().setUp() + + self.app = fastapi.FastAPI() + + @self.app.get("/foobar") + async def _(): + return {"message": "hello world"} + + otel_fastapi.FastAPIInstrumentor().instrument_app(self.app) + self.client = TestClient(self.app) + self.tracer = self.tracer_provider.get_tracer(__name__) + + def tearDown(self) -> None: + super().tearDown() + with self.disable_logging(): + otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app) + + def test_mark_span_internal_in_presence_of_span_from_other_framework(self): + with self.tracer.start_as_current_span( + "test", kind=trace.SpanKind.SERVER + ) as parent_span: + resp = self.client.get("/foobar") + self.assertEqual(200, resp.status_code) + + span_list = self.memory_exporter.get_finished_spans() + for span in span_list: + print(str(span.__class__) + ": " + str(span.__dict__)) + + # there should be 4 spans - single SERVER "test" and three INTERNAL "FastAPI" + self.assertEqual(trace.SpanKind.INTERNAL, span_list[0].kind) + self.assertEqual(trace.SpanKind.INTERNAL, span_list[1].kind) + # main INTERNAL span - child of test + self.assertEqual(trace.SpanKind.INTERNAL, span_list[2].kind) + self.assertEqual( + parent_span.context.span_id, span_list[2].parent.span_id + ) + # SERVER "test" + self.assertEqual(trace.SpanKind.SERVER, span_list[3].kind) + self.assertEqual( + parent_span.context.span_id, span_list[3].context.span_id + ) From 12032172780ee10561d5ae0e255c4312b9ed61c1 Mon Sep 17 00:00:00 2001 From: dh Date: Thu, 27 Jun 2024 14:20:53 +0200 Subject: [PATCH 5/7] move changelog to expected location --- CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a1874d80f..8fd9235c09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2612](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2612)) - `opentelemetry-instrumentation-system-metrics` Permit to use psutil 6.0+. ([#2630](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2630)) +- `opentelemetry-instrumentation-asgi` Fix generation of `http.target` and `http.url` attributes for ASGI apps + using sub apps + ([#2477](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2477)) + ### Added @@ -90,9 +94,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-dbapi` Fix compatibility with Psycopg3 to extract libpq build version ([#2500](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2500)) -- `opentelemetry-instrumentation-asgi` Fix generation of `http.target` and `http.url` attributes for ASGI apps - using sub apps - ([#2477](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2477)) - `opentelemetry-instrumentation-grpc` AioClientInterceptor should propagate with a Metadata object ([#2363](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2363)) - `opentelemetry-instrumentation-boto3sqs` Instrument Session and resource From fff03f312d98112bcdab0c1412f125844f6958a8 Mon Sep 17 00:00:00 2001 From: dh Date: Thu, 27 Jun 2024 14:44:51 +0200 Subject: [PATCH 6/7] fix issue with test inheritance and linting --- .../tests/test_fastapi_instrumentation.py | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index debc3d1118..839c227645 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -54,7 +54,7 @@ } -class BaseFastAPI: +class TestBaseFastAPI(TestBase): def _create_app(self): app = self._create_fastapi_app() self._instrumentor.instrument_app( @@ -77,6 +77,15 @@ def _create_app_explicit_excluded_urls(self): ) return app + @classmethod + def setUpClass(cls): + if cls is TestBaseFastAPI: + raise unittest.SkipTest( + f"{cls.__name__} is an abstract base class" + ) + + super(TestBaseFastAPI, cls).setUpClass() + def setUp(self): super().setUp() self.env_patch = patch.dict( @@ -132,7 +141,16 @@ async def _(): return app -class BaseManualFastAPI(BaseFastAPI): +class TestBaseManualFastAPI(TestBaseFastAPI): + + @classmethod + def setUpClass(cls): + if cls is TestBaseManualFastAPI: + raise unittest.SkipTest( + f"{cls.__name__} is an abstract base class" + ) + + super(TestBaseManualFastAPI, cls).setUpClass() def test_sub_app_fastapi_call(self): """ @@ -177,7 +195,16 @@ def test_sub_app_fastapi_call(self): ) -class BaseAutoFastAPI(BaseFastAPI): +class TestBaseAutoFastAPI(TestBaseFastAPI): + + @classmethod + def setUpClass(cls): + if cls is TestBaseAutoFastAPI: + raise unittest.SkipTest( + f"{cls.__name__} is an abstract base class" + ) + + super(TestBaseAutoFastAPI, cls).setUpClass() def test_sub_app_fastapi_call(self): """ @@ -232,7 +259,7 @@ def test_sub_app_fastapi_call(self): ) -class TestFastAPIManualInstrumentation(BaseManualFastAPI, TestBase): +class TestFastAPIManualInstrumentation(TestBaseManualFastAPI, TestBase): def test_instrument_app_with_instrument(self): if not isinstance(self, TestAutoInstrumentation): self._instrumentor.instrument() @@ -472,7 +499,7 @@ async def _(): return app -class TestFastAPIManualInstrumentationHooks(BaseManualFastAPI, TestBase): +class TestFastAPIManualInstrumentationHooks(TestBaseManualFastAPI, TestBase): _server_request_hook = None _client_request_hook = None _client_response_hook = None @@ -522,7 +549,7 @@ def client_response_hook(send_span, scope, message): ) -class TestAutoInstrumentation(BaseAutoFastAPI, TestBase): +class TestAutoInstrumentation(TestBaseAutoFastAPI, TestBase): """Test the auto-instrumented variant Extending the manual instrumentation as most test cases apply @@ -636,7 +663,7 @@ def test_sub_app_fastapi_call(self): ) -class TestAutoInstrumentationHooks(BaseAutoFastAPI, TestBase): +class TestAutoInstrumentationHooks(TestBaseAutoFastAPI, TestBase): """ Test the auto-instrumented variant for request and response hooks From 71f2dbf9ba2865705dfb66e2bf3e312c3ef0665a Mon Sep 17 00:00:00 2001 From: dh Date: Fri, 28 Jun 2024 17:30:41 +0200 Subject: [PATCH 7/7] remove non-required multi inheritance --- .../tests/test_fastapi_instrumentation.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 839c227645..0ad63164d5 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -259,7 +259,7 @@ def test_sub_app_fastapi_call(self): ) -class TestFastAPIManualInstrumentation(TestBaseManualFastAPI, TestBase): +class TestFastAPIManualInstrumentation(TestBaseManualFastAPI): def test_instrument_app_with_instrument(self): if not isinstance(self, TestAutoInstrumentation): self._instrumentor.instrument() @@ -499,7 +499,7 @@ async def _(): return app -class TestFastAPIManualInstrumentationHooks(TestBaseManualFastAPI, TestBase): +class TestFastAPIManualInstrumentationHooks(TestBaseManualFastAPI): _server_request_hook = None _client_request_hook = None _client_response_hook = None @@ -549,7 +549,7 @@ def client_response_hook(send_span, scope, message): ) -class TestAutoInstrumentation(TestBaseAutoFastAPI, TestBase): +class TestAutoInstrumentation(TestBaseAutoFastAPI): """Test the auto-instrumented variant Extending the manual instrumentation as most test cases apply @@ -663,7 +663,7 @@ def test_sub_app_fastapi_call(self): ) -class TestAutoInstrumentationHooks(TestBaseAutoFastAPI, TestBase): +class TestAutoInstrumentationHooks(TestBaseAutoFastAPI): """ Test the auto-instrumented variant for request and response hooks