Skip to content

Fix handling root_path#81

Merged
stephenhillier merged 1 commit intostephenhillier:masterfrom
dolfinus:master
Jan 6, 2024
Merged

Fix handling root_path#81
stephenhillier merged 1 commit intostephenhillier:masterfrom
dolfinus:master

Conversation

@dolfinus
Copy link
Copy Markdown
Contributor

@dolfinus dolfinus commented Dec 23, 2023

  1. Fix handling of applications started using uvicorn --root-path=/api. Fixes filter_unhandled_paths broken with root_path set via Uvicorn #75.

Previously PrometheusMiddleware just threw away root_path from request scope. Because of this, _get_router_path always returned None, and if PrometheusMiddleware was used with both group_paths=True and filter_unhandled_paths=True, all application endpoints were just excluded from monitoring.

This requires statlette>=0.33 because of Kludex/starlette#2352.

  1. Previously __call__ method was implemented like this:
try:
  await self.app(scope, receive, send)
except Exception:
  ...
  raise
finally:
  if is_known_endpoint(path):
    return
  ...

But if the finally clause executes a return statement, exceptions are not re-raised. So bug from item 1 prevented unhandled exceptions from being raised properly. This caused some unexpected log messages, like this:

Simple application
from fastapi import FastAPI

app = FastAPI()

@app.get("/data")
def get_data():
    raise RuntimeError

from starlette_exporter import PrometheusMiddleware

app.add_middleware(
    PrometheusMiddleware,
    app_name="my_app",
    group_paths=True,
    filter_unhandled_paths=True,
)
uvicorn main:app --port 8080 --root-path /api
curl http://localhost:8080/data
INFO:     Started server process [64935]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8080 (Press CTRL+C to quit)
ERROR:    ASGI callable returned without starting response.
INFO:     127.0.0.1:58474 - "GET /data HTTP/1.1" 500 Internal Server Error

I've just removed finally block:

exception = None
try:
  await self.app(scope, receive, send)
except Exception as e:
  ...
  exception = e

if is_known_endpoint(path):
  if exception:
    raise exception
  return

...

if exception:
  raise exception

Fixes #80.

  1. Added tests for custom root_path with failing endpoints and not, with enabled filter_unhandled_paths and not, to cover all possible cases. Also rearranged few tests a bit.

@stephenhillier
Copy link
Copy Markdown
Owner

@dolfinus thanks for the PR. This looks like a good solution. For the new base path test, it looks like route matching is failing - self._get_router_path(scope) on line 340 might be returning None. Any ideas? I'll try to take a look as well over the next few days.

@dolfinus
Copy link
Copy Markdown
Contributor Author

dolfinus commented Jan 4, 2024

This test is failing on starlette<0.33 because of Kludex/starlette#2352. Added condition here.

It is possible to update minimal version of starlette to fix this, but FastAPI still does not support this version: fastapi/fastapi#10798

@stephenhillier stephenhillier merged commit e1c1938 into stephenhillier:master Jan 6, 2024
@stephenhillier
Copy link
Copy Markdown
Owner

stephenhillier commented Jan 6, 2024

Thank you! Really appreciate the contribution and explanation.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Starlette AuthenticationBackend exceptions disappear filter_unhandled_paths broken with root_path set via Uvicorn

2 participants