-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Capture stdout and stderr to log4j log #50259
Conversation
This commit overrides the stdout and stderr print streams to be redirected to the main elasticsearch.log file. While the Elasticsearch project ensures stdout and stderr are not written to, the jdk or 3rd party libs may do this, which can be unexepected for users used to looking the elasticsearch log. closes elastic#50156
Pinging @elastic/es-core-infra (:Core/Infra/Logging) |
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.
LGTM I left one question
throw new IOException("buffer closed"); | ||
} | ||
if (b == 0) return; | ||
if (b == '\n') { |
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.
so this looks fine in plaintext logs, but might end up harder to analyze with JSON logs. Why not adding \n
to the buffer too?
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.
That would cause double the lines in the log. Think of it as making this equivalent to most of our logging calls do not contain a newline in the message. Log4j inserts newlines per message emitted.
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.
makes sense, thank you
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.
Looks good as discussed, I left a question.
server/src/main/java/org/elasticsearch/common/logging/LoggingOutputStream.java
Outdated
Show resolved
Hide resolved
@elasticmachine run elasticsearch-ci/bwc |
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 looks good.
This commit overrides the stdout and stderr print streams to be redirected to the main elasticsearch.log file. While the Elasticsearch project ensures stdout and stderr are not written to, the jdk or 3rd party libs may do this, which can be unexepected for users used to looking the elasticsearch log. closes #50156
With this commit we avoid reading from stdout/stderr which might have been closed by the application and rely solely on the process' return code. Relates elastic/elasticsearch#50259
With this commit we avoid reading from stdout/stderr which might have been closed by the application and rely solely on the process' return code. Relates elastic/elasticsearch#50259
In elastic#50259 we redirected stdout and stderr to log4j, to capture jdk and external library messages. However, a typo in the method name used to redirect the stream in java means stdout is currently being duplicated twice, and stderr not captured. This commit corrects that mistake. Unfortunately this is at a level that cannot really be tested, thus we are still missing tests for this behavior.
In #50259 we redirected stdout and stderr to log4j, to capture jdk and external library messages. However, a typo in the method name used to redirect the stream in java means stdout is currently being duplicated twice, and stderr not captured. This commit corrects that mistake. Unfortunately this is at a level that cannot really be tested, thus we are still missing tests for this behavior.
In #50259 we redirected stdout and stderr to log4j, to capture jdk and external library messages. However, a typo in the method name used to redirect the stream in java means stdout is currently being duplicated twice, and stderr not captured. This commit corrects that mistake. Unfortunately this is at a level that cannot really be tested, thus we are still missing tests for this behavior.
This commit overrides the stdout and stderr print streams to be
redirected to the main elasticsearch.log file. While the Elasticsearch
project ensures stdout and stderr are not written to, the jdk or 3rd
party libs may do this, which can be unexepected for users used to
looking the elasticsearch log.
closes #50156