Skip to content

Conversation

@rjernst
Copy link
Member

@rjernst rjernst commented Feb 28, 2025

All CLIs in elasticsearch support command line flags for controlling the output level. When --silent is used, the expectation is that normal logging is omitted. Yet the log4j logger is still configured to output error level logs. This commit sets the appropriate log level for log4j depending on the Terminal log level.

All CLIs in elasticsearch support command line flags for controlling the
output level. When --silent is used, the expectation is that normal
logging is omitted. Yet the log4j logger is still configured to output
error level logs. This commit sets the appropriate log level for log4j
depending on the Terminal log level.
@rjernst rjernst added :Core/Infra/Logging Log management and logging utilities >refactoring auto-backport Automatically create backport pull requests when merged v8.18.1 v8.19.0 v9.0.1 v9.1.0 labels Feb 28, 2025
@rjernst rjernst requested a review from a team as a code owner February 28, 2025 17:52
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 28, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst
Copy link
Member Author

rjernst commented Feb 28, 2025

Note to reviewers: I had wanted to change the console logger to output to stderr. Unfortunately that is difficult given our current LogConfigurator. We should still do that in the future, but this seemed a reasonable stopgap since the levels between Terminal and log4j should match.

Testing was also difficult, we don't really have many tests at this level. I tested manually by running a tool locally that had a log4j error message added artificially.

Copy link
Contributor

@JVerwolf JVerwolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@prdoyle
Copy link
Contributor

prdoyle commented Feb 28, 2025

The test failure could be real.

NodeJoinExecutorTests > testPerNodeLogging FAILED
--
  | java.lang.AssertionError: expected to see info message but did not
  | Expected: <0L>
  | but: was <1L>

@mosche
Copy link
Contributor

mosche commented Feb 28, 2025

I think @prdoyle is right, I've tried fixing the packaging tests this way initially. However the "is ready" check for ES in the packaging tests depends on some info logs.

@rjernst
Copy link
Member Author

rjernst commented Feb 28, 2025

I believe the test failure was because CLI tests would not restore the level, so if eg silent was tested with a cli test, logging in later tests was borked. I've tweaked this to capture and restore the log level in all CLI tests.


@Before
public void resetTerminal() throws IOException {
capturedLogLevel = LoggerFactory.provider().getRootLevel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've bitten ourselves before by tests doing this and then all subsequent tests create insane amounts of log output causing all kinds of havoc. Is there any reason not to do this in ESTestCase so that we catch all these scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. I pushed 2a9144a

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to relax this a little bit. For some reason, capturing and restoring per-test in ESTestCase causes several tests which test logging to fail, seemingly because they don't get the expected log levels. So I changed this to be at the suite level for now.

@rjernst rjernst enabled auto-merge (squash) March 2, 2025 16:53
@rjernst rjernst merged commit 39a2e88 into elastic:main Mar 2, 2025
17 checks passed
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Mar 2, 2025
All CLIs in elasticsearch support command line flags for controlling the
output level. When --silent is used, the expectation is that normal
logging is omitted. Yet the log4j logger is still configured to output
error level logs. This commit sets the appropriate log level for log4j
depending on the Terminal log level.
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Mar 2, 2025
All CLIs in elasticsearch support command line flags for controlling the
output level. When --silent is used, the expectation is that normal
logging is omitted. Yet the log4j logger is still configured to output
error level logs. This commit sets the appropriate log level for log4j
depending on the Terminal log level.
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Mar 2, 2025
All CLIs in elasticsearch support command line flags for controlling the
output level. When --silent is used, the expectation is that normal
logging is omitted. Yet the log4j logger is still configured to output
error level logs. This commit sets the appropriate log level for log4j
depending on the Terminal log level.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0

elasticsearchmachine pushed a commit that referenced this pull request Mar 2, 2025
All CLIs in elasticsearch support command line flags for controlling the
output level. When --silent is used, the expectation is that normal
logging is omitted. Yet the log4j logger is still configured to output
error level logs. This commit sets the appropriate log level for log4j
depending on the Terminal log level.
elasticsearchmachine pushed a commit that referenced this pull request Mar 2, 2025
All CLIs in elasticsearch support command line flags for controlling the
output level. When --silent is used, the expectation is that normal
logging is omitted. Yet the log4j logger is still configured to output
error level logs. This commit sets the appropriate log level for log4j
depending on the Terminal log level.
elasticsearchmachine pushed a commit that referenced this pull request Mar 4, 2025
All CLIs in elasticsearch support command line flags for controlling the
output level. When --silent is used, the expectation is that normal
logging is omitted. Yet the log4j logger is still configured to output
error level logs. This commit sets the appropriate log level for log4j
depending on the Terminal log level.

Co-authored-by: Lorenzo Dematté <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Logging Log management and logging utilities >refactoring Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants