-
Notifications
You must be signed in to change notification settings - Fork 252
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
Structlog #5687
Structlog #5687
Conversation
Can you provide some examples of the resulting Nautobot logs both with and without DEBUG enabled? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a non-trivial feature, would you please target the PR to next
(2.3.0) rather than develop
(2.2.x)?
nautobot/core/settings.py
Outdated
INSTALLED_APPS, | ||
MIDDLEWARE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these work as intended when adding additional apps via EXTRA_INSTALLED_APPS
(https://docs.nautobot.com/projects/core/en/stable/user-guide/administration/configuration/optional-settings/#extra-applications) and/or when a Nautobot App adds its own middleware? (https://docs.nautobot.com/projects/core/en/stable/development/apps/api/nautobot-app-config/?h=middleware#optional-nautobotappconfig-attributes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well with both EXTRA_INSTALLED_APPS
and EXTRA_MIDDLEWARE
.
nautobot/core/settings.py
Outdated
debug_db=is_truthy(os.getenv("NAUTOBOT_LOG_DEBUG_DB", "False")), | ||
plain_format=is_truthy(os.getenv("NAUTOBOT_LOG_PLAIN", "False")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are only configurable via environment variables (not ideal!) then they should be documented in nautobot/docs/user-guide/administration/configuration/optional-settings.md
under the "Environment-Variable-Only Settings" header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a better way to configure these? Note, we need to process these configuration settings into LOGGING
Django settings attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally they'd be settable directly via Python in any nautobot_config.py
as well without needing to manipulate os.environ
.
if "test" in sys.argv: | ||
django_logging["handlers"] = { | ||
"null_handler": { | ||
"level": "INFO", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not log_level
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
level
is the correct key here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I meant instead of "INFO"
being hard-coded. Same comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing, it was hardcoded to INFO
in the original settings, so this keeps the same behavior.
As it is using "class": "logging.NullHandler"
, it disables log anyway.
} | ||
for logger in django_logging["loggers"].values(): | ||
logger["handlers"] = ["null_handler"] | ||
logger["level"] = "INFO" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not log_level
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
level
is the correct key here.
if debug_db: | ||
django_logging["loggers"]["django.db.backends"] = {"level": "DEBUG"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this specific logger handled specially?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is driven by NAUTOBOT_LOG_DEBUG_DB
environment variable, inspired by management services configuration.
Added info about new environment variables to changes/5687.added
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the answer to why
is to be able to selectively enable database queries logging.
Changed base branch to |
0c13b1e
to
f67f724
Compare
Log examples with:
With:
With:
|
Is this a breaking change? Will the defaults have logs in the same output as before? I'm thinking about security organizations that may have parsers of logs set up... |
I don’t feel comfortable with the size and scope of this for 2.3. When we spoke about this, my understanding was we were going to add structlog as a core dependency only. Any configuration can go in |
By default, log output is different now, using structlog JSON formatting. For instances rewriting |
Separated structlog configuration to Added Added |
As I understand it the approach we would prefer is to add |
Updated based on the latest comments, ready for review now. |
@@ -85,3 +85,30 @@ Markdown rendering is supported for the `description`, `details`, and `default_l | |||
### Technical Details of Settings Documentation | |||
|
|||
The `optional-settings.md` and `required-settings.md` files are rendered as Jinja2 templates via [`Mkdocs-Macros`](https://mkdocs-macros-plugin.readthedocs.io/en/latest/). The file `nautobot/docs/macros.py` is responsible for loading `settings.yaml` into the template context for rendering. `mkdocs.yml` instructs Mkdocs-Macros to run that file at documentation rendering time. The macros and templating are *not* enabled for all documentation by default - instead, only the files with `render_macros: true` in their headers will be templated. | |||
|
|||
## Structlog Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This settings.md
file is developer-facing not end-user-facing. Can you add this content to nautobot/core/settings.yaml
under the LOGGING
details
section instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved as proposed.
Closes NaN
What's Changed
structlog
logging.NAUTOBOT_LOG_DEBUG_DB
environment variable to enable Django database logs.NAUTOBOT_LOG_PLAIN
environment variable to switch to plaintext structlog output from JSON.To Do
Open a new pull request to
cookiecutter-nautobot-app
repository to updatedevelopment/nautobot_config.py
file accordinglyThis update, to be working properly in apps development environment, requires change to apps
development/nautobot_config.py
:e.g. https://github.com/nautobot/nautobot-app-ssot/blob/develop/development/nautobot_config.py#L91
Here we should assign
LOGGING
only when it's not already defined to keep backward compatibility.