[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).

https://github.com/opensearch-project/OpenSearch/blob/6629b09aad2629fafb7aad998153867c2f3fe472/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java#L192-L201

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? https://github.com/opensearch-project/OpenSearch/blob/2c27dfddbdabbf064317bd315a5464ba5f41c1da/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java#L211-L229

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: https://github.com/opensearch-project/OpenSearch/blob/2c27dfddbdabbf064317bd315a5464ba5f41c1da/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java#L257-L262 (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: https://github.com/opensearch-project/OpenSearch/blob/2c27dfddbdabbf064317bd315a5464ba5f41c1da/server/src/main/java/org/opensearch/common/logging/LogConfigurator.java#L129-L131

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: https://github.com/opensearch-project/OpenSearch/blob/ff2d5be004509adae62ca39ff6f0efcdf47d950e/server/src/main/java/org/opensearch/common/logging/Loggers.java#L79

[tlfeng]

Thank you for reporting the legacy code to be removed! The legacy code in LogConfigurator class have been removed by PR https://github.com/opensearch-project/OpenSearch/pull/4568 / commit https://github.com/opensearch-project/OpenSearch/commit/3ca749f09bf284263391d296af56e01fb81e0cfa. 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 https://github.com/opensearch-project/OpenSearch/pull/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.