feat(integrations): Support Litestar (#2413)

[KellyWalker]

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

---

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.

[sentrivana]

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.

[KellyWalker]

@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.

[szokeasaurusrex]

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.

[KellyWalker]

@szokeasaurusrex

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!

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 @ensure_integration_enabled as needed and the dependence on a PR you mentioned you would make to make that work properly. So, feel free to make another pass to make sure I addressed things as expected.

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.

TL;DR I think it would be complicated to do and not worth it.
Most of the differences between the two integrations have to do with the location of the types involved. I imagine someone with sufficient Python-fu (not me unfortunately) could do some aliasing magic such that some of the functions could be made to work for both integrations, but I also think that might be confusing enough that it outweighs the benefit of code reuse. I guess we would still end up with several instances of checking which integration we are in. There is probably some code without such concerns that could be extracted and called by both integrations, but I think it is little enough to not make it worth the effort.

[KellyWalker]

@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 tox.ini:

    # Litestar
    {py3.8,py3.11,py3.12}-litestar-v{2.0}
    {py3.8,py3.11,py3.12}-litestar-v{2.5}
    {py3.8,py3.11,py3.12}-litestar-latest
...

    # Litestar
    litestar: pytest-asyncio
    litestar: python-multipart
    litestar: requests
    litestar: cryptography
    litestar-v2.0: litestar~=2.0.0
    litestar-v2.5: litestar~=2.5.0
    litestar-latest: litestar

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:
https://github.com/litestar-org/litestar/releases/tag/v2.3.0.

How should I update things to limit testing to valid versions?

Options I can think of:

1. Replace testing against litestar 2.0.0 with 2.3.0 since the latter is valid for all versions. But that means we won't be testing against earlier versions that are valid for Python < 3.12.
2. Change the target testing litestar 2.0.0 to not include Python 3.12, and just test Python 3.12 with litestar latest. But that means we won't be testing the earliest version of litestar that supported Python 3.12, and that kind of boundary testing seems to be part of the goal.
3. Change the target testing litestar 2.0.0 to not include Python 3.12, then add a target testing 2.3.0 that includes at least (perhaps only) Python 3.12.

I suggest we go for option 3 and have something like this:

    # Litestar
    {py3.8,py3.11}-litestar-v{2.0}
    {py3.12}-litestar-v{2.3}
    {py3.8,py3.11,py3.12}-litestar-v{2.5}
    {py3.8,py3.11,py3.12}-litestar-latest
...

    # Litestar
    litestar: pytest-asyncio
    litestar: python-multipart
    litestar: requests
    litestar: cryptography
    litestar-v2.0: litestar~=2.0.0
    litestar-v2.3: litestar~=2.3.0
    litestar-v2.5: litestar~=2.5.0
    litestar-latest: litestar

But if y'all prefer option 1 or 2 (or something else) to minimize test runtime, I am open to that.

[szokeasaurusrex]

@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.

[szokeasaurusrex]

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]

Codecov Report

Attention: Patch coverage is 82.75862% with 25 lines in your changes missing coverage. Please review.

Project coverage is 79.73%. Comparing base (901a5e8) to head (dad4129).

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
sentry_sdk/integrations/litestar.py	82.39%	18 Missing and 7 partials ⚠️

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     

Files	Coverage Δ
sentry_sdk/consts.py	93.05% <100.00%> (+0.09%)	⬆️
sentry_sdk/integrations/litestar.py	82.39% <82.39%> (ø)

... and 1 file with indirect coverage changes

[szokeasaurusrex]

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.

[sentrivana]

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.

[sentrivana]

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

[KellyWalker]

@sentrivana

Finished looking through the integration, have some comments/suggestions, please take a look.

I applied all your suggestions, and I answered your question regarding a difference relative to the StarliteIntegration.

Next up: checking the tests and testing with a live app -- will do this tomorrow

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 StarliteIntegration docs), it would serve to explicitly validate that as well.
https://github.com/getsentry/sentry-docs/pull/10880/files#:~:text=%60%60%60-,%23%23%20Verify,%60%60%60,-When%20you%20point

[antonpirker]

@KellyWalker added an empty commit to trigger the CI again (github had problems yesterday)

[antonpirker]

I tested the integration with this demo app:
https://github.com/antonpirker/testing-sentry/tree/main/test-litestar

Errors are there:

Details of errors are also there:

The span tree is 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?

[sentrivana]

Checked the tests and also tested with a live app (followed the draft docs page). All looking good!

[antonpirker]

Looks fine!