refactor: change middleware implementation to pure asgi

[Kludex]

Instead of a class solution, I went with a function layout (which makes more sense in this case). You can check how this is done in https://github.com/simonw/asgi-cors

Closes #23

References:

• https://asgi.readthedocs.io/
• https://github.com/encode/starlette/

[Kludex]

@trallnag I didn't finish yet. I'm probably going to finish in the weekend.

[Kludex]

I need your input here. You only use the request and response objects inside the metrics.Info to get the headers. Can I assume that is the only data we need?

[trallnag]

Hey @Kludex. Currently yes, the request and response attributes in an Info instance are currently only used in metrics.response_size.instrumentation, metrics.combined_size.instrumentation and metrics.default.instrumentation to get the content length from the headers.

But generally there might be other use cases where someone wants to use something else from these objects in a custom function that gets added to the Instrumentator with add().

PromFI (I don't want to type out the whole name all the time) has the characteristic / disadvantage that the actual instrumentation (for eaxmple incrementing a counter) only happens after the request has occurred and a response has been returned or an exception has been thrown. This makes it easy to have arbitrary code that runs after request / response

[Kludex]

I can't have the Request on the middleware without duplicating data. And I don't recommend duplicating data.

The stream is consumed in the application: https://github.com/encode/starlette/blob/e4307065ea6dbe708fba9643d14fe7adfff06d46/starlette/requests.py#L194

So the solution is to sacrifice the body, but I'm not sure if that's something we want. Maybe the best approach here is to let the BaseHTTPMiddleware based middleware.

[trallnag]

Hm, I guess not having the body would be okay even though this is a breaking change. But request/response headers and other data like request.url should be definitely available from Info instances passed to instrumentation functions.

[Kludex]

Yes, it's a breaking change. It's up to you. Either way, this is not a well-known problem.

The comment and issue where you can find more information about the problem: encode/starlette#919 (comment)

Maybe we should wait and see what happens around the subject.

[trallnag]

Maybe we should wait and see what happens around the subject.

Yeah I guess that's always an option 😄 Since I would be okay with this specific breaking change I just leave it up to you.

I actually can't think of a use case atm where you would instrument something in the body and also profit from using PromFI. I feel like such instrumentation would be single handler and not a whole FastAPI so I would just do the instrumentation without any middleware directly within the handler method.

[Kludex]

Black automatic, if you want, I can revert it.

[Kludex]

Are those tests working? 🤔 I mean, I haven't added any test, but I assume I would break some tests...

[trallnag]

Hey @Kludex, sry for keeping this PR hanging for so long. So I have checked out your feature branch locally and it indeed breaks a bunch of tests.

I've attached a file with the logs for bash run.sh test_not_slow

testlog.txt

It's basically all of the tests in metrics.py. None of them work with the async code. From the logs it seems like the actual requests are all correct.

[Kludex]

I'm going to check this at some point this week.

Do you mind if add a github workflow for the CI, to run pytest (in another PR)?

[trallnag]

Hmm there is already a workflow for running the tests and it works for me when I push or commit something:

https://github.com/trallnag/prometheus-fastapi-instrumentator/blob/master/.github/workflows/commit.yml

No idea why the workflow is not triggered for this branch. So feel free to create a PR if you know what's wrong