Consistently use slf4j/log4j bridge#139226
Conversation
Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM excellent, confirmed to work with e.g.
diff --git a/modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3BasicCredentialsRestIT.java b/modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3BasicCredentialsRestIT.java
index dfe3bbbc2613..8757263427d2 100644
--- a/modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3BasicCredentialsRestIT.java
+++ b/modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3BasicCredentialsRestIT.java
@@ -50,6 +50,8 @@ public class RepositoryS3BasicCredentialsRestIT extends AbstractRepositoryS3Rest
.keystore("s3.client." + CLIENT + ".access_key", ACCESS_KEY)
.keystore("s3.client." + CLIENT + ".secret_key", SECRET_KEY)
.setting("s3.client." + CLIENT + ".endpoint", s3Fixture::getAddress)
+ .systemProperty("es.insecure_network_trace_enabled", "true")
+ .setting("logger.org.apache.http.headers", "TRACE")
.build();
@ClassRule|
NB closes #137556 |
|
I proposed a test to verify the logging in Note that we restrict the AWS SDK v1 loggers in case they expose sensitive information: Should we do that for the AWS SDK v2 loggers too? I.e. everything under |
| .systemProperty("es.insecure_network_trace_enabled", "true") | ||
| .setting("logger.org.apache.http.headers", "TRACE") |
There was a problem hiding this comment.
We don't need these here, this was just something I mentioned in a review comment as how I verified by hand that it worked. RepositoryS3WireLoggingRestIT is a much more comprehensive test of this.
It turns out that the Azure SDK logs copiously at `ERROR` level for totally benign events such as a `BlobNotFound` response when trying to delete a blob that has already been deleted. Until elastic#139226 these messages were screamed into the void and caused no issue, but since that change landed we now see them in the server logs too. In due course we'd like to be able to capture these logs at a more appropriate level, maybe `DEBUG`, but in the meantime this commit removes the SLF4J bridge from the `repository-azure` module to redirect them back into the void.
It turns out that the Azure SDK logs copiously at `ERROR` level for totally benign events such as a `BlobNotFound` response when trying to delete a blob that has already been deleted. Until elastic#139226 these messages were screamed into the void and caused no issue, but since that change landed we now see them in the server logs too. In due course we'd like to be able to capture these logs at a more appropriate level, maybe `DEBUG`, but in the meantime this commit removes the SLF4J bridge from the `repository-azure` module to redirect them back into the void.
It turns out that the Azure SDK logs copiously at `ERROR` level for totally benign events such as a `BlobNotFound` response when trying to delete a blob that has already been deleted. Until #139226 these messages were screamed into the void and caused no issue, but since that change landed we now see them in the server logs too. In due course we'd like to be able to capture these logs at a more appropriate level, maybe `DEBUG`, but in the meantime this commit removes the SLF4J bridge from the `repository-azure` module to redirect them back into the void.
It turns out that the Azure SDK logs copiously at `ERROR` level for totally benign events such as a `BlobNotFound` response when trying to delete a blob that has already been deleted. Until #139226 these messages were screamed into the void and caused no issue, but since that change landed we now see them in the server logs too. In due course we'd like to be able to capture these logs at a more appropriate level, maybe `DEBUG`, but in the meantime this commit removes the SLF4J bridge from the `repository-azure` module to redirect them back into the void.
It turns out that the Azure SDK logs copiously at `ERROR` level for totally benign events such as a `BlobNotFound` response when trying to delete a blob that has already been deleted. Until elastic#139226 these messages were screamed into the void and caused no issue, but since that change landed we now see them in the server logs too. In due course we'd like to be able to capture these logs at a more appropriate level, maybe `DEBUG`, but in the meantime this commit removes the SLF4J bridge from the `repository-azure` module to redirect them back into the void.
It turns out that the Azure SDK logs copiously at `ERROR` level for totally benign events such as a `BlobNotFound` response when trying to delete a blob that has already been deleted. Until #139226 these messages were screamed into the void and caused no issue, but since that change landed we now see them in the server logs too. In due course we'd like to be able to capture these logs at a more appropriate level, maybe `DEBUG`, but in the meantime this commit removes the SLF4J bridge from the `repository-azure` module to redirect them back into the void.
|
@rjernst I think we're missing backports here, this only seems to be in the 9.3 and main branches at the moment. |
|
This is why we need |
Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. closes elastic#137556 Co-authored-by: David Turner <david.turner@elastic.co>
Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. closes elastic#137556 Co-authored-by: David Turner <david.turner@elastic.co>
Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. closes elastic#137556 Co-authored-by: David Turner <david.turner@elastic.co>
Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. closes elastic#137556 Co-authored-by: David Turner <david.turner@elastic.co>
* Consistently use slf4j/log4j bridge (#139226) Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. closes #137556 Co-authored-by: David Turner <david.turner@elastic.co> * remove param from main * fix dep * update license * more licenses --------- Co-authored-by: David Turner <david.turner@elastic.co>
* Consistently use slf4j/log4j bridge (#139226) Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. closes #137556 Co-authored-by: David Turner <david.turner@elastic.co> * remove param from main * license update * more licenses --------- Co-authored-by: David Turner <david.turner@elastic.co>
* Consistently use slf4j/log4j bridge (#139226) Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. closes #137556 Co-authored-by: David Turner <david.turner@elastic.co> * fix dep * fix licenses * more licenses * fix compile --------- Co-authored-by: David Turner <david.turner@elastic.co>
Some 3rd party dependencies use SLF4J but Elasticsearch internally uses log4j. Historically there were problems with using the log4j adapter for SLF4J, but those have gone away with the removal of SecurityManager. This commit updates uses of slf4j to consistently use the log4j bridge. Note that long term we should still aim for something more turnkey so that we don't end up with multiple copies of the bridge jar, but this change at least removes warnings about no SLF4J providers being found.
closes #137556