Skip to content

Don't suggest using settings file for Django integration #5326

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

Closed
xiongchiamiov opened this issue Jul 22, 2022 · 3 comments
Closed

Don't suggest using settings file for Django integration #5326

xiongchiamiov opened this issue Jul 22, 2022 · 3 comments

Comments

@xiongchiamiov
Copy link

Core or SDK?

Platform/SDK

Which part? Which one?

Python SDK in Django

Description

We've been initializing Sentry in the settings files, which is how
Sentry recommends it
. However, this produces problems with our
inheritance structure, because the init is run on import. We've worked
around this thus far by defining a wrapper function in base.py and
calling it at the end of every settings file. But this is a bit
clumsy. And with second-layer imports,
we can't override Sentry-related settings to be different than the imported file.
Due to these sorts of problems, generally it's advised to avoid putting
app initialization code in the settings files.

Example inheritance structure for settings that causes issues:

base.py
│
├─►development.py
├─►staging.py
├  ├─►other_staging.py
├─►production.py

Suggested Solution

In Django 1.7 they added a ready hook for apps to provide a way to
do setup code without doing it in your settings file, and so I've
created a new sentry app in our project to do this hooking. This is also slightly
nicer in that we don't need to remember to call the function when
temporarily adding Sentry to dev for debugging purposes. The app is very simple, having only an apps.py like this:

import sentry_sdk
from django.apps import AppConfig
from django.conf import settings
from sentry_sdk.integrations.celery import CeleryIntegration
from sentry_sdk.integrations.django import DjangoIntegration


class SentryConfig(AppConfig):
    name = 'sentry'

    def ready(self):
        if not settings.ENABLE_SENTRY:
            return

        sentry_sdk.init(
            dsn=settings.SENTRY_DSN,
            environment=settings.SENTRY_ENVIRONMENT,
            integrations=[DjangoIntegration(), CeleryIntegration()],
            send_default_pii=True,
            traces_sample_rate=settings.SENTRY_TRACE_SAMPLE_RATE)

and then this app is added to the top of INSTALLED_APPS.

@getsentry-release
Copy link
Contributor

Routing to @getsentry/team-web-sdk-backend for triage. ⏲️

mblayman added a commit to mblayman/homeschool that referenced this issue Dec 29, 2022
The problem is related to some cycle that happens from Sentry
initialization. By moving the init call to a ready hook, the cycle is
avoided.

See getsentry/sentry-docs#5326 for details.
@szokeasaurusrex
Copy link
Member

Since we have only received one complaint about the currently recommended setup, and since changing the instructions for setting up a commonly used integration could potentially cause issues for even larger numbers of users, we will keep the default instructions the same.

However, we agree that it would be a good idea to add the ready hook setup to the documentation, as well. We will specify it as an alternative method for setting up the Django integration when the recommended settings.py method is causing import issues.

@szokeasaurusrex szokeasaurusrex self-assigned this Mar 5, 2024
@szokeasaurusrex szokeasaurusrex removed their assignment Apr 19, 2024
@szokeasaurusrex
Copy link
Member

However, we agree that it would be a good idea to add the ready hook setup to the documentation, as well. We will specify it as an alternative method for setting up the Django integration when the recommended settings.py method is causing import issues

Upon further reflection, I believe we should not mention this alternative setup method in the docs. This setup method directly contradicts our configuration docs, which clearly state that the SDK initialization should occur "as early as possible in [the] application's lifecycle." Per Django's docs,AppConfig.ready is only called later in the app's lifecycle (specifically, after "the registry is fully populated"). Furthermore, the current method of setting up the SDK in Django clearly works for the vast majority of users; by comparison, this new method would be largely untested, since most users likely use our currently recommended setup.

I will close this issue for now. We can revisit this decision if more users report the same problem with the current setup instructions.

@szokeasaurusrex szokeasaurusrex closed this as not planned Won't fix, can't repro, duplicate, stale Apr 29, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants