-
Notifications
You must be signed in to change notification settings - Fork 797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fastapi + Gunicorn example in README #812
Fastapi + Gunicorn example in README #812
Conversation
README.md
Outdated
def make_metrics_app(): | ||
registry = CollectorRegistry() | ||
multiprocess.MultiProcessCollector(registry) | ||
return prometheus_client.make_asgi_app(registry=registry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashirviskas should be based on the import
return make_asgi_app(registry=registry)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be late, but I just fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me with a small nit. In addition, the second commit needs to be signed off.
README.md
Outdated
|
||
metrics_app = make_metrics_app() | ||
app.mount("/metrics", metrics_app)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: extra newline here.
README.md
Outdated
|
||
|
||
metrics_app = make_metrics_app() | ||
app.mount("/metrics", metrics_app)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove double )
at the end of line
I am still learning a bit when it comes to pythons async implementation but should it not be noted in the Multiprocess portion that multiprocess mode uses blocking io calls (to a memory mapped file so it is quick, but will still blocks the event loop)? |
I know this isn't merged yet, but is it good practice? |
It's the 11 month anniversary of this PR, are we still deliberating or could this be merged in? As per the motive of the original #810, having this in the README.md would have saved significant time. |
I cannot merge this without the DCO being signed for all commits.
As the readme mentions, that is best practice to avoid metrics registering themselves with the multiprocess registry. Since the registry is only created inside of the create app function I think this approach is correct. |
Whats the easiest way I could resign all the commits? EDIT: Amended the commit messages, just no idea how to trigger DCO to rerun. |
9a7198a
to
b1c9ce5
Compare
Also provided additional multiprocessing instructions for FastAPI + Gunicorn setup with code examples as per this issue: prometheus#810 Signed-off-by: Matas Minelga <minematas@gmail.com>
Signed-off-by: Matas Minelga <minematas@gmail.com>
Removed extra characters Signed-off-by: Matas Minelga <minematas@gmail.com>
b1c9ce5
to
c4af65e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it ran when you force pushed (as it should). Thanks!
Wow! Thanks everyone for sorting today and for creating the additional PR in the first place. |
Also provided additional multiprocessing instructions for FastAPI + Gunicorn setup with code examples as per this issue: #810
Signed-off-by: Matas Minelga minematas@gmail.com