Skip to content
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

Modify heartDebug in client.ts to log to stdout #152

Closed
wants to merge 1 commit into from

Conversation

kumuvetti
Copy link

These heart beat messages are really not errors, so it's better to send them to stdout.
Some logging systems might flag stderr logs as errors intead of debug messages.

These heart beat messages are really not errors, so it's better to send them to stdout.  
Some logging systems might flag stderr logs as errors intead of debug messages.
@jbielick
Copy link
Owner

It's fairly common to use STDERR to output debug / warn / info lines during program execution. Since all of the debug loggers in this package all print to stderr, do you think changing just one of them to be stdout is a good idea? I think that's kind of inconsistent and I'm not sure that I'm convinced that stderr is the wrong place to log these.

@DanielChanJA
Copy link

DanielChanJA commented Jun 15, 2021

I don't think it's a common practice to use stderr for debug/warn/info during program execution. stderr should only be used for error level messages, and heartbeats are definitely not one of them.

Refer to another logging library that moved debug messages from stderr to stdout - winstonjs/winston#1332

That being said, I think for consistency sake, all messages that aren't errors, should be going to stdout instead of stderr.

@jbielick
Copy link
Owner

jbielick commented Jun 15, 2021

@DanielChanJA you’re joking, right?

Since 100% of the logging in this package is optional and you have to explicitly enable it by setting the DEBUG environment variable, why does it matter if this output s to stderr? They’re debug lines used for debugging. You don’t need heartbeat logs unless you’re debugging connection issues. Do you have DEBUG set in production? If so, why?

If you register an event listener for “error” and “fail” you can report errors to an error service. I don’t think alerting on every line written to stdout is a good idea.

@jbielick
Copy link
Owner

jbielick commented Jun 15, 2021

I’ll consider logging debug to stdout but this PR only addresses like 1 out of 10 debug loggers that write to stderr so I can’t really accept it as is if I agreed with it.

@jbielick jbielick closed this Jun 15, 2021
@DanielChanJA
Copy link

@DanielChanJA you’re joking, right?

Since 100% of the logging in this package is optional and you have to explicitly enable it by setting the DEBUG environment variable, why does it matter if this output s to stderr? They’re debug lines used for debugging. You don’t need heartbeat logs unless you’re debugging connection issues. Do you have DEBUG set in production? If so, why?

If you register an event listener for “error” and “fail” you can report errors to an error service. I don’t think alerting on every line written to stdout is a good idea.

We do it to ensure that the workers aren't stuck in an infinite loop and consuming resources as a result of the infinite loop, the heartbeats are a good indicator of a healthy worker.

My apologies, I meant that we should have the option of where the debug should print out to, I'd definitely love to help out if that's something that you'd like to see as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants