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

Stop using TaliskerRequestsTransport #611

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

suligap
Copy link
Member

@suligap suligap commented Dec 4, 2024

At least for now stop using the TaliskerRequestsTransport.

In practice, for flask apps it was not used in the apps/workers because FlaskSentry was explicitly setting a None 'transport' in the client kwargs.

This change makes sure that the same default raven (threaded) transport is used by logging in the main process. This will mitigate the deadlocks described in
prometheus/client_python#1076 (comment) because there will be no prometheus instrumentation for sentry requests.

At least for now stop using the TaliskerRequestsTransport.

In practice, for flask apps it was not used in the apps/workers because
FlaskSentry was explicitly setting a None 'transport' in the client
kwargs.

This change makes sure that the same default raven (threaded) transport
is used by logging in the main process. This will mitigate the deadlocks
described in
prometheus/client_python#1076 (comment)
because there will be no prometheus instrumentation for sentry requests.
@suligap
Copy link
Member Author

suligap commented Dec 4, 2024

This is a cleaner alternative to #610

Copy link
Contributor

@verterok verterok left a comment

Choose a reason for hiding this comment

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

I think this is good, and no test failures...
I couldn't find any users of this in our codebase

@suligap suligap merged commit a80eda9 into master Dec 5, 2024
5 of 6 checks passed
@suligap suligap deleted the always-use-default-raven-transport branch December 5, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants