-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: logging to debug console #2474
Conversation
4744507
to
8a38e70
Compare
8a38e70
to
790b893
Compare
I believe this was addressed with #2276 |
@neilenns Not really. That PR causes everything to be displayed as an error. It would be better for the flag to be called something like |
It logs using the appropriate console method based on the severity, either |
I want to log everything to stderr and to the inspector with the correct severity. This cannot be done currently. |
That seems like a slightly odd specific combo, what's the need there, and is that a common case? I'd never expect something that goes to |
The typical Unix behavior of programs is to use stderr for logging and stdout for program output. If you log to stdout, that means you can no longer pipe your program's output to another program or save it to a file (since logs will end up there too).
Not very odd. To do it in Node.js: const logger = new console.Console(process.stderr);
This PR is pretty simple, it's more about correct behavior. Rather than having the very specific |
Good points. Getting the verbose/debug levels to inspector is an especially good point, I see that the recently merged |
@Jimbly @neilenns Do you think that this PR is a superset of what #2276 does? (Like, should we have merged this PR instead of #2276?) Or are these complementary? If so, should we make any revisions to this one given that #2276 has been merged and released? I am open to merging stuff but relying on your guys' expertise and conclusions here |
I'm not an expert in this unfortunately. The "does not call |
@DABH I looked at the code changes here. It's covering a related but different issue than what #2276 did, which simply made it possible to opt in to using This PR uses the inspector APIs to log with the level specified, including logging If this PR were updated to move the changes into the At least that's my understanding while looking at all the related and this issue while drinking morning coffee. |
I agree with this proposal by the way, it really feels like the |
@@ -30,6 +31,20 @@ module.exports = class Console extends TransportStream { | |||
this.name = options.name || 'console'; | |||
this.stderrLevels = this._stringArrayToSet(options.stderrLevels); | |||
this.consoleWarnLevels = this._stringArrayToSet(options.consoleWarnLevels); | |||
this.consoleLevels = options.consoleLevels || { |
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 needs to be added to index.d.ts as a property on ConsoleTransportOptions
.
@DABH To wrap this up neatly for you, here's what I believe needs to happen:
A branch showing these variations is available here: https://github.com/neilenns/winston/tree/neilenns/2474-update. A branch of my project using these changes is available here: https://github.com/neilenns/streamdeck-trackaudio/tree/neilenns/try-winston. The relevant logger code looks like this: import winston from "winston";
const Logger = winston.createLogger({
level: "debug",
transports: [
new winston.transports.Console({ forceConsole: true }),
new winston.transports.Console(),
],
});
export default Logger; The result will be:
Example output in VSCode: {"level":"debug","message":"WebSocket connection closed","service":"trackAudio"}
{"level":"debug","message":"Attempting to reconnect...","service":"trackAudio"} @mohd-akram @Jimbly can you please review my comments above and see if they align with your understanding? |
Doing that and the new
After my conversation with the author above yesterday, I'm leaning toward yes, this was probably the better solution that addresses the same problem, although (I swear I commented on this before, but I don't see it...) this PR as written enables writing to inspector by default, which is a significant change and will potentially be a breaking change for anyone who connects Node Inspector remotely to production servers that are very logging-heavy (will potentially almost immediately get a socket overflow and inspector or the target process will hang). So, it probably at least needs an option that doesn't write to Personally/abstractly I like the proposal of splitting |
Putting this PR as a dedicated "inspector" transport seems reasonable? It would be a non-breaking change and wouldn't require adding yet more options to the existing console logger. Then all that'd be required is fixing the infinite recursion option for console.*. |
I opened a dedicated PR to address the circular reference issue. I think that will resolve the issues exposed by the new flag, and this PR can be considered as a standalone feature request to support logging to inspector, presumably as a new transport instead of getting added to the existing Console transport. |
Thank you so much for wrapping this up neatly for me 😅 This sounds like a good plan then. Let's try to use #2498 to fix the issues that @neilenns and @Jimbly have been discussing, and then if @mohd-akram wants to, they can redo this PR to make it some kind of separate Inspector transport (although then we should discuss whether that should be a default/built-in transport, or whether it should be one of the |
Since the new It kinda seems like the cleanest/simplest solution from the user side is a single console transport with "stdio: both/stdout/stderr/none/mapping" (the existing and "inspector: yes/no/mapping" and the default (to avoid being a breaking change) is "both/no" or (to generally do what one expects/desires, as a change to defaults in a 4.0) is "both/yes". That being said, with the exception of the state need to both redirect all output to stderr and write to inspector as separate levels, that's almost what we've got with the |
I don't think a separate inspector transport makes sense, since Console is the inspector transport. As I mentioned elsewhere, there already is a stream transport for purely stdout/stderr. With the new |
Fixes #1544