Skip to content

Commit 2245957

Browse files
authored
Logging: Make node name consistent in logger (#31588)
First, some background: we have 15 different methods to get a logger in Elasticsearch but they can be broken down into three broad categories based on what information is provided when building the logger. Just a class like: ``` private static final Logger logger = ESLoggerFactory.getLogger(ActionModule.class); ``` or: ``` protected final Logger logger = Loggers.getLogger(getClass()); ``` The class and settings: ``` this.logger = Loggers.getLogger(getClass(), settings); ``` Or more information like: ``` Loggers.getLogger("index.store.deletes", settings, shardId) ``` The goal of the "class and settings" variant is to attach the node name to the logger. Because we don't always have the settings available, we often use the "just a class" variant and get loggers without node names attached. There isn't any real consistency here. Some loggers get the node name because it is convenient and some do not. This change makes the node name available to all loggers all the time. Almost. There are some caveats are testing that I'll get to. But in *production* code the node name is node available to all loggers. This means we can stop using the "class and settings" variants to fetch loggers which was the real goal here, but a pleasant side effect is that the ndoe name is now consitent on every log line and optional by editing the logging pattern. This is all powered by setting the node name statically on a logging formatter very early in initialization. Now to tests: tests can't set the node name statically because subclasses of `ESIntegTestCase` run many nodes in the same jvm, even in the same class loader. Also, lots of tests don't run with a real node so they don't *have* a node name at all. To support multiple nodes in the same JVM tests suss out the node name from the thread name which works surprisingly well and easy to test in a nice way. For those threads that are not part of an `ESIntegTestCase` node we stick whatever useful information we can get form the thread name in the place of the node name. This allows us to keep the logger format consistent.
1 parent 5f30258 commit 2245957

File tree

32 files changed

+493
-136
lines changed

32 files changed

+493
-136
lines changed

benchmarks/src/main/resources/log4j2.properties

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
appender.console.type = Console
22
appender.console.name = console
33
appender.console.layout.type = PatternLayout
4-
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%m%n
4+
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%node_name]%marker %m%n
55

66
# Do not log at all if it is not really critical - we're in a benchmark
77
rootLogger.level = error
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
appender.console.type = Console
22
appender.console.name = console
33
appender.console.layout.type = PatternLayout
4-
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%m%n
4+
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%node_name]%marker %m%n
55

66
rootLogger.level = info
77
rootLogger.appenderRef.console.ref = console

distribution/src/config/log4j2.properties

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,13 @@ logger.action.level = debug
77
appender.console.type = Console
88
appender.console.name = console
99
appender.console.layout.type = PatternLayout
10-
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] %marker%m%n
10+
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] [%node_name]%marker %m%n
1111

1212
appender.rolling.type = RollingFile
1313
appender.rolling.name = rolling
1414
appender.rolling.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}.log
1515
appender.rolling.layout.type = PatternLayout
16-
appender.rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] %marker%.-10000m%n
16+
appender.rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] [%node_name]%marker %.-10000m%n
1717
appender.rolling.filePattern = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}-%d{yyyy-MM-dd}-%i.log.gz
1818
appender.rolling.policies.type = Policies
1919
appender.rolling.policies.time.type = TimeBasedTriggeringPolicy
@@ -38,7 +38,7 @@ appender.deprecation_rolling.type = RollingFile
3838
appender.deprecation_rolling.name = deprecation_rolling
3939
appender.deprecation_rolling.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_deprecation.log
4040
appender.deprecation_rolling.layout.type = PatternLayout
41-
appender.deprecation_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] %marker%.-10000m%n
41+
appender.deprecation_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] [%node_name]%marker %.-10000m%n
4242
appender.deprecation_rolling.filePattern = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_deprecation-%i.log.gz
4343
appender.deprecation_rolling.policies.type = Policies
4444
appender.deprecation_rolling.policies.size.type = SizeBasedTriggeringPolicy
@@ -55,7 +55,7 @@ appender.index_search_slowlog_rolling.type = RollingFile
5555
appender.index_search_slowlog_rolling.name = index_search_slowlog_rolling
5656
appender.index_search_slowlog_rolling.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_search_slowlog.log
5757
appender.index_search_slowlog_rolling.layout.type = PatternLayout
58-
appender.index_search_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%.-10000m%n
58+
appender.index_search_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%node_name]%marker %.-10000m%n
5959
appender.index_search_slowlog_rolling.filePattern = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_search_slowlog-%i.log.gz
6060
appender.index_search_slowlog_rolling.policies.type = Policies
6161
appender.index_search_slowlog_rolling.policies.size.type = SizeBasedTriggeringPolicy
@@ -72,7 +72,7 @@ appender.index_indexing_slowlog_rolling.type = RollingFile
7272
appender.index_indexing_slowlog_rolling.name = index_indexing_slowlog_rolling
7373
appender.index_indexing_slowlog_rolling.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_indexing_slowlog.log
7474
appender.index_indexing_slowlog_rolling.layout.type = PatternLayout
75-
appender.index_indexing_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%.-10000m%n
75+
appender.index_indexing_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%node_name]%marker %.-10000m%n
7676
appender.index_indexing_slowlog_rolling.filePattern = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_indexing_slowlog-%i.log.gz
7777
appender.index_indexing_slowlog_rolling.policies.type = Policies
7878
appender.index_indexing_slowlog_rolling.policies.size.type = SizeBasedTriggeringPolicy

docs/reference/index-modules/slowlog.asciidoc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ appender.index_search_slowlog_rolling.type = RollingFile
5050
appender.index_search_slowlog_rolling.name = index_search_slowlog_rolling
5151
appender.index_search_slowlog_rolling.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_search_slowlog.log
5252
appender.index_search_slowlog_rolling.layout.type = PatternLayout
53-
appender.index_search_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%.-10000m%n
53+
appender.index_search_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%node_name]%marker %.-10000m%n
5454
appender.index_search_slowlog_rolling.filePattern = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_search_slowlog-%i.log.gz
5555
appender.index_search_slowlog_rolling.policies.type = Policies
5656
appender.index_search_slowlog_rolling.policies.size.type = SizeBasedTriggeringPolicy
@@ -103,7 +103,7 @@ appender.index_indexing_slowlog_rolling.type = RollingFile
103103
appender.index_indexing_slowlog_rolling.name = index_indexing_slowlog_rolling
104104
appender.index_indexing_slowlog_rolling.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_indexing_slowlog.log
105105
appender.index_indexing_slowlog_rolling.layout.type = PatternLayout
106-
appender.index_indexing_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%.-10000m%n
106+
appender.index_indexing_slowlog_rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%node_name]%marker %.-10000m%n
107107
appender.index_indexing_slowlog_rolling.filePattern = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_index_indexing_slowlog-%i.log.gz
108108
appender.index_indexing_slowlog_rolling.policies.type = Policies
109109
appender.index_indexing_slowlog_rolling.policies.size.type = SizeBasedTriggeringPolicy

docs/reference/setup/logging-config.asciidoc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ appender.rolling.type = RollingFile <1>
2525
appender.rolling.name = rolling
2626
appender.rolling.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}.log <2>
2727
appender.rolling.layout.type = PatternLayout
28-
appender.rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] %marker%.-10000m%n
28+
appender.rolling.layout.pattern = [%d{ISO8601}][%-5p][%-25c{1.}] [%node_name]%marker %.-10000m%n
2929
appender.rolling.filePattern = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}-%d{yyyy-MM-dd}-%i.log.gz <3>
3030
appender.rolling.policies.type = Policies
3131
appender.rolling.policies.time.type = TimeBasedTriggeringPolicy <4>

qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilBootstrapChecksTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public void testInvalidValue() {
8787
final boolean enforceLimits = randomBoolean();
8888
final IllegalArgumentException e = expectThrows(
8989
IllegalArgumentException.class,
90-
() -> BootstrapChecks.check(new BootstrapContext(Settings.EMPTY, null), enforceLimits, emptyList(), "testInvalidValue"));
90+
() -> BootstrapChecks.check(new BootstrapContext(Settings.EMPTY, null), enforceLimits, emptyList()));
9191
final Matcher<String> matcher = containsString(
9292
"[es.enforce.bootstrap.checks] must be [true] but was [" + value + "]");
9393
assertThat(e, hasToString(matcher));

qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
import java.util.stream.Collectors;
5757
import java.util.stream.IntStream;
5858

59+
import static org.hamcrest.Matchers.endsWith;
5960
import static org.hamcrest.Matchers.equalTo;
6061
import static org.hamcrest.Matchers.hasItem;
6162
import static org.hamcrest.Matchers.lessThan;
@@ -319,9 +320,11 @@ public void testPrefixLogger() throws IOException, IllegalAccessException, UserE
319320
assertThat(events.size(), equalTo(expectedLogLines + stackTraceLength));
320321
for (int i = 0; i < expectedLogLines; i++) {
321322
if (prefix == null) {
322-
assertThat(events.get(i), startsWith("test"));
323+
assertThat("Contents of [" + path + "] are wrong",
324+
events.get(i), startsWith("[" + getTestName() + "] test"));
323325
} else {
324-
assertThat(events.get(i), startsWith("[" + prefix + "] test"));
326+
assertThat("Contents of [" + path + "] are wrong",
327+
events.get(i), startsWith("[" + getTestName() + "][" + prefix + "] test"));
325328
}
326329
}
327330
}
@@ -357,6 +360,24 @@ public void testProperties() throws IOException, UserException {
357360
}
358361
}
359362

363+
public void testNoNodeNameWarning() throws IOException, UserException {
364+
setupLogging("no_node_name");
365+
366+
final String path =
367+
System.getProperty("es.logs.base_path") +
368+
System.getProperty("file.separator") +
369+
System.getProperty("es.logs.cluster_name") + ".log";
370+
final List<String> events = Files.readAllLines(PathUtils.get(path));
371+
assertThat(events.size(), equalTo(2));
372+
final String location = "org.elasticsearch.common.logging.LogConfigurator";
373+
// the first message is a warning for unsupported configuration files
374+
assertLogLine(events.get(0), Level.WARN, location, "\\[null\\] Some logging configurations have %marker but don't "
375+
+ "have %node_name. We will automatically add %node_name to the pattern to ease the migration for users "
376+
+ "who customize log4j2.properties but will stop this behavior in 7.0. You should manually replace "
377+
+ "`%node_name` with `\\[%node_name\\]%marker ` in these locations:");
378+
assertThat(events.get(1), endsWith("no_node_name/log4j2.properties"));
379+
}
380+
360381
private void setupLogging(final String config) throws IOException, UserException {
361382
setupLogging(config, Settings.EMPTY);
362383
}

qa/evil-tests/src/test/resources/org/elasticsearch/common/logging/config/log4j2.properties

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
appender.console.type = Console
22
appender.console.name = console
33
appender.console.layout.type = PatternLayout
4-
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%m%n
4+
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%test_thread_info]%marker %m%n
55

66
appender.file.type = File
77
appender.file.name = file
88
appender.file.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}.log
99
appender.file.layout.type = PatternLayout
10-
appender.file.layout.pattern = [%p][%l] %marker%m%n
10+
appender.file.layout.pattern = [%p][%l] [%test_thread_info]%marker %m%n
1111

1212
rootLogger.level = info
1313
rootLogger.appenderRef.console.ref = console
@@ -23,10 +23,9 @@ appender.deprecation_file.type = File
2323
appender.deprecation_file.name = deprecation_file
2424
appender.deprecation_file.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_deprecation.log
2525
appender.deprecation_file.layout.type = PatternLayout
26-
appender.deprecation_file.layout.pattern = [%p][%l] %marker%m%n
26+
appender.deprecation_file.layout.pattern = [%p][%l] [%test_thread_info]%marker %m%n
2727

2828
logger.deprecation.name = deprecation
2929
logger.deprecation.level = warn
3030
logger.deprecation.appenderRef.deprecation_file.ref = deprecation_file
3131
logger.deprecation.additivity = false
32-

qa/evil-tests/src/test/resources/org/elasticsearch/common/logging/config/third/log4j2.properties

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
appender.console3.type = Console
22
appender.console3.name = console3
33
appender.console3.layout.type = PatternLayout
4-
appender.console3.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%m%n
4+
appender.console3.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%test_thread_info]%marker %m%n
55

66
logger.third.name = third
77
logger.third.level = debug

qa/evil-tests/src/test/resources/org/elasticsearch/common/logging/deprecation/log4j2.properties

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
appender.console.type = Console
22
appender.console.name = console
33
appender.console.layout.type = PatternLayout
4-
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] %marker%m%n
4+
appender.console.layout.pattern = [%d{ISO8601}][%-5p][%-25c] [%test_thread_info]%marker %m%n
55

66
appender.file.type = File
77
appender.file.name = file
88
appender.file.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}.log
99
appender.file.layout.type = PatternLayout
10-
appender.file.layout.pattern = [%p][%l] %marker%m%n
10+
appender.file.layout.pattern = [%p][%l] [%test_thread_info]%marker %m%n
1111

1212
rootLogger.level = info
1313
rootLogger.appenderRef.console.ref = console
@@ -17,7 +17,7 @@ appender.deprecation_file.type = File
1717
appender.deprecation_file.name = deprecation_file
1818
appender.deprecation_file.fileName = ${sys:es.logs.base_path}${sys:file.separator}${sys:es.logs.cluster_name}_deprecation.log
1919
appender.deprecation_file.layout.type = PatternLayout
20-
appender.deprecation_file.layout.pattern = [%p][%l] %marker%m%n
20+
appender.deprecation_file.layout.pattern = [%p][%l] [%test_thread_info]%marker %m%n
2121

2222
logger.deprecation.name = deprecation
2323
logger.deprecation.level = warn

0 commit comments

Comments
 (0)