[BUG] legacy code in LogConfigurator

[rursprung]

Describe the bug
stumbled across this by chance, the comment claims that this should've been removed with ES 7.0 (which obviously didn't happen if it was still present in 7.10.2 at the time of forking away). it should be re-considered if this is still needed and be refactored accordingly (either by fulfilling the promise of the comment or by removing the comment and cleaning up the code as needed).

OpenSearch/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java

Lines 192 to 201 in 6629b09

	/*
	* Subclass the properties configurator to hack the new pattern in
	* place so users don't have to change log4j2.properties in
	* a minor release. In 7.0 we'll remove this and force users to
	* change log4j2.properties. If they don't customize log4j2.properties
	* then they won't have to do anything anyway.
	*
	* Everything in this subclass that isn't marked as a hack is copied
	* from log4j2's source.
	*/

To Reproduce
n/a

Expected behavior
no code which has a comment stating that it should've been removed a long time ago.

Plugins
n/a

Screenshots
n/a

Host/Environment (please complete the following information):
n/a

Additional context
stumbled across this because this code tries to scan all files present in the config/ folder on startup and it failed on some file for us (i still don't know why... but got a workaround now so i can safely ignore the problem for the time being). it'd be great if it wouldn't have to scan all files even just for startup time reasons (as plugin configs are now also in this folder there will be more and more files to scan, increasing the startup time - not by much, but still: "death by a thousand cuts" applies here as well).

[tlfeng]

@rursprung Could you help me double check, it is the "hack" part that cause problem to you?

OpenSearch/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java

Lines 211 to 229 in 2c27dfd

	// Hack the new pattern into place
	for (String name : properties.stringPropertyNames()) {
	if (false == name.endsWith(".pattern")) continue;
	// Null is weird here but we can't do anything with it so ignore it
	String value = properties.getProperty(name);
	if (value == null) continue;
	// Tests don't need to be changed
	if (value.contains("%test_thread_info")) continue;
	/*
	* Patterns without a marker are sufficiently customized
	* that we don't have an opinion about them.
	*/
	if (false == value.contains("%marker")) continue;
	if (false == value.contains("%node_name")) {
	locationsWithDeprecatedPatterns.add(source.getLocation());
	properties.setProperty(name, value.replace("%marker", "[%node_name]%marker "));
	}
	}
	// end hack

I want to make sure that removing the customized override of the method getConfiguration() can solve your problem.

I think the part of code can be removed safely in next major version, because:
1 there is already a warning message shown to the user:

OpenSearch/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java

Lines 257 to 262 in 2c27dfd

	LogManager.getLogger(LogConfigurator.class)
	.warn(
	"Some logging configurations have %marker but don't have %node_name. "
	+ "We will automatically add %node_name to the pattern to ease the migration for users who customize "
	+ "log4j2.properties but will stop this behavior in 7.0. You should manually replace `%node_name` with "
	+ "`[%node_name]%marker ` in these locations:\n {}",

(This link contains a actual example of the warning message: https://forum.search-guard.com/t/no-known-master-node/1460)
2 it has been declared as a breaking change in document: https://www.elastic.co/guide/en/elasticsearch/reference/6.5/breaking-changes-6.5.html#breaking_65_logging_changes or https://github.com/elastic/elasticsearch/blob/v6.5.0/docs/reference/migration/migrate_6_5.asciidoc#logging-changes

In addition, scanning all files from config directory can not be avoided after fulfilling the promise of code removal, because scanning all files is an old behavior since 2016:

OpenSearch/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java

Lines 129 to 131 in 2c27dfd

	* Configure logging reading from any log4j2.properties found in the config directory and its
	* subdirectories from the specified environment. Will also configure logging to point the logs
	* directory from the specified environment.

If you would suggest to avoid scanning files in config folder, then I think it's better to create a separate issue, and that will need more work.

Additional context:

1. an explanation of the log pattern: https://discuss.elastic.co/t/the-parsing-of-the-log4j2-configuration-file/161023
2. about the "marker" in the log pattern. Adding %marker in log format definition of the log4j property file can let the marker name shows in the actual log (https://stackoverflow.com/a/58141537/11408564), and the "marker" name can be "index name [shard id]" or other defined words:
   

   OpenSearch/server/src/main/java/org/opensearch/common/logging/Loggers.java

   Line 79 in ff2d5be

   public static Logger getLogger(String loggerName, ShardId shardId) {

[tlfeng]

Thank you for reporting the legacy code to be removed!
The legacy code in LogConfigurator class have been removed by PR #4568 / commit 3ca749f.
So that the node name will not be added into log pattern for customized log4j property file from version 3.0
The version that shown in the warning message for the code removal is also updated to 3.0, by PR #4569. The warning message will be updated in version 2.3.1 and 2.4.0 and above.

I will close the issue now, if you think scanning all files from config directory is a problem, please open a new issue.

[rursprung]

I will close the issue now, if you think scanning all files from config directory is a problem, please open a new issue.

thanks! i'll re-verify once 3.0.0 is coming out and the removal is in there (it's not a priority for us right now, thus i won't bother testing the latest main version manually) and raise a new issue with more details if there's still a problem related to this.