-
Notifications
You must be signed in to change notification settings - Fork 484
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(integrations): Support Litestar (#2413) #3358
feat(integrations): Support Litestar (#2413) #3358
Conversation
Support Litestar through a new LitestarIntegration based on porting the existing StarliteIntegration. `starlite` was renamed `litestar` as part of its move to version 2.0. Closes #2413
`pydantic` is not strictly required for `litestar` as it was for `starlite`. It seems preferable to have an initial implementation excluding any use of `pydantic` and only add handling for anything `pydantic` related if/when the need arises.
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.
Hey @KellyWalker, thank you so much, this looks great! We will give this a proper review soon -- for now just two suggestions to tweak the test targets.
Co-authored-by: Ivana Kellyer <ivana.kellyer@sentry.io>
Co-authored-by: Ivana Kellyer <ivana.kellyer@sentry.io>
@sentrivana thanks for the suggestions. The tox.ini, automated testing across versions, etc. is something I knew would probably need some work by someone more familiar with the project. If you are allowed to directly make those kinds of edits to this PR as a maintainer, feel free to do so, or I can review and commit them if you prefer. |
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.
Thank you for your contribution!
I have done a first pass review and have added some suggestions. Once you address those, we will be happy to re-review. Please let me know if you have any questions!
One other thing I am wondering: since it seems a lot of this code was just copied over from the StarliteIntegration, maybe there is a way we could extract the common code so we can use the same code in both places? Although, it is probably not worth it if it would be super complicated to do.
Thanks for the initial review. I made most of the changes suggested. I think the only comments left open that you added have to do with adding
TL;DR I think it would be complicated to do and not worth it. |
…`Ref` type for route handler functions Rather than referring directly to the `Ref` type (which has been removed) in any way, we check for the presence of the single member `value` that `Ref` defined
…n equality Some older versions of litestar will end up with "partial(<function some_function>)" rather than "some_function"
@sentrivana @szokeasaurusrex could I get some advice on how to handle something wrt tox.ini and testing across versions? Things are currently configured to test litestar version 2.0.0, 2.5.0, and latest (2.10.0 for now) using Python 3.8, 3.11, 3.12. From
It turns out Python 3.12 support did not really get added to litestar until 2.3.0. See the first of the "New Features" here: How should I update things to limit testing to valid versions? Options I can think of:
I suggest we go for option 3 and have something like this:
But if y'all prefer option 1 or 2 (or something else) to minimize test runtime, I am open to that. |
@KellyWalker option 3 makes the most sense to me! We can test Litestar v2.3 against Python 3.12 only. I would expect the test runtime to be the same as what we have right now, since instead of testing Python 3.12 with Litestar 2.0, we test it with 2.3. |
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.
Thanks for replying to the feedback so quickly! I re-reviewed this PR, including looking at the tests this time.
I made a small mistake with my previous review, which I have corrected in this review. Sorry about that!
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #3358 +/- ##
==========================================
+ Coverage 79.71% 79.73% +0.02%
==========================================
Files 132 133 +1
Lines 14264 14409 +145
Branches 3003 3032 +29
==========================================
+ Hits 11370 11489 +119
- Misses 2071 2089 +18
- Partials 823 831 +8
|
…_integration_enabled
…tation details Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>
…ithub.com/KellyWalker/sentry-python into feature/kelly.walker/litestar-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.
Looks good, thanks for the contribution!
Since this was a larger change, I would like to get a second member of the team to review this before we merge. Maybe you could do the review @sentrivana?
Also, we should test the changes manually against a simple Litestar app.
Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>
Thanks for addressing all the comments so far! I will take a look soon so that we can wrap this up this week. Will also test this with a live 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.
Finished looking through the integration, have some comments/suggestions, please take a look.
Next up: checking the tests and testing with a live app -- will do this tomorrow
I applied all your suggestions, and I answered your question regarding a difference relative to the
No doubt you know how to make a sample live app, but if you could do it like it is described in this draft PR for updating the sentry-docs (which is a copy from the |
@KellyWalker added an empty commit to trigger the CI again (github had problems yesterday) |
I tested the integration with this demo app: Details of errors are also there: Looks really good! One thing about the span tree: I have a 200ms sleep in my view. Should be the first three spans from the middleware be before those 200ms and the last three spans from the middleware at the end? |
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.
Checked the tests and also tested with a live app (followed the draft docs page). All looking good!
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 fine!
…h new LitestarIntegration (#3384) The new LitestarIntegration was initially ported from the StarliteIntegration, but then had a thorough code review that resulted in use of type comments instead of type hints (the convention used throughout the repo), more concise code in several places, and additional/updated tests. This PR backports those improvements to the StarliteIntegration. See #3358. --------- Co-authored-by: Anton Pirker <anton.pirker@sentry.io>
Adds support for Litestar through a new LitestarIntegration based on porting the existing StarliteIntegration. Starlite was renamed Litestar as part of its move to version 2.0. Closes #2413 --------- Co-authored-by: Ivana Kellyer <ivana.kellyer@sentry.io> Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com> Co-authored-by: Anton Pirker <anton.pirker@sentry.io>
…h new LitestarIntegration (#3384) The new LitestarIntegration was initially ported from the StarliteIntegration, but then had a thorough code review that resulted in use of type comments instead of type hints (the convention used throughout the repo), more concise code in several places, and additional/updated tests. This PR backports those improvements to the StarliteIntegration. See #3358. --------- Co-authored-by: Anton Pirker <anton.pirker@sentry.io>
Support Litestar through a new LitestarIntegration based on porting the existing StarliteIntegration.
starlite
was renamedlitestar
as part of its move to version 2.0.Closes #2413
General Notes
Thank you for contributing to
sentry-python
!Please add tests to validate your changes, and lint your code using
tox -e linters
.Running the test suite on your PR might require maintainer approval. Some tests (AWS Lambda) additionally require a maintainer to add a special label to run and will fail if the label is not present.
For maintainers
Sensitive test suites require maintainer review to ensure that tests do not compromise our secrets. This review must be repeated after any code revisions.
Before running sensitive test suites, please carefully check the PR. Then, apply the
Trigger: tests using secrets
label. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.