danielwelch / django-q-sentry

Bringing Sentry error tracking to Django Q
MIT License
8 stars 7 forks source link

django-q-sentry breaks normal Sentry integration #12

Open spookylukey opened 3 years ago

spookylukey commented 3 years ago

If you follow the current docs for using Sentry with Python/Django you will add something like the following code to your settings.py:

import sentry_sdk
from sentry_sdk.integrations.django import DjangoIntegration

sentry_sdk.init(
    dsn="https://mysecretthing@blahblah.ingest.sentry.io/1234",
    integrations=[DjangoIntegration()],
    traces_sample_rate=0.05,
    send_default_pii=True,
    release="myapp@1.0.0",
)

If you then follow the django-q-sentry docs, you will also add this:

Q_CLUSTER {
   # ...
   'error_reporter': {
        'sentry': {
            'dsn': SENTRY_DSN
        }
    }
}

The problem is that doing this will break your earlier config. In particular, I noticed that the performance tracing was not working at all - Sentry was not receiving any performance traces. When I disabled the error_report key in the Q_CLUSTER setting, it started working.

Digging it, it looks like django-q-sentry does its own call to sentry_sdk.init in https://github.com/danielwelch/django-q-sentry/blob/c2d0846f665c26cb58a825f58ced2265ebff8e91/django_q_sentry/sentry.py#L45 which I presume is overriding my call, and breaking things.

With django-q-sentry disabled, it appears that Sentry works just fine for reporting crashes inside my tasks. So is django-q-sentry needed at all? Would it be best to deprecate this package?

gdalmau commented 2 years ago

The workaround I'm running is as follows:

  1. Configure Q_CLUSTER without sentry in the error_reporter, for example:
Q_CLUSTER = {
    'name': 'django_q_backend',
    'max_attempts': 1,
    'redis': {
        'host': REDIS_HOSTNAME,
        'port': REDIS_PORT,
        'db': REDIS_DB,
    }
    ...
}
  1. Create a SENTRY_CONFIG, for example:
SENTRY_CONFIG = {
    'dsn': SENTRY_DSN,
    'integrations': [DjangoIntegration()],
    'debug': DEBUG,
    'send_default_pii': True,
    ...
}
  1. Make sentry to use the config
sentry_sdk.init(**SENTRY_CONFIG)
  1. Make django-Q to use the SENTRY_CONFIG
Q_CLUSTER['error_reporter'] = {
    'sentry': SENTRY_CONFIG
}
martinn commented 1 year ago

Did you find using django-q-sentry provided any extra benefits over the default setup in the end? One thing I noticed is that with the default setup, the stacktrace can often show up truncated in Sentry. And that grouping of issues can fail since a random name gets prepended to all error titles (i.e. "[oscar-purple-lithium-jig]").

gdalmau commented 1 year ago

Did you find using django-q-sentry provided any extra benefits over the default setup in the end?

I didn't test it without django-q-sentry, it works fine as is even if it could be better.

One thing I noticed is that with the default setup, the stacktrace can often show up truncated in Sentry. And that grouping of issues can fail since a random name gets prepended to all error titles (i.e. "[oscar-purple-lithium-jig]").

I think I solved this issue by following the suggestion in https://github.com/Koed00/django-q/issues/463#issuecomment-672797647

So having for example the following model:

class User:
    first_name: str
    last_name: int
    email: str

    def send_account_activation_email():
        ...

Instead of doing:

new_user = User(...)
async_task(new_user.send_account_activation_email)

You can do something like:


class BgTasks:
    @classmethod
    def send_account_activation_email(cls, u: User):
        u.send_account_activation_email()

new_user = User(...)
async_task(BgTasks.send_account_activation_email)
martinn commented 1 year ago

@gdalmau Thank you for your response!

One thing I noticed is that with the default setup, the stacktrace can often show up truncated in Sentry. And that grouping of issues can fail since a random name gets prepended to all error titles (i.e. "[oscar-purple-lithium-jig]").

I think I solved this issue by following the suggestion in Koed00/django-q#463 (comment)

Sorry, do you mean this solves the truncated stacktrace or the error titles?

Most of our async_tasks are simple functions (not class-based) however.