feat(logger): use threaded logger #436
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the logging system to use the Winston logging library instead of a custom logging implementation. The main goals are to leverage Winston's multi-transport capabilities and enable threaded logging by bubbling errors and logs to the main thread, eliminating the need for a singleton getInstance() method.
- Replaced custom logger implementation with Winston-based logging
- Added thread-aware logging that routes worker thread logs to the main thread
- Removed custom transport implementations in favor of Winston transports
Reviewed Changes
Copilot reviewed 17 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/threading/worker.mjs | Updated message format to include type field for structured communication |
| src/threading/index.mjs | Added log message handling to route worker logs to main thread logger |
| src/logger/transports.mjs | Created new Winston-based transport implementations |
| src/logger/parent.mjs | Added main thread logger configuration using Winston |
| src/logger/child.mjs | Created worker thread logger that forwards messages to parent |
| src/logger/index.mjs | Simplified to export thread-appropriate logger instance |
| package.json | Added Winston dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 74.72% 72.15% -2.58%
==========================================
Files 107 100 -7
Lines 10436 9598 -838
Branches 681 595 -86
==========================================
- Hits 7798 6925 -873
- Misses 2636 2671 +35
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
bump @nodejs/web-infra |
There was a problem hiding this comment.
I'm reluctant on approving this as it pretty much removes all the work @araujogui did. If we wanted to use a 3rd party, why all that effort?
|
Could you justify why Winston? And what is this PR improving? Is it performance? Couldn't it be achieved differently? |
I chose Winston because it provides a straightforward way to add and configure transports on a shared global logger, rather than requiring a new logger instance for every thread.
This PR eliminates redundant logger instantiations across threads. Instead, it routes all log messages up to the parent process, where they are managed centrally. That way, the children can just use a
Yes, this isn’t the only possible approach. The PR simply offers an alternative design, it’s not strictly necessary to merge. |
|
My qualm here is, if this was something you were looking at for a while already... It invalidates the effort that @araujogui did on introducing an in-house logger just to have it being removed this soon. And that makes me a bit uncomfortable as in a certain way that's unappreciation of their efforts. Despite of making it threaded, is there any real issue that we're facing that this PR is solving? Or is it just a nice to have? I don't see a related issue to this PR which makes me believe it is an impromptu PR, which is fine, but if this is just refactor because of what it could provide, I feel that I'd rather stick with the current implementation until we actually start facing problems. |
|
cc @nodejs/web-infra anyone with different opinions here? |
|
I definately 100% appreciate the work of @araujogui, and clearly this came off the wrong way. I'm closing this. For clarity, the primary change I wanted to introduce was a threaded logger, and I didn't have a preference whether that was in house or out. |
I know it's not your intent (And I appreciate you!), the timing was... too soon, yk? If we actually feel that the logger not being threaded is causing issues/bug reports or noticeably slowing down the performance (assume our logger is making the script take 3+ seconds extra) then of course this is welcome. If this comes to an imaginary but possibly in the future issue... Making these changes out of nowhere can come wrong. Imagine if you made a PR that you spend weeks to do in a certain way, it gets merged just so that one of the reviewers of the same PR to make another one removing most of your work and changing it completely a few weeks later? 🙈 |
This PR makes the logger rely on
winston, a library used for multiple logger transports. Additionally, by bubbling all errors to the main thread, we can avoid thegetInstance()method, and just export the logger directly.