feat: add additional logging handlers#907
feat: add additional logging handlers#907AngeloDanducci wants to merge 1 commit intogenerative-computing:mainfrom
Conversation
AngeloDanducci
requested review from
a team,
jakelorocco and
nrfulton
as code owners
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
jakelorocco
left a comment
jakelorocco
left a comment
There was a problem hiding this comment.
Implementation looks good! Just a few small concerns.
| It always appends new handlers (the caller is responsible for clearing | ||
| existing ones before re-configuring). It is called automatically by | ||
| :meth:`MelleaLogger.get_logger` and is also available for programmatic use. |
There was a problem hiding this comment.
I think the language of it being called automatically by .get_logger is confusing given the preceding sentence; it makes me think that calling .get_logger multiple times will continuously append the handlers. Can you please modify this comment? Maybe something like "It is called automatically on the first call of MelleaLogger.get_logger. Subsequent calls to get_logger utilize the same singleton logger and will have the handlers setup.". Feel free to change to a more nicely worded message; this was just my first attempt.
| ``MELLEA_LOG_CONSOLE`` | ||
| Set to ``false`` / ``0`` / ``no`` to disable the console (stdout) handler. | ||
| Defaults to ``true``. | ||
| ``MELLEA_LOG_FILE`` | ||
| Absolute or relative path for rotating file output (e.g. | ||
| ``/var/log/mellea.log``). When unset no file handler is attached. | ||
| ``MELLEA_LOG_FILE_MAX_BYTES`` | ||
| Maximum size in bytes before the log file is rotated. Defaults to | ||
| ``10485760`` (10 MB). | ||
| ``MELLEA_LOG_FILE_BACKUP_COUNT`` | ||
| Number of rotated backup files to keep. Defaults to ``5``. | ||
| ``MELLEA_LOG_OTLP`` | ||
| Set to ``true`` / ``1`` / ``yes`` to export logs via OpenTelemetry Logs | ||
| Protocol. Requires ``opentelemetry-sdk`` and an OTLP endpoint configured | ||
| via ``OTEL_EXPORTER_OTLP_LOGS_ENDPOINT`` or ``OTEL_EXPORTER_OTLP_ENDPOINT``. | ||
| ``MELLEA_LOG_WEBHOOK`` | ||
| HTTP(S) URL to forward log records to via HTTP POST. When set a | ||
| :class:`RESTHandler` is attached. Supersedes the deprecated ``MELLEA_FLOG`` | ||
| and ``FLOG`` variables. |
There was a problem hiding this comment.
Can you please make sure all of these are covered in docs/docs/evaluation-and-observability/logging.md?
| "File logging will be skipped.", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
I agree that we shouldn't let the exception crash the program. Could we add a comment about this in the docstring to the function? And potentially in the logging docs.
| - MELLEA_LOG_OTLP: Enable OTLP logs exporter (default: false) | ||
| - OTEL_EXPORTER_OTLP_LOGS_ENDPOINT: Logs-specific endpoint (optional, overrides general) |
There was a problem hiding this comment.
I'm fine with changing this to be singular, but I feel like it should be singular for OTEL_EXPORTER_OTLP_LOGS_ENDPOINT as well then.
2 participants
Misc PR
Type of PR
Description
Testing
Attribution