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

Author: dhofstetter

Diff for: 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 for: 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 for: 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):