Skip to content
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: add Starlite integration #1748

Merged
merged 27 commits into from Jan 11, 2023
Merged

Feat: add Starlite integration #1748

merged 27 commits into from Jan 11, 2023

Conversation

gazorby
Copy link
Contributor

@gazorby gazorby commented Nov 18, 2022

Supplant #1597 and resolves #1549.

@Goldziher
Copy link
Contributor

I guess this needs to be updated @gazorby

setup.py Outdated Show resolved Hide resolved
Co-authored-by: Na'aman Hirschfeld <nhirschfeld@gmail.com>
@antonpirker
Copy link
Member

Hey @Goldziher and @gazorby ! I ran the test suite and the test for Python 3.7 and 3.8 are failing. Could you have a look: https://github.com/getsentry/sentry-python/actions/runs/3866938112/jobs/6611898016

@gazorby
Copy link
Contributor Author

gazorby commented Jan 9, 2023

Should be fixed now @antonpirker!

@antonpirker
Copy link
Member

Thanks for the update!

Now please skip the tests if the starlite package is not installed. Similar like we do in starlette tests: https://github.com/getsentry/sentry-python/blob/master/tests/integrations/starlette/__init__.py (this should fix the failing opentelemetry tests)

And in the test_starlite.py there are some unused imports, please remove them:
https://github.com/getsentry/sentry-python/actions/runs/3878616735/jobs/6621266350

@antonpirker
Copy link
Member

You can run the linters locally by running tox -e linters, or if you do not have tox installed, you can install linter-requirements.txt and then call those commands locally: https://github.com/getsentry/sentry-python/blob/master/tox.ini#L411-L413 (preferably running in Python 3.9 to have the exact same setup like the CI)
(running this locally, should prevent more back and forth)

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of review.

sentry_sdk/integrations/starlite.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/starlite.py Outdated Show resolved Hide resolved
@antonpirker
Copy link
Member

First @gazorby and @Goldziher: Great work!

I have now created a demo app like this:
Screenshot 2023-01-10 at 17 43 33

When I access the endpoint the error that ends up in Sentry as "generic Starlite request" and I am not sure why not a proper transaction name is set:

Screenshot 2023-01-10 at 17 44 21

Could you please check if you can get a proper name or tell me what I do wrong?

@gazorby
Copy link
Contributor Author

gazorby commented Jan 10, 2023

Should be fixed now @antonpirker

@antonpirker
Copy link
Member

I started the tests again, and will review again tomorrow! Thanks for the quick fixes!

@antonpirker
Copy link
Member

Ok, we have now nice transaction names!

Screenshot 2023-01-11 at 09 58 37

@antonpirker
Copy link
Member

All the tests are also green now! Nice work!

The things missing now @gazorby is in my two comments in the review. (One is just nitpicking, so feel free to ignore)

One thing I also noticed is that there is only endpoint in transaction_style. In other integrations we have also url, but most of the time endpoint is the default, so I guess this is fine. If a lot of users demand url style transaction names in the future, we can always add it.

tests/utils/test_transaction.py Outdated Show resolved Hide resolved
@antonpirker
Copy link
Member

One last thing @gazorby . See above!

@gazorby
Copy link
Contributor Author

gazorby commented Jan 11, 2023

@antonpirker As I refactored utils.transaction_from_function to support partials, django.signal_handlers._get_receiver_name
(and maybe other stuff as well) may benefit from it, should we deduplicate here or do this in another PR?

@antonpirker
Copy link
Member

please in another PR.

@antonpirker antonpirker merged commit 20c25f2 into getsentry:master Jan 11, 2023
@antonpirker
Copy link
Member

I think we are good to go! Amazing work @gazorby, thanks a lot!

We will try to ship a new version of the SDK this week.

If you send me your shipping address to anton (dot) pirker {at} sentry .dot. io I can send you a little bit of swag as a small "thank you".

@gazorby
Copy link
Contributor Author

gazorby commented Jan 11, 2023

Thanks @antonpirker!

@peterschutt
Copy link
Contributor

Thanks @gazorby!

@antonpirker
Copy link
Member

antonpirker commented Jan 12, 2023

Starlite support has been released in version 1.13.0 of the Sentry SDK:
https://github.com/getsentry/sentry-python/blob/master/CHANGELOG.md

@gazorby
Copy link
Contributor Author

gazorby commented Jan 12, 2023

Thanks for the quick release @antonpirker !

@antonpirker
Copy link
Member

No problem! Do not forget to send me your shipping address and t-shirt size to anton.pirker@sentry.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Integration Integrating with a new framework or library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] Starlite Integration
5 participants