Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

feat(sentry): add starlite sentry integration #239

Merged
merged 5 commits into from
Jan 15, 2023
Merged

feat(sentry): add starlite sentry integration #239

merged 5 commits into from
Jan 15, 2023

Conversation

gazorby
Copy link
Collaborator

@gazorby gazorby commented Jan 13, 2023

Closes #52

@peterschutt
Copy link
Member

So the way the sentry integration globally patches starlite means that if you run that failing test on its own, it passes as sentry.configure() has never been called. Once it is called at all, that test will fail as there is an exc. handler added in the init wrapper (as you know!).

So I think we just disable sentry for the tests, thoughts @gazorby?

@gazorby
Copy link
Collaborator Author

gazorby commented Jan 13, 2023

Yes it's better to disable it for unit tests, but maybe we can do something in the integration environement? I just saw that LifespanManager trigger lifecycle hooks, so we could try something like that:

from starlite import Starlite
from sentry_sdk.integrations.starlite import SentryStarliteASGIMiddleware

def test_sentry(app):
    old_init = Starlite.__init__
    async with LifespanManager(app):
        assert isinstance(app.middleware[0], SentryStarliteASGIMiddleware)
    Starlite.__init__ = old_init

@gazorby
Copy link
Collaborator Author

gazorby commented Jan 13, 2023

Also, what do you think about disabling sentry when ENVIRONMENT == "local"?

@peterschutt
Copy link
Member

peterschutt commented Jan 13, 2023

but maybe we can do something in the integration environement?

It would be nice to have the sentry integration enabled somewhere within the tests to pick up any side-effects of having it activated before deploying. I'd even be happy to run the whole integration test suite with it activated - perhaps we can use the app fixture in tests/integration/conftest.py, make it a yield fixture, and have the logic to "un-patch" starlite after the yield.

I've still got to fix that integration test that has been x-failed.

If you want to do it in this PR or another, I'm easy.

Also, what do you think about disabling sentry when ENVIRONMENT == "local"?

Yes, makes sense, and have the implicit setting able to be overridden by environment like we did for reloading.

@peterschutt peterschutt changed the base branch from main to 0.28 January 15, 2023 00:29
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@peterschutt peterschutt merged commit 740f3f7 into 0.28 Jan 15, 2023
@peterschutt peterschutt deleted the add-sentry branch January 15, 2023 00:33
@peterschutt peterschutt mentioned this pull request Jan 15, 2023
peterschutt added a commit that referenced this pull request Jan 16, 2023
* ✨ feat(sentry): add starlite sentry integration

* ✅ test(sentry): add traces sampler test

* feat(sentry): disabled by default if environment "local" or "test"

Co-authored-by: Peter Schutt <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(sentry): the sentry asgi middleware doesn't really work for us
2 participants