nameko / nameko-sentry

Apache License 2.0
28 stars 9 forks source link

Migration plan for sentry-python? #24

Open zsiciarz opened 4 years ago

zsiciarz commented 4 years ago

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 https://github.com/getsentry/sentry-python/commit/87e574900533e0affcd3f5a84aca96c3a58d4aeb. If porting nameko-sentry to sentry-python is desirable, I might take a stab at a PR.

mattbennett commented 4 years ago

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 commented 4 years ago

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

mattbennett commented 4 years ago

Hi @AlexanderVanEck. The problem was exactly the one @zsiciarz mentioned in his post, fixed in this commit: https://github.com/getsentry/sentry-python/commit/87e574900533e0affcd3f5a84aca96c3a58d4aeb

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 commented 3 years ago

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

zsiciarz commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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