feat: Add option to disable logging via environment variable#52
feat: Add option to disable logging via environment variable#52hahahamid wants to merge 3 commits into
Conversation
Add DISABLE_LOGGING environment variable to control whether logs are output. When set to 'true', logging is disabled; otherwise logging is enabled (default). Changes: - Add Enabled() method to ILogger interface - Add enabled field to ZeroLog struct - Add Enabled() implementation to both NoOpLogger and ZeroLog - Update logging methods in ZeroLog to check enabled flag - Check DISABLE_LOGGING env var in NewZeroLogger() Fixes Zomato#31
Mrsandeep27
left a comment
There was a problem hiding this comment.
The intent — let users silence the logger without changing call sites — is good. A few thoughts before merge:
1. enabled is read once at NewZeroLogger() time. If a deployment toggles `DISABLE_LOGGING` after process start, nothing notices. For a long-lived service that's almost certainly fine; for a test or short-running tool it can surprise. Worth a one-liner in the README/godoc clarifying the read happens at construction, not per-call.
2. The Enabled() bool addition to ILogger is a public-API change to anyone implementing the interface externally. Worth noting in the changelog/release notes — third-party loggers will fail to compile until they add the method. `NoOpLogger` is updated here, but custom implementations downstream won't be.
3. if l.enabled { ... } repeated four times. Consider gating once at the top via a `level()` helper: `if !l.enabled { return }`. Same effect, less surface area for someone to forget the check on a 5th method.
4. The ENV var name is DISABLE_LOGGING. Negative env vars (`DISABLE_X`) are surprising — DISABLE_LOGGING=false\ reads as "logging is on" but visually parses as a negation. Consider `ESPRESSO_LOG_ENABLED` (positive, namespaced) or `ESPRESSO_LOG_LEVEL=off`. Not a correctness issue, but namespace hygiene matters when integrators set env vars in container manifests.
5. Enabled() on the interface is unused inside the package. If the only purpose is so a caller can short-circuit expensive log-arg construction (`if logger.Enabled() { logger.Info(ctx, expensive(), fields) }`), worth showing that pattern in a doc comment — otherwise consumers won't know to use it.
Direction is sensible; mostly polish.
- Rename DISABLE_LOGGING to ESPRESSO_LOG_ENABLED (positive flag) - Add logIfEnabled helper to reduce repeated if checks - Add doc comments explaining Enabled() usage pattern - Add note about env var being read once at init time - BREAKING: ILogger interface now requires Enabled() method
|
@Mrsandeep27 Thanks for the review! I've addressed all 5 points:
For point 2 (breaking interface change), since this is a v0.x project, I didn't add a formal changelog file, but the commit message notes it. If you have a specific changelog format preference, let me know! |
Summary
Implements feature request #31 - Add option to disable logs based on conditions.
Changes
DISABLE_LOGGINGenvironment variable to control logging outputEnabled()method toILoggerinterfaceUsage
Why this matters
Users reported that PDF generation steps fill up the console stdio. This gives them control to disable verbose logging when needed.
Fixes #31