-
-
Notifications
You must be signed in to change notification settings - Fork 301
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
Error reporter #261
Merged
Merged
Error reporter #261
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
9e97d48
registering entry points for sentry and rollbar plugins, maintained i…
danielwelch aac6984
replace rollbar reporting with generic error_reporter reporting in cl…
danielwelch c82ef6a
fixing export from conf
danielwelch 8dda520
Merge pull request #1 from Koed00/master
danielwelch f90c4f5
merge conflict resolution
danielwelch f5e62bb
documentation for error reporter plugin system
danielwelch cd038f6
remove old rollbar stuff from config
danielwelch 4530b05
putting back support for backwards compatibility
danielwelch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
Errors | ||
------ | ||
.. py:currentmodule:: django_q | ||
|
||
Django Q uses a pluggable error reporter system based upon python `extras <https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies>`, allowing anyone to develop plugins for error reporting and monitoring integration. Currently implemented examples include `Rollbar <https://rollbar.com/>` and `Sentry <https://docs.sentry.io/>`). | ||
|
||
Error reporting plugins register a class which implements a ``report`` method, which is invoked when a Django Q cluster encounters an error, pasing information to the particular service. Error reporters must be :ref:`configured<error_reporter>` via the ``Q_CLUSTER`` dictionary in your :file:`settings.py`. These settings are passed as kwargs upon initiation of the Error Reporter. Therefore, in order to implement a new plugin, a package must expose a class which will be instantiated with the necessary information via the ``Q_CLUSTER`` settings and implements a single ``report`` method. | ||
|
||
For example implementations, see `django-q-rollbar <https://github.com/danielwelch/django-q-rollbar>` and `django-q-sentry <https://github.com/danielwelch/django-q-sentry>` | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you provide examples except that they are references to a different repository.
Shouldn't
django-q-rollbar
be included by default in this repo?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eagllus
Understandable. To me, the most obvious and probably easiest solution is just to leave the old rollbar support in place along side the error reporter implementation. Then no one currently using rollbar support needs to modify their
Q_CLUSTER
settings, while new projects can instead use theerror_reporter
config setting and include thedjango-q-rollbar
extra. Seems to me this would involve simply "putting back" what was removed fromdjango_q/conf.py
anddjango_q/cluster.py
. I could modify the documentation changes to re-include the rollbar section, maybe with a note about that config setting being there for compatibility purposes and a link to the error_reporter setting. Maybe a deprecation warning if `Q_CLUSTER['rollbar'] is detected, if you're inclined.In my opinion, different repositories is a strength (and kind of the point). I don't see any reason that
django-q-rollbar
should be included by default over, let's say,django-q-sentry
aside from the backwards compatibility issue you brought up, which I think is valid. So the question seems to be, what is the best way to provide rollbar support for current users without breaking things: viaQ_CLUSTER['rollbar']
" vs. includingdjango-q-rollbar
in the django-q repository itself. To me, it seems cleaner to providedjango-q-rollbar
in the same way thatdjango-q-sentry
and potentially other plugins are provided: via a seperate repository. But I am but a hopeful contributor and those design decisions are probably not mine to make.