Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Address the two moderate filtering issues involving handler wrappers and silent transports.
Review effort: Lite
Findings: None
What changed in this PR
Adds opt-in logger-level filtering before formatting to avoid unnecessary formatter work for rejected records.
Changes:
- Adds
filterBeforeFormatruntime behavior and TypeScript support. - Adds unit coverage for filtering and fallback cases.
- Documents configuration and behavioral effects.
| File | Summary |
|---|---|
test/unit/winston/logger-filter-before-format.test.js |
Tests filtering behavior and edge cases. |
test/typescript-definitions.ts |
Validates TypeScript usage. |
README.md |
Documents configuration and behavior. |
lib/winston/logger.js |
Implements pre-format filtering; handler wrappers and silent transports require correction. |
index.d.ts |
Adds TypeScript declarations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Logger-wide formatting currently runs even when every transport rejects a record's level. This filters those records before formatting, while retaining transport-level overrides and conservative fallbacks for handler records, startup buffering, unknown levels and custom streams.
The draft currently uses
filterBeforeFormat: true, defaulting tofalse, in the shared_transformpath.Open question: configurable or a breaking change?
Should this remain opt-in, or should filtering before formatting become unconditional and be released as a breaking change, removing the configuration option entirely?
Filtering earlier changes two observable behaviors: rejected records disappear from the logger's readable output, and their formatters no longer run. This can affect
datalisteners and stateful formats such asms(), as well as custom formatter side effects. The opt-in implementation preserves existing behavior by default, but the API choice is open for maintainer feedback. Exception handling, buffering and custom-transport fallbacks remain necessary safeguards under either approach.Validation
The focused suite exits normally. The broader suite previously reported open handles after completion, so the coverage run uses
--forceExit.Reported benchmark results
For 100,000 messages on Node 22.21.1 / macOS arm64, using label/color/timestamp/metadata/printf formatting and the real Console transport filter with output I/O stubbed:
debuginfoCounts and timings were measured separately, with two warmup pairs and nine alternating measured pairs. The measured tradeoff is about 6.4× faster rejected-message logging and 3.8% overhead for accepted messages. These are logging-loop measurements, not application throughput. The benchmark is reported here rather than included in the patch.
Related: #2441, #1627, #1669 and CommunitySolidServer/CommunitySolidServer#2226.