Implement custom JUL bridge #96872

rjernst · 2023-06-15T13:51:47Z

The log4j JUL bridge turned out to have issues because it relied on java beans. This commit implements a custom bridge between JUL and Log4j.

closes #94613

The log4j JUL bridge turned out to have issues because it relied on java beans. This commit implements a custom bridge between JUL and Log4j. closes elastic#94613

elasticsearchmachine · 2023-06-15T13:52:14Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

uschindler

Hahah much more easy to understand. That's a quite simple imp and we need no external dependency.

Once we move to own logging framework this can easily be ported to directly use ESLogger or similar.

uschindler · 2023-06-15T15:00:04Z

Just the question: How did the beans shit break ES?

uschindler · 2023-06-15T15:03:56Z

server/src/main/java/org/elasticsearch/common/logging/JULBridge.java

+    public static void install() {
+        var rootJulLogger = java.util.logging.LogManager.getLogManager().getLogger("");
+        // clear out any other handlers, so eg we don't also print to stdout
+        for (var existingHandler : rootJulLogger.getHandlers()) {


is it a good idea to remove elements while executing the loop?

It's an array, so it doesn't actually change as remove is called.

should be safe. the getHandlers return a copied array of handlers

uschindler · 2023-06-15T15:13:08Z

server/src/main/java/org/elasticsearch/common/logging/JULBridge.java

+        Level.ALL
+    );
+
+    private static final TreeMap<Integer, Level> sortedLevelMap = new TreeMap<>(


You could also write:

sortedLevelMap = levelMap.entrySet().stream().collect(Collectors.toMap(e -> e.getKey().intValue(), Map.Entry::getValue, (k1, k2) -> throw new UnsupportedOperationException(), TreeMap::new));

(I hate that theres no Collector method that allows to specify custom map factory, but uses default merge function. 🤮

We do have Maps.toUnmodifiableSortedMap that collects to a NavigableMap

Thanks! I used the Maps method and changed to NavigableMap

rjernst · 2023-06-15T15:22:28Z

How did the beans shit break ES?

PropertyChangeListener was unavailable at runtime because we don't read the java.desktop module.

rjernst · 2023-06-15T19:21:32Z

@elasticsearchmachine run elasticsearch-ci/part-2

rjernst · 2023-06-15T20:40:05Z

I addressed the feedback, and switched this to use the ES logging api instead of log4j directly.

uschindler · 2023-06-15T20:57:24Z

server/src/main/java/org/elasticsearch/common/logging/JULBridge.java

+import java.util.stream.Collectors;
+
+/**
+ * A Java Util Logging handler that writes log messages to log4j.


This should be EsLogger instead log4j

pgomulka

It looks good, I am worried that we perhaps are incorrectly formatting parameterised messages? or have I missed something?
I made some suggestions in rjernst#4

pgomulka · 2023-06-16T12:25:48Z

server/src/main/java/org/elasticsearch/common/logging/JULBridge.java

+    public static void install() {
+        var rootJulLogger = java.util.logging.LogManager.getLogManager().getLogger("");
+        // clear out any other handlers, so eg we don't also print to stdout
+        for (var existingHandler : rootJulLogger.getHandlers()) {


should be safe. the getHandlers return a copied array of handlers

pgomulka · 2023-06-16T12:57:32Z

server/src/main/java/org/elasticsearch/common/logging/JULBridge.java

+        Level level = translateJulLevel(record.getLevel());
+        Throwable thrown = record.getThrown();
+        if (thrown == null) {
+            logger.log(level, record.getMessage());


totally artificial to me, but turns out that jul allows a message to be null
https://github.com/qos-ch/slf4j/blob/master/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java#L309
this is how jul-slf4j bridge handles this.
do we want the same?

pgomulka · 2023-06-16T13:17:26Z

server/src/main/java/org/elasticsearch/common/logging/JULBridge.java

+        if (thrown == null) {
+            logger.log(level, record.getMessage());
+        } else {
+            logger.log(level, record::getMessage, thrown);


also we need to take into account formatting
I would do something along the lines here https://github.com/qos-ch/slf4j/blob/master/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java#L271
so logger.log(level, ()-> MessageFormat.formatMessage(record.getMessage,record.getParameters), thrown)

rjernst · 2023-06-16T16:36:50Z

@pgomulka You are right, I added handling for null and formatted messages, see e8900c0

uschindler · 2023-06-16T17:41:20Z

server/src/main/java/org/elasticsearch/common/logging/JULBridge.java

+    }
+
+    private Level translateJulLevel(java.util.logging.Level julLevel) {
+        Level log4jLevel = levelMap.get(julLevel);


Still log4j 🤨💪

pgomulka

LGTM,
we assert about log4j-api in testing, which I think is ok. Making this working with ES api would probably require changes to the test framework.

pgomulka · 2023-06-19T07:11:50Z

server/src/test/java/org/elasticsearch/common/logging/JULBridgeTests.java

+
+package org.elasticsearch.common.logging;
+
+import org.apache.logging.log4j.Level;


I guess this is fine, but perhaps we should be making sure that the bridge works between jul-es_logging_api

rjernst · 2023-06-20T16:04:06Z

I removed log4j from the impl, but it remains in tests because that is what the mock logger works on. We can followup with a way for the ES logger to be mocked.

elastic/elasticsearch#96872

Implement custom JUL bridge

430fe31

The log4j JUL bridge turned out to have issues because it relied on java beans. This commit implements a custom bridge between JUL and Log4j. closes elastic#94613

rjernst added :Core/Infra/Logging Log management and logging utilities >refactoring v8.9.0 labels Jun 15, 2023

rjernst requested review from ChrisHegarty and pgomulka June 15, 2023 13:51

fix metadata

871354a

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jun 15, 2023

rjernst mentioned this pull request Jun 15, 2023

Implement custom JUL bridge #96871

Closed

spotless

7a316a5

uschindler reviewed Jun 15, 2023

View reviewed changes

iter

a79079b

uschindler reviewed Jun 15, 2023

View reviewed changes

rjernst added 3 commits June 15, 2023 14:06

fix javadoc and imports

adf8717

spotless

b3f4258

Merge branch 'main' into logging/jul2

a75db21

pgomulka reviewed Jun 16, 2023

View reviewed changes

rjernst added 2 commits June 16, 2023 09:34

add null and formatted message

e8900c0

spotless

5249d93

uschindler reviewed Jun 16, 2023

View reviewed changes

pgomulka approved these changes Jun 19, 2023

View reviewed changes

scrub log4j from impl

9a74288

rjernst added 2 commits June 20, 2023 08:59

Merge branch 'main' into logging/jul2

2fb8bba

create MessageFormat explicitly with locale

a5d582f

rjernst merged commit 7d8aac3 into elastic:main Jun 20, 2023

floragunncom pushed a commit to floragunncom/search-guard that referenced this pull request Mar 3, 2024

fix junit tests - commons-logging.properties

0f98325

elastic/elasticsearch#96872

floragunncom pushed a commit to floragunncom/search-guard that referenced this pull request Mar 3, 2024

fix junit tests - commons-logging.properties

48f8c8a

elastic/elasticsearch#96872

floragunncom pushed a commit to floragunncom/search-guard that referenced this pull request Mar 3, 2024

fix junit tests - commons-logging.properties

fc36a47

elastic/elasticsearch#96872


		package org.elasticsearch.common.logging;

		import org.apache.logging.log4j.Level;

Implement custom JUL bridge #96872

Implement custom JUL bridge #96872

Uh oh!

Conversation

rjernst commented Jun 15, 2023

Uh oh!

elasticsearchmachine commented Jun 15, 2023

Uh oh!

uschindler left a comment

Choose a reason for hiding this comment

Uh oh!

uschindler commented Jun 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uschindler Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jun 15, 2023

Uh oh!

rjernst commented Jun 15, 2023

Uh oh!

rjernst commented Jun 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jun 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

uschindler Jun 15, 2023 •

edited

Loading