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

Migration plan for sentry-python? #24

Open
zsiciarz opened this issue Jan 17, 2020 · 12 comments
Open

Migration plan for sentry-python? #24

zsiciarz opened this issue Jan 17, 2020 · 12 comments

Comments

@zsiciarz
Copy link

Hey folks,
are there any plans to migrate from raven to the new Sentry SDK, aka sentry-python? The API changed somewhat, but there's a migration guide.
Last time I've checked, the new SDK had some issues with eventlet, however it looks like they fixed it in getsentry/sentry-python@87e5749.
If porting nameko-sentry to sentry-python is desirable, I might take a stab at a PR.

@mattbennett
Copy link
Member

Hi @zsiciarz

I attempted to do this a while ago and ran into the problems with Eventlet. If they are now fixed it'd be awesome to migrate to the new SDK. PR very welcome, thanks :)

@alexandervaneck
Copy link

@mattbennett Could you be more specific on the errors with Eventlet? :) I'd very much like the new SDK and have time 👌

@mattbennett
Copy link
Member

Hi @alexandervaneck. The problem was exactly the one @zsiciarz mentioned in his post, fixed in this commit: getsentry/sentry-python@87e5749

The SDK would throw an AttributeError: Queue object has no attribute all_tasks_done and fail. The old client would throw this same error if you attempted to use the (default) threaded transport, but it also shipped with a compatible eventlet transport that nameko-sentry makes use of.

@geoffjukes
Copy link

I'm interested in this too. Sentry just started emailing me about using an outdated SDK

@zsiciarz
Copy link
Author

After some experimenting, I have good news and bad news.

The good news is that the eventlet issue mentioned above has been resolved and it is at least possible to use the new SDK manually via sentry_sdk.capture_exception() in RPC handlers.

The bad news is I can't get the new SDK to work as a DependencyProvider. With raven we had a local Client and (assuming use of eventlet-specific transport) it all fit well into the worker lifecycle. The new SDK API is relying on an implicit "global" (actually, threadlocal) state. I'm attempting to rewrite nameko-sentry almost from scratch using the new API, but I got stuck - most likely on some concurrency issue. The last place I can reliably send any event to Sentry is the setup() method. Any later in the life cycle, eg. in worker_setup(), worker_result(), the calls to capture_exception() appear to have no effect. I've tried cloning Hubs as Sentry docs suggest, but without success.

There also appears to be another way of integrating sentry-sdk with frameworks such as Django, aiohttp etc. They provide Integration subclasses which then plug into sentry_sdk.init(). See aiohttp integration for an example. However, these integrations seem to override request handlers globally per application, or inject WSGI middleware in case of HTTP frameworks. I don't know if that approach fits nameko, with dependencies defined individually per each service class.

I can put up a gist with my unsuccesful attempts, if anyone is willing to help move nameko-sentry forward.

@mattbennett
Copy link
Member

I recall seeing this issue along with the "queue has no attribute" error. I think the problem is that setup() runs in a different eventlet thread to the worker, so the client never actually sees whatever the worker adds to its thread-local storage.

@zsiciarz please post your gist, and we can use that as a starting point.

@zsiciarz
Copy link
Author

@mattbennett here's my attempt as of now: https://gist.github.com/zsiciarz/84a358e9bfabc7f4857590e407d77b3b

I've stripped the dependency to bare minimum and tried to raise and capture exceptions in worker lifecycle methods, just to see if anything works. I've also attempted to store cloned Hub instances in a dictionary indexed by worker, but even worker_setup() fails to reach Sentry.

@geoffjukes
Copy link

I know this is old, and closed. Is there any forward progress on this?

I'd love to start using the Sentry "Profiling" to replace ScoutAPM

@puittenbroek
Copy link

puittenbroek commented Oct 20, 2022

After looking at @zsiciarz code I went tinkering locally since we also wanted to have Sentry for our nameko workers.
Here's what I created and what works for us for logging errors to Sentry.

https://gist.github.com/puittenbroek/f9de6ddc1fbc1ac838fd46b31c827371

@joaomcarlos
Copy link

joaomcarlos commented Jun 22, 2023

Anyone noticed that if you call sentry_sdk.init() from within the DependencyProvider.setup method any calls to Sentry from outside the worker-specific Hub are not working?

Example sentry_sdk.capture_exception instead of worker_hub.capture_exception.

I am not sure whats happening here. I tried @puittenbroek code and it seems to work fine for catching exceptions that bubble up to the worker_result. But then after adapting it to my own needs, and capturing a bunch more information I figured that any calls done without the reference to that worker_hub are never passed to Sentry.

Anyone understands whats going on here? Could this be a timing issue perhaps? Any monkeypatching trickery?

This code is based off @puittenbroek example and includes most additional features written by @mattbennett (that I could get working) and seems to be working fine for me. However, because I can't init Sentry inside the setup function, I omitted the calls to nameko.config and used env vars directly.

https://gist.github.com/joaomcarlos/e6491ec3582d57e70dd21ce807d67cf3

I also experimented with transactions, but the results were not very satisfactory. I have to explore that a little bit more. Ideally I'd like to be able to trace calls to the source of the publisher, in case of pub/sub (maybe with a manually supplied trace id?), and to the RPC caller in case of RPC.

@puittenbroek
Copy link

puittenbroek commented Aug 10, 2023

Nice work @joaomcarlos ! I ran into the problem you mentioned as well and copied your gist and adjusted a little to suit our needs :)

Don't have any further clues at the moment, sorry!

@joaomcarlos
Copy link

@puittenbroek Do let me know if you find any more info/solutions. I will do the same. We are currently busy with other things, I cant dedicate time to this. But hopefully in the future we can come back to this and get more features working.

@MattBennet I am sure my gist, while not perfect, might be good enough for a starting point towards updating nameko-sentry

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

No branches or pull requests

6 participants