Update rqworker command Sentry configuration (alternative version) #442
Conversation
I have added a method to the command to combine existing Sentry config options with the options passed in on the command invocation. This means that if you have initialised Sentry in the Django settings module with specific options, these will be maintained. Fixes #440
In the version I have attempted to tackle the issue of Sentry configuration from a clean slate. I have removed the code in rqworker that falls back the the `SENTRY_DSN` Django setting, on the basis that if the client user has set this in their project settings.py, then they are capable of configuring Sentry there. If the user passes `sentry_dsn` explicitly to the command, then this will override any configuration in the Django settings, and replace them with a configuration that includes the RqIntegration and DjangoIntegration.
Codecov Report
@@ Coverage Diff @@
## master #442 +/- ##
==========================================
+ Coverage 89.78% 90.16% +0.37%
==========================================
Files 27 27
Lines 1635 1637 +2
==========================================
+ Hits 1468 1476 +8
+ Misses 167 161 -6
Continue to review full report at Codecov.
|
| sentry_dsn = options.pop('sentry_dsn') | ||
| if sentry_dsn: | ||
| try: | ||
| configure_sentry(sentry_dsn, **options) | ||
| except ImportError: | ||
| self.stderr.write("Please install sentry-sdk using `pip install sentry-sdk`") | ||
| sys.exit(1) |
There was a problem hiding this comment.
This is the big change in this PR - there is no fallback to django.conf.settings.SENTRY_DSN - if you don't pass in an explicit sentry-dsn to the command, then it will use the existing Django configuration.
| sentry_options = { | ||
| 'debug': options.get('sentry_debug', False), | ||
| 'ca_certs': options.get('sentry_ca_certs', None), | ||
| 'integrations': [ | ||
| sentry_sdk.integrations.redis.RedisIntegration(), | ||
| sentry_sdk.integrations.rq.RqIntegration(), | ||
| sentry_sdk.integrations.django.DjangoIntegration() | ||
| ] | ||
| } | ||
| sentry_sdk.init(sentry_dsn, **sentry_options) |
There was a problem hiding this comment.
This is a direct replacement for the call to rq.contrib.sentry.register_sentry. The only functional change that this results in is the addition of the RedisIntegration and DjangoIntegration to the list of configured sentry integrations.
|
Hey, sorry for the late reply. I’m ok with this change. |
|
Give me a few days to further process this to think of other potential ramifications. |
This is a simpler alternative to #441. Fixes #440 and #427.
This PR changes how the
rqworkercommand integrates with Sentry.At the current time, if you pass in
sentry-dsn, or havedjango.conf.settings.SENTRY_DSNset, then the command will reinitialise Sentry using a limited subset of options, overriding any existing Django setup. The options supported aredebugandca_certs, and the list of integrations is restricted toRqIntegration.This PR prevents the possible clash with Django settings by removing the fallback to
SENTRY_DSN. If you passsentry-dsnto the command, it will use it, and ignore any existing Django settings configuration. If you don't pass it, it will do nothing - i.e. it will rely on any existing Django configuration.In addition, this does away with the
rq.contrib.sentry.register_sentryfunction call in favour of a directsentry_sdk.initcall. The RQ function explicitly sets up Sentry withintegrations=[RqIntegration()], preventing the use of any other integrations. This makes sense within the context ofrq, but if you are usingdjango-rqthen it makes more sense to use[RedisIntegration(), RqIntegration(), DjangoIntegration()].This PR will be a breaking change for anyone who does not pass
sentry-dsnto the command, but does havedjango.conf.settings.SENTRY_DSNset. This may be a dealbreaker, but that group seems like an edge case?