diff --git a/client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt b/client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt index cc179e12e3163..a9214e9333c4e 100644 --- a/client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt +++ b/client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt @@ -22,7 +22,6 @@ org.apache.http.entity.ContentType#create(java.lang.String,org.apache.http.NameV @defaultMessage ES's logging infrastructure uses log4j2 which we don't want to force on high level rest client users org.elasticsearch.common.logging.DeprecationLogger -org.elasticsearch.common.logging.ESLoggerFactory org.elasticsearch.common.logging.LogConfigurator org.elasticsearch.common.logging.LoggerMessageFormat org.elasticsearch.common.logging.Loggers diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java index bebdb320db4a1..8438c002c2a4e 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java @@ -27,7 +27,6 @@ import org.apache.logging.log4j.core.appender.ConsoleAppender; import org.apache.logging.log4j.core.appender.CountingNoOpAppender; import org.apache.logging.log4j.core.config.Configurator; -import org.apache.logging.log4j.spi.ExtendedLogger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.lucene.util.Constants; import org.elasticsearch.cli.UserException; @@ -301,7 +300,7 @@ public void testPrefixLogger() throws IOException, IllegalAccessException, UserE setupLogging("prefix"); final String prefix = randomAlphaOfLength(16); - final Logger logger = new PrefixLogger((ExtendedLogger) LogManager.getLogger("prefix_test"), "prefix_test", prefix); + final Logger logger = new PrefixLogger(LogManager.getLogger("prefix_test"), prefix); logger.info("test"); logger.info("{}", "test"); final Exception e = new Exception("exception"); @@ -332,7 +331,7 @@ public void testPrefixLoggerMarkersCanBeCollected() throws IOException, UserExce final int prefixes = 1 << 19; // to ensure enough markers that the GC should collect some when we force a GC below for (int i = 0; i < prefixes; i++) { // this has the side effect of caching a marker with this prefix - new PrefixLogger((ExtendedLogger) LogManager.getLogger("prefix" + i), "prefix" + i, "prefix" + i); + new PrefixLogger(LogManager.getLogger("logger" + i), "prefix" + i); } System.gc(); // this will free the weakly referenced keys in the marker cache diff --git a/server/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java b/server/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java deleted file mode 100644 index 20f52a742a2f2..0000000000000 --- a/server/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java +++ /dev/null @@ -1,64 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.elasticsearch.common.logging; - -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.spi.ExtendedLogger; - -/** - * Factory to get {@link Logger}s - */ -final class ESLoggerFactory { - - private ESLoggerFactory() { - - } - - static Logger getLogger(String prefix, String name) { - return getLogger(prefix, LogManager.getLogger(name)); - } - - static Logger getLogger(String prefix, Class clazz) { - /* - * At one point we didn't use LogManager.getLogger(clazz) because - * of a bug in log4j that has since been fixed: - * https://github.com/apache/logging-log4j2/commit/ae33698a1846a5e10684ec3e52a99223f06047af - * - * For now we continue to use LogManager.getLogger(clazz.getName()) - * because we expect to eventually migrate away from needing this - * method entirely. - */ - return getLogger(prefix, LogManager.getLogger(clazz.getName())); - } - - static Logger getLogger(String prefix, Logger logger) { - /* - * In a followup we'll throw an exception if prefix is null or empty - * redirecting folks to LogManager.getLogger. - * - * This and more is tracked in https://github.com/elastic/elasticsearch/issues/32174 - */ - if (prefix == null || prefix.length() == 0) { - return logger; - } - return new PrefixLogger((ExtendedLogger)logger, logger.getName(), prefix); - } -} diff --git a/server/src/main/java/org/elasticsearch/common/logging/Loggers.java b/server/src/main/java/org/elasticsearch/common/logging/Loggers.java index 9a5ab8730835a..1ee99c72e4d6f 100644 --- a/server/src/main/java/org/elasticsearch/common/logging/Loggers.java +++ b/server/src/main/java/org/elasticsearch/common/logging/Loggers.java @@ -57,7 +57,8 @@ public static Logger getLogger(Class clazz, ShardId shardId, String... prefix * Class and no extra prefixes. */ public static Logger getLogger(String loggerName, ShardId shardId) { - return ESLoggerFactory.getLogger(formatPrefix(shardId.getIndexName(), Integer.toString(shardId.id())), loggerName); + String prefix = formatPrefix(shardId.getIndexName(), Integer.toString(shardId.id())); + return new PrefixLogger(LogManager.getLogger(loggerName), prefix); } public static Logger getLogger(Class clazz, Index index, String... prefixes) { @@ -65,15 +66,15 @@ public static Logger getLogger(Class clazz, Index index, String... prefixes) } public static Logger getLogger(Class clazz, String... prefixes) { - return ESLoggerFactory.getLogger(formatPrefix(prefixes), clazz); + return new PrefixLogger(LogManager.getLogger(clazz), formatPrefix(prefixes)); } public static Logger getLogger(Logger parentLogger, String s) { - String prefix = null; + Logger inner = LogManager.getLogger(parentLogger.getName() + s); if (parentLogger instanceof PrefixLogger) { - prefix = ((PrefixLogger)parentLogger).prefix(); + return new PrefixLogger(inner, ((PrefixLogger)parentLogger).prefix()); } - return ESLoggerFactory.getLogger(prefix, parentLogger.getName() + s); + return inner; } private static String formatPrefix(String... prefixes) { diff --git a/server/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java b/server/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java index f46d360a3fa5b..8a4d43f4df84a 100644 --- a/server/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java +++ b/server/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java @@ -20,6 +20,7 @@ package org.elasticsearch.common.logging; import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.Marker; import org.apache.logging.log4j.MarkerManager; import org.apache.logging.log4j.message.Message; @@ -70,26 +71,27 @@ public String prefix() { * Construct a prefix logger with the specified name and prefix. * * @param logger the extended logger to wrap - * @param name the name of this prefix logger * @param prefix the prefix for this prefix logger */ - PrefixLogger(final ExtendedLogger logger, final String name, final String prefix) { - super(logger, name, null); + PrefixLogger(final Logger logger, final String prefix) { + super((ExtendedLogger) logger, logger.getName(), null); - final String actualPrefix = (prefix == null ? "" : prefix); + if (prefix == null || prefix.isEmpty()) { + throw new IllegalArgumentException("if you don't need a prefix then use a regular logger"); + } final Marker actualMarker; // markers is not thread-safe, so we synchronize access synchronized (markers) { - final Marker maybeMarker = markers.get(actualPrefix); + final Marker maybeMarker = markers.get(prefix); if (maybeMarker == null) { - actualMarker = new MarkerManager.Log4jMarker(actualPrefix); + actualMarker = new MarkerManager.Log4jMarker(prefix); /* * We must create a new instance here as otherwise the marker will hold a reference to the key in the weak hash map; as * those references are held strongly, this would give a strong reference back to the key preventing them from ever being * collected. This also guarantees that no other strong reference can be held to the prefix anywhere. */ // noinspection RedundantStringConstructorCall - markers.put(new String(actualPrefix), actualMarker); + markers.put(new String(prefix), actualMarker); } else { actualMarker = maybeMarker; } diff --git a/server/src/test/java/org/elasticsearch/common/logging/PrefixLoggerTests.java b/server/src/test/java/org/elasticsearch/common/logging/PrefixLoggerTests.java new file mode 100644 index 0000000000000..b0b6182fa4629 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/common/logging/PrefixLoggerTests.java @@ -0,0 +1,36 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.logging; + +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.containsString; + +public class PrefixLoggerTests extends ESTestCase { + public void testNullPrefix() { + Exception e = expectThrows(IllegalArgumentException.class, () -> new PrefixLogger(logger, null)); + assertThat(e.getMessage(), containsString("use a regular logger")); + } + + public void testEmptyPrefix() { + Exception e = expectThrows(IllegalArgumentException.class, () -> new PrefixLogger(logger, "")); + assertThat(e.getMessage(), containsString("use a regular logger")); + } +} \ No newline at end of file