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

Fix broken error ouput for django.setup() call. #12602

Closed

Conversation

whoodes
Copy link
Collaborator

@whoodes whoodes commented Jun 18, 2019

When settings.py is broken, Django doesn't start. When Django doesn't
start, relevant data is never written to our errors.log file.

This isn't desirable, so we instead catch any NameErrors that may
arise and log them appropriately.

Closes: #7032

Tested by:

  • set up a production server
  • verified it was running normally
  • Added the line: 'Foo': bar within DEFAULT_SETTINGS in settings.py
  • ran supervisorctl restart all
  • received internal server error
  • confirmed the line: 2019-06-19 20:29:25,535 ERROR zproject.wsgi name 'bar' is not defined was in var/log/zulip/errors.log.

@whoodes whoodes force-pushed the improve-settings-syntax-debuggability branch 2 times, most recently from 5a9adac to a90e219 Compare June 19, 2019 20:34
@whoodes
Copy link
Collaborator Author

whoodes commented Jun 19, 2019

@timabbott I tested this out in a production environment and it looks to be doing the job. Looking forward to your feedback :)

zproject/wsgi.py Outdated
try:
django.setup() # We need to call setup to load applications.
except NameError as e:
logger.error(e)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put the block that configures the logger here, so it only runs in the exception case?

Also, we should reraise the exception to exit, rather than continuing execution...

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, I think if we use logger.exception, the full traceback will go in the log, which is probably what we want?)

@timabbott
Copy link
Sponsor Member

This looks like a reasonable approach; let's test the improvements I suggested above.

@whoodes whoodes force-pushed the improve-settings-syntax-debuggability branch from a90e219 to 2e5f952 Compare June 20, 2019 18:53
It's common for a broken settings.py file to result in Django not
starting, thus never writing to `/var/log/zulip/errors.log`.  Such
behavior can be discouraging when the server 500s without a traceback
to accompany it.  To fix this, we simply catch the NameError, if
raised, and log the exception appropriately.

Closes: zulip#7032
@whoodes whoodes force-pushed the improve-settings-syntax-debuggability branch from 2e5f952 to 1847dcf Compare June 20, 2019 19:16
@whoodes whoodes changed the title [WIP] Fix broken error ouput for django.setup() call. Fix broken error ouput for django.setup() call. Jun 20, 2019
@whoodes
Copy link
Collaborator Author

whoodes commented Jun 20, 2019

@timabbott FYI :) I moved logging to be imported dynamically. I also updated the comment. Here's an example with the full traceback (definitely what we wanted):

2019-06-20 18:19:20,643 ERROR zproject.wsgi name 'bar' is not defined
Traceback (most recent call last):
  File "./zproject/wsgi.py", line 31, in <module>
    django.setup()  # We need to call setup to load applications.
  File "/home/zulip/deployments/2019-06-19-20-12-57/zulip-py3-venv/lib/python3.5/site-packages/django/__init__.py", line 22, in setup
    configure_logging(settings.LOGGING_CONFIG, settings.LOGGING)
  File "/home/zulip/deployments/2019-06-19-20-12-57/zulip-py3-venv/lib/python3.5/site-packages/django/conf/__init__.py", line 56, in __getattr__
    self._setup(name)
  File "/home/zulip/deployments/2019-06-19-20-12-57/zulip-py3-venv/lib/python3.5/site-packages/django/conf/__init__.py", line 41, in _setup
    self._wrapped = Settings(settings_module)
  File "/home/zulip/deployments/2019-06-19-20-12-57/zulip-py3-venv/lib/python3.5/site-packages/django/conf/__init__.py", line 110, in __init__
    mod = importlib.import_module(self.SETTINGS_MODULE)
  File "/srv/zulip-venv-cache/e1eb6592bce842070114364c4b6387c63e4c43d9/zulip-py3-venv/lib/python3.5/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 986, in _gcd_import
  File "<frozen importlib._bootstrap>", line 969, in _find_and_load
  File "<frozen importlib._bootstrap>", line 958, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 673, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 665, in exec_module
  File "<frozen importlib._bootstrap>", line 222, in _call_with_frames_removed
  File "./zproject/settings.py", line 132, in <module>
    'FOO': bar,
NameError: name 'bar' is not defined

@timabbott
Copy link
Sponsor Member

Merged as 3c1982e, after tweaking the comments for clarity and making it catch all exceptions, not just NameError (there's other syntax issues one can create in /etc/zulip/settings.py, and we want all of them to be logged properly).

Thanks @whoodes! This will result in a nontrivial reduction in folks being confused when they make mistakes setting up a Zulip production system.

@timabbott timabbott closed this Jun 24, 2019
@whoodes whoodes deleted the improve-settings-syntax-debuggability branch June 24, 2019 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve debuggability of syntax errors in /etc/zulip/settings.py files
3 participants