diff --git a/CHANGELOG.md b/CHANGELOG.md index ceb97433f1..1ac77bad8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `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 405c470ceb..fea264137a 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py @@ -370,7 +370,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 4269dfa2e4..38cc8d6100 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -11,6 +11,7 @@ # 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 timeit import default_timer @@ -310,9 +311,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 _(): @@ -330,6 +378,8 @@ async def _(param: str): async def _(): return {"message": "ok"} + app.mount("/sub", app=sub_app) + return app @@ -444,6 +494,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): """ @@ -485,6 +587,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 3784672fb5..9d95f51b2d 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 @@ -99,6 +99,43 @@ def test_basic_starlette_call(self): for span in spans: self.assertIn("GET /foobar", span.name) + 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") @@ -242,13 +279,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 @@ -346,6 +390,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): """ @@ -366,6 +476,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):