Skip to content
This repository has been archived by the owner on Feb 19, 2021. It is now read-only.

WIP: Logging configuration #496

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ovv
Copy link
Contributor

@ovv ovv commented Jan 30, 2019

Putting it out here to get feedback on the approach.

I figured adding a file to configure the whole of logging would be better than individual option (See #495 ).
I'm not sure how to best name the file, creating a paperless directory and simply naming it logging.yml seems fine but it changes where the configuration should be located.

I still need to do some more tests and write some documentation

@danielquinn
Copy link
Collaborator

This is all good stuff, and I just have a few notes:

  1. I like the move to use YAML for configuration. Especially in the case of logging, this is a natural choice. My only concern though is that we're introducing another config file in another format, which isn't very user-friendly. Paperless has always followed the 12-factor methodology (configuration only through environment variables) to keep things portable, and adding this new file (and format) introduces a requirement for the existence of a readable file. If we're going to introduce this requirement, we should be conscious of the limitations it'll introduce in terms of portability between setups (docker vs bare metal vs some other implementation).
  2. The use of Sentry is a great idea, but it's probably best to make use of their raven library rather than rolling our own integration. Sentry has some pretty good docs on this subject, but the TL;DR is that you need to add their library to INSTALLED_APPS which could be done in settings.py dynamically after detecting the preferred configuration.

@danielquinn
Copy link
Collaborator

Further to 1️⃣ above, would it be altogether crazy to set an environment variable to a JSON value that would represent the logging config? It'd be ugly, but at least more portable. Something like:

PAPERLESS_LOGGING={"version": 1, "disable_existing_loggers": false, "handlers": {"consumer": {"class": "documents.loggers.PaperlessLogger"}}, "loggers": {"documents": {"handlers": ["consumer"], "level": "INFO"}}}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants