Skip to content

Commit

Permalink
Fix log appender in InsecureSettingTests (opensearch-project#11866)
Browse files Browse the repository at this point in the history 
The appender was being added to the logger before the appender was
started. This can lead to logger errors with concurrently running tests
because the logger is static state shared across the JVM. See opensearch-project#10799.

I've also added a removeAppender call that was missing from
LoggersTests, but that is mostly hygiene and would not lead to failures.

Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
  • Loading branch information
@andrross @shiv0408
andrross authored and shiv0408 committed Apr 25, 2024
1 parent 575c957 commit bc90112
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 32 deletions.
63 changes: 34 additions & 29 deletions server/src/test/java/org/opensearch/common/logging/LoggersTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,40 +53,45 @@ public void testParameterizedMessageLambda() throws Exception {
appender.start();
final Logger testLogger = LogManager.getLogger(LoggersTests.class);
Loggers.addAppender(testLogger, appender);
Loggers.setLevel(testLogger, Level.TRACE);
try {
Loggers.setLevel(testLogger, Level.TRACE);

Throwable ex = randomException();
testLogger.error(() -> new ParameterizedMessage("an error message"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.ERROR));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an error message"));
Throwable ex = randomException();
testLogger.error(() -> new ParameterizedMessage("an error message"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.ERROR));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an error message"));

ex = randomException();
testLogger.warn(() -> new ParameterizedMessage("a warn message: [{}]", "long gc"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.WARN));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a warn message: [long gc]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining("long gc"));
ex = randomException();
testLogger.warn(() -> new ParameterizedMessage("a warn message: [{}]", "long gc"), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.WARN));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a warn message: [long gc]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining("long gc"));

testLogger.info(() -> new ParameterizedMessage("an info message a=[{}], b=[{}], c=[{}]", 1, 2, 3));
assertThat(appender.lastEvent.getLevel(), equalTo(Level.INFO));
assertThat(appender.lastEvent.getThrown(), nullValue());
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an info message a=[1], b=[2], c=[3]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(1, 2, 3));
testLogger.info(() -> new ParameterizedMessage("an info message a=[{}], b=[{}], c=[{}]", 1, 2, 3));
assertThat(appender.lastEvent.getLevel(), equalTo(Level.INFO));
assertThat(appender.lastEvent.getThrown(), nullValue());
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an info message a=[1], b=[2], c=[3]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(1, 2, 3));

ex = randomException();
testLogger.debug(() -> new ParameterizedMessage("a debug message options = {}", Arrays.asList("yes", "no")), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.DEBUG));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a debug message options = [yes, no]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(Arrays.asList("yes", "no")));
ex = randomException();
testLogger.debug(() -> new ParameterizedMessage("a debug message options = {}", Arrays.asList("yes", "no")), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.DEBUG));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a debug message options = [yes, no]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(Arrays.asList("yes", "no")));

ex = randomException();
testLogger.trace(() -> new ParameterizedMessage("a trace message; element = [{}]", new Object[] { null }), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.TRACE));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a trace message; element = [null]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(new Object[] { null }));
ex = randomException();
testLogger.trace(() -> new ParameterizedMessage("a trace message; element = [{}]", new Object[] { null }), ex);
assertThat(appender.lastEvent.getLevel(), equalTo(Level.TRACE));
assertThat(appender.lastEvent.getThrown(), equalTo(ex));
assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a trace message; element = [null]"));
assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(new Object[] { null }));
} finally {
Loggers.removeAppender(testLogger, appender);
appender.stop();
}
}

private Throwable randomException() {
Expand Down
7 changes: 4 additions & 3 deletions server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class InsecureSettingTests extends OpenSearchTestCase {
private List<String> rootLogMsgs = new ArrayList<>();
private AbstractAppender rootAppender;

protected void assertSettingWarning() {
private void assertSettingWarning() {
assertWarnings(
"[setting.name] setting was deprecated in OpenSearch and will be removed in a future release! See the breaking changes documentation for the next major version."
);
Expand All @@ -50,13 +50,14 @@ public void append(LogEvent event) {
InsecureSettingTests.this.rootLogMsgs.add(message);
}
};
Loggers.addAppender(LogManager.getRootLogger(), rootAppender);
rootAppender.start();
Loggers.addAppender(LogManager.getLogger(SecureSetting.class), rootAppender);
}

@After
public void removeInsecureSettingsAppender() {
Loggers.removeAppender(LogManager.getRootLogger(), rootAppender);
Loggers.removeAppender(LogManager.getLogger(SecureSetting.class), rootAppender);
rootAppender.stop();
}

public void testShouldRaiseExceptionByDefault() {
Expand Down

0 comments on commit bc90112

Please sign in to comment.