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

Add Sentry support #210

Closed
joeyespo opened this issue Dec 2, 2016 · 11 comments
Closed

Add Sentry support #210

joeyespo opened this issue Dec 2, 2016 · 11 comments

Comments

@joeyespo
Copy link

joeyespo commented Dec 2, 2016

I see Rollbar is built in. Could you add support for Sentry too?

Alternatively, you could pull out Rollbar into its own django-q-rollbar repo (and perhaps use extras so someone can add django-q[rollbar] to their requirements.txt), and expose a generic error handling interface so others can add their own.

Really enjoying this project btw 😄

@orangle
Copy link

orangle commented Dec 4, 2016

+1 . good idea

@jordanmkoncz
Copy link

+1 I'd definitely use the Sentry support if it was added.

@danielwelch
Copy link
Contributor

I took an initial shot at the setuptools extras/entry_points approach for rollbar and sentry support in my fork. See conf.py for most of the details.

My general idea was to require any plugin to provide an object (I refer to it as Reporter) that:

  1. exposes a report() method, which is called within the context of an exception to report it
  2. can be instantiated using configuration provided in the Q_CLUSTER dictionary in settings.py, via the key error_reporter. This key is mapped to a dictionary of configuration options, which are passed to Reporter.__init__ as kwargs (see the rollbar implementation here, for example). This is similar to how extra config options are currently passed to rollbar.init.

I created 2 packages to add support for rollbar and sentry, designed to work with the changes I made: django-q-rollbar and django-q-sentry. I modified setup.py to register entry points and define the requirements as extras.

Some drawbacks:

  • This only allows for configuration though the Q_CLUSTER dict, and the current design relies on a plugin developer being able to boil down everything they need to do into one call to report. If this is sufficient, I think it's a plus. But I worry those needing more advanced usage of sentry, rollbar, etc. will need more options.
  • I added the ErrorReporter class mostly to provide a clean interface for handling multiple error reporting mechanisms/plugins, and it feels a little awkward to me. Feedback welcome.

Disclaimer: this is my first stab at writing any python code that takes advantages of extras or entry points. I don't do this professionally, and I'd love any feedback.

@eskhool
Copy link

eskhool commented Apr 6, 2017

👍 Sentry being open source and based on python is an even better fit than roller IMHO

@Eagllus
Copy link
Collaborator

Eagllus commented Apr 7, 2017

@danielwelch I have had a look at your code and it looks good.
One important thing that I'm missing is documentation.

I'm not sure if @Koed00 has a problem with 2 extra files in the repo for the ErrorReporters 😉

@jordanmkoncz
Copy link

Any updates on this guys? :)

@danielwelch
Copy link
Contributor

I'd be happy to write up some documentation for using this and developing additional implementations, but I never heard if my approach was acceptable.

@jordanmkoncz
Copy link

@danielwelch it looks like the approach is acceptable, as @Eagllus is a contributor and seems to have given his approval. @Koed00 maybe you could confirm for us whether this approach would be acceptable?

@danielwelch
Copy link
Contributor

@jordanmkoncz Since there still seems to be some interest, I went ahead an added some documentation and submitted a pull request #261. I've also added documentation for my two proposed implementations for sentry and rollbar.

@jordanmkoncz
Copy link

Appreciate the effort @danielwelch, hopefully someone can review and approve the pull request. :)

@Eagllus
Copy link
Collaborator

Eagllus commented Feb 6, 2018

Can we close this issue because #261 has been merged?

@Eagllus Eagllus closed this as completed Feb 6, 2018
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