Skip to content

fix(asgi): resolve path template for routers added with include_router - #907

Open
adityaanikam wants to merge 1 commit into
instana:mainfrom
adityaanikam:906-asgi-included-router-path-tpl
Open

adityaanikam wants to merge 1 commit into
instana:mainfrom
adityaanikam:906-asgi-included-router-path-tpl

Conversation

@adityaanikam

@adityaanikam adityaanikam commented Sep 28, 2026 •

Copy link
Copy Markdown

Fixes #906

With FastAPI 0.137 and later, routers added with include_router() are kept in a lazy _IncludedRouter wrapper that has no path attribute. InstanaASGIMiddleware._collect_kvs read route.path for the matched route, so it raised AttributeError for every request to those endpoints. The exception is swallowed and logged at debug level, and http.path_tpl was never set, so those endpoints lost their path template.

This flattens the app's routes before matching, with a new _iter_routes helper. It uses FastAPI's public iter_route_contexts() (0.137.2 and later), falls back to effective_route_contexts() on the wrapper for 0.137.0 and 0.137.1, and leaves older FastAPI versions and plain Starlette apps unchanged, since their routes are already flat. Each flattened route carries its full templated path, including every router prefix, so routers nested several levels deep are covered. The private _match() is no longer used. Match and iter_route_contexts are imported at the top of the module behind ImportError guards, so Starlette and FastAPI stay optional and instana.middleware still imports without them.

Testing: added test_path_templates_with_included_routers with a single level and a nested include_router() route, and test_iter_routes_without_iter_route_contexts for the fallback branches. The two router cases fail on the unmodified code (no path_tpl on the span) and pass with the change. Ran tests/frameworks/test_fastapi.py against FastAPI 0.136.1, 0.137.0, 0.137.1 and 0.141.1 (14 passed each), and test_fastapi_middleware.py, test_starlette.py and test_starlette_middleware.py with 0.141.1 (12 passed). ruff check is clean on the changed files, and ruff format --check on asgi.py and test_fastapi.py.

@adityaanikam
adityaanikam requested a review from a team as a code owner September 28, 2026 10:42
Copilot AI lite review requested due to automatic review settings September 28, 2026 10:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@CagriYonca CagriYonca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, then it looks good

Comment thread src/instana/instrumentation/asgi.py Outdated
Comment thread src/instana/instrumentation/asgi.py Outdated
@@ -55,7 +71,9 @@ def _collect_kvs(self, scope: Dict[str, Any], span: "InstanaSpan") -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed after this

@GSVarsha GSVarsha left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adityaanikam,

Thanks for working on this bug!

I would suggest using the public helper iter_route_contexts in fastapi.routing introduced in FastAPI ≥ 0.137.2 that flattens the _IncludedRouter tree into matchable route context objects, each carrying the full templated path. For FastAPI 0.137.0–0.137.1 (which lack iter_route_contexts but still have _IncludedRouter), effective_route_contexts() on the router node serves as a fallback. For FastAPI < 0.137, routes are already flat and need no special handling.

This would avoid relying on _match(), which is a private Starlette method with no stability guarantee — a future Starlette or FastAPI release could rename it, change its return type, or remove it entirely, breaking path_tpl collection silently for all requests without any warning at startup or runtime.

For more details, refer the official documentation

@adityaanikam
adityaanikam force-pushed the 906-asgi-included-router-path-tpl branch from cfdf4c8 to a4aa388 Compare October 1, 2026 11:36
@adityaanikam

Copy link
Copy Markdown
Author

Thanks both, I reworked it along both reviews. _get_path_template and the private _match() are gone. _collect_kvs now iterates _iter_routes(app.routes), which uses fastapi.routing.iter_route_contexts (0.137.2 and later), falls back to effective_route_contexts() on the wrapper for 0.137.0 and 0.137.1, and leaves older FastAPI versions and plain Starlette routes as they are.

Match is now imported at the top of the module. I kept it behind an ImportError guard because instana.middleware exports this middleware for non-Starlette ASGI apps too, and the in-function import is removed.

I ran the FastAPI tests against 0.136.1, 0.137.0, 0.137.1 and 0.141.1, and the included-router cases fail on the unmodified code.

@CagriYonca

Copy link
Copy Markdown
Contributor

Hello again @adityaanikam , could you rebase your code to the main? CI tests will pass, then we can merge it.

Thanks a lot for your efforts!

FastAPI 0.137 stores routers added with include_router() in a lazy wrapper that has no path attribute. _collect_kvs read route.path unconditionally, so it raised AttributeError, which was swallowed and logged at debug level, and http.path_tpl was never set for those endpoints.

Flatten the routes with FastAPI's public iter_route_contexts() (0.137.2 and later), or with effective_route_contexts() on the wrapper for 0.137.0 and 0.137.1, so every route carries its fully prefixed path. Older FastAPI versions and Starlette keep their routes flat and are handled as before.

Fixes instana#906

Signed-off-by: adityaanikam <adityanikam9502@gmail.com>
@adityaanikam
adityaanikam force-pushed the 906-asgi-included-router-path-tpl branch from a4aa388 to bc03d4b Compare October 2, 2026 18:05

@CagriYonca CagriYonca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, it looks good to me now. Let's wait until @GSVarsha also reviews, then we can merge.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ASGI instrumentation crashes with AttributeError on FastAPI ≥ 0.137.0 (_IncludedRouter has no attribute 'path')

5 participants