-
Notifications
You must be signed in to change notification settings - Fork 530
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
feat(starlette): add Starlette integration #1441
Conversation
bec5b4c
to
2c325f5
Compare
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
45a0192
to
545968a
Compare
af89c86
to
ff1153a
Compare
0370bee
to
b188178
Compare
d952009
to
d0308f7
Compare
b523595
to
5f42a1a
Compare
… requires 3.7 and up
…sentry-python into neel/WEBBACKEND-167/starlette
Add FastAPI integration that sets transaction names and sources (the rest is done in the Starlette integration)
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.
This is awesome! 🚀
Just a couple of minor comments for the future.
def patch_middlewares(): | ||
# type: () -> None | ||
|
||
old_build_middleware_stack = FastAPI.build_middleware_stack |
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.
The Starlette base middleware will probably change to fix a couple of issues, and this logic to build the middleware stack will probably change in the future. But I would expect the new version should be easier to patch too.
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.
Good to know. (And yes, it was a bit cumbersome to instrument the middlewares :-) )
Any plans on when this will change?
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.
Not sure yet because it will probably have to be fixed in Starlette. There are several people trying to solve it, I still have to help review all that too. And then port/update it in FastAPI
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.
The Starlette base middleware will probably change to fix a couple of issues
What issues? If you mean the BaseHTTPMiddleware
yes, but if you mean the build_middleware_stack
, there's nothing on Starlette's issues.
EDIT: Reading this again, I think you meant BaseHTTPMiddleware
. Which doesn't affect the build_middleware_stack
.
if not _should_send_default_pii(): | ||
return |
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.
This is great for Starlette!
For FastAPI, the AuthenticationMiddleware
is probably not commonly used because that is handled with dependencies, and there's no predefined expected/standardized user data/model.
It would probably make sense to explain in the docs how to manually and optionally record user data for FastAPI users in dependencies.
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.
Good to know. What is the most used dependency for user handling?
We have something in the documentation on how to set the user: https://docs.sentry.io/platforms/python/enriching-events/identify-user/
I will make sure it lands in the FastAPI docs too!
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.
That sounds perfect!
There's not an expected predefined user data model or dependency, so every application can use the data model they find more useful. It normally doesn't require adopting a predefined model or any type of dependency, because installing, learning, and using some package would require the same amount of effort and code as implementing it directly.
The documentation explaining how to handle users is here: https://fastapi.tiangolo.com/tutorial/security/get-current-user/
But again, it's just explaining the concepts, not enforcing a data model.
I think that documenting how to handle PII in the developers' code, as you suggest with the existing docs, would probably be the best approach here too. Instead of trying to solve it behind the scenes as there's nothing obvious to expect.
Good feedback! I will look at your comments tomorrow! |
I don't know how to say this without sounding pedantic, but... Is it possible to change the name of the integration to |
Integration for the Starlette Framwork: https://www.starlette.io/
Following features are implemented:
Missing (will be done in a later release):
Closes #1463