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 ZEN_QUERIES_DISABLED_HANDLER setting #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dominikbak
Copy link

This PR introduces the ZEN_QUERIES_DISABLED_HANDLER, a feature that allows developers to substitute the default raise of QueriesDisabledError with any callable.

The motivation behind this change is to differentiate the behavior based on the application's environment - in my case, development, QA, or production. In development and QA, I want to raise exceptions to immediately spot potential issues. However, in production, I don't want to disrupt the user experience with errors. Instead, I would like to silently log these incidents.

This is one potential solution, but another could involve having default settings files and a path to _raise_exception set in ZEN_QUERIES_DISABLED_HANDLER

Your feedback is highly appreciated.

@j4mie
Copy link
Member

j4mie commented Jul 17, 2023

Hi,

Huge thanks for the PR and for your time spent thinking about this.

We've had several requests over the years for an option to "turn down" django-zen-queries in production, for example by simply logging the query rather than raising an exception. Of the suggestions I've seen, I like your idea the best: delegate to a callback and make it configurable.

However, at the moment I'm not sure that merging this is something I want to do. I feel like the whole point of zen-queries is to be a sharp tool, and by muting the exceptions you're blunting it. As soon as it becomes optional, developers will start ignoring it. The idea is that a stray query should be a "drop everything and fix this now" event, or you might as well not use it.

@balazs-endresz
Copy link

To elaborate on the use case a bit more if we could have queries_disabled() raise an exception only when we have e.g. DEBUG=True then we'd always get an error locally when making changes to the code. And that's really the only point where we want to see those errors.

On production logging a warning might be useful, but we'd see n+1 queries reported by other tools anyway, so this isn't strictly necessary there.

Being able to do this conditionally would also help to adopt queries_disabled() incrementally. Applications might not have any optimisations early on but as the amount of data grows we keep addressing those issues continuously as needed, which tend to affect only a small number of users first. So it doesn't necessarily make sense to have this strictly enabled in production for every project.

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

Successfully merging this pull request may close these issues.

3 participants