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

Make a stderr configuration of the logger module, and use it in tests #537

Merged
merged 2 commits into from
Oct 23, 2020

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Oct 23, 2020

Sometimes, it isn't acceptable to log on stdout, because a CLI tool
may be required to return structured output on stdout.

Typically, the stderr channel is reserved for logging in unix, and
this is what glog does by default. I'm not sure why we log to stdout.

To avoid breaking any of our partners who may be depending on the stdout
behavior, this creates a configuration for stderr, but keeps the defaults
with the current behavior.

If we are sure that stderr is acceptable then ideally we would drop this and not log to stdout ever.

Sometimes, it isn't acceptable to log on stdout, because a CLI tool
may be required to return structured output on stdout.

Typically, the stderr channel is reserved for logging in unix, and
this is what glog does by default. I'm not sure why we log to stdout.

To avoid breaking any of our partners who may be depending on the stdout
behavior, this creates a configuration for stderr, but keeps the defaults
with the current behavior.
common/src/logger/loggers/mod.rs Show resolved Hide resolved
common/src/logger/loggers/mod.rs Outdated Show resolved Hide resolved
common/src/logger/loggers/mod.rs Outdated Show resolved Hide resolved
@kylefleming
Copy link
Contributor

Sometimes, it isn't acceptable to log on stdout, because a CLI tool
may be required to return structured output on stdout.

Can you elaborate on the motivation for this PR? Is this only in response to the log capture issues when testing in CI? Are there any other reasons for this change?

The reason I ask is being Rust is currently in the process of fixing the log capturing mechanism in libtest, which would fix the issue of log output getting mixed up with json test result output.

Here's a bug report for the issue: rust-lang/rust#42474
And here's a PR that is currently in progress to fix it: rust-lang/rust#78227

@cbeck88
Copy link
Contributor Author

cbeck88 commented Oct 23, 2020

The motivation here is, I have CLI tools in the fog conformance test that need to produce specific structured output on STDOUT, which gets parsed by python as JSON, and if the logging also goes there, then the python code cannot work. I think the logging should always go to STDERR and never to STDOUT. And that would be my preferred fix, as that is the unix convention, and what glog does. But at this point that might break our partners or something, so a more conservative change is appropriate. I think logging to STDOUT is always wrong.

@cbeck88 cbeck88 requested a review from eranrund October 23, 2020 19:45
@cbeck88 cbeck88 merged commit d4ae898 into mobilecoinfoundation:master Oct 23, 2020
@cbeck88 cbeck88 deleted the stderr_logger branch October 23, 2020 20:15
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