You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/02/21 21:23:02 UTC

[GitHub] [logging-log4j2] dfa1 opened a new pull request #770: Initial support for detached markers in SLF4J binding

dfa1 opened a new pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770


   Hello all,
   
   please accept this small patch from a long time user of log4j: it will enable using log4j 2.x in several applications here at @six-group!
   
   This proposal allows creating detached `Markers` as specified by the `IMarkerFactory` interface. A detached marker allows the caller to inject non-constant data that can be used to implement structured logging (my use-case). Because of this, they cannot be cached in `Log4jMarkerFactory.markerMap` or converted.
    
   Please let me know if you require additional modifications (e.g. unit tests perhaps?)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r828470138



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       This `MarkerManager.getMarker(name)` invocation causes the `name` to be tracked in the `org.apache.logging.log4j.MarkerManager.MARKERS` collection, the proposed change only avoids the slf4j adapters cache of markers converted into the slf4j api.
   
   For this to work, log4j2 would also need to be aware of detached markers, which would also require changes to `MarkerManager.Log4jMarker.isInstanceOf(Marker marker)` which currently does a reference equality check rather than checking the string value.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073193347


   @rgoers thanks for yours the honest feedback! :) Maintaining my own fork(s) of SLF4J is what I wanted to avoid by proposing this PR!
   
   After this long discussion, I realized that what **I really** need is just a way to avoid:
   
   ```java
   return addMarkerIfAbsent(name, convertMarker(marker));
   ```
   
   do you see a clean way to skip this conversion? maybe a feature toggle (config)? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073168934


   @dfa1 I do understand your dilemma. You already have existing code that has implemented this in the only way possible using SLF4J, which is a hack, However, I have a real problem implementing something that encourages doing things the wrong way. I don't want to see you have to maintain your private workaround. I'd rather see you spend time improving what Log4j does to make doing it the right way even easier. This change doesn't do that. The question I would ask is if in the long run you would be better off maintaining your own fork of the slf4j-log4j bridge or in spending the effort to do it the right way.
   
   FWIW, I did a search of Jira and only one other issue (LOG4J2-585) mentions detached Markers and that was to agree they should not be implemented in Log4j 2. To be honest, a surprisingly low number of users use Markers despite the utility they provide. But you are the first to have ever requested support for SLF4J's detached markers. 
   
   I guess I am at the point with this PR that I personally don't want to merge it. But if another committer disagrees I won't veto it if they do. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1050256000


   I'm going to be quite busy for a couple of days but I will try to take a look. It might be only next week though. Thank you for your patience! That should not prevent someone else on the team to look at this of course. Do take note that we are currently taking time to validate a release candidate for 2.17.2.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072958711


   @rgoers yes, of course :-)  Let me try to explain clearly: in https://www.six-group.com/ there is an internal requirement to use a structured logging format (called unified log event) for all our applications. This format is quite rich and detailed: there are predefined events for auditing, security incidents and so on. 
   
   The initial implement was simply `JsonLayout` + fluent bit configuration (hundreds of lines of configuration): but that is not enough to express certain combination of logs (and quite complex to understand and hard to test).
   
   Then, after some struggling with fluent bit, I tried to write a custom plugin in log4j2 to have a `UnifiedLogEventLayout`. The experiment was successful : now it is possible to unit test all the possible events and I must say was also a learning experience (i.e. log4j plugins are awesome!). 
   
   And here the main point: I'm using custom SLF4J `Marker` instances to pass extra information like structured arg or predefined events. I don't expect to create thousands of markers per second, but usually we create few per day per application node (i.e. I'm not worried about performance at all).
   
   Example:
   ```java
   HealthCheck pojo = ...;
   LOGGER.info(StructuredArg.of(healthPojo), "health check status");
   ```
   
   `StructuredArg` implements `Marker` and `getName` yields always `STRUCTURED_ARG` but data is always different, so it cannot be cached by "name" (the default behaviour we have in log4j 2.17.x):
   
   https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java#L379-L388
   
   My first attempt to workaround the problem was to wrap my marker with `Log4jMarker` (to go via line 383). To implement this workaround I used a custom `org.slf4j.impl.StaticMarkerBinder`: problem is that now all application have 2 `StaticMakerBinders` and the order matters.
   
   My second attempt was this one (in the hope that someone else finds this useful too!).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072954499


   @dfa1 In your comment above you said you do have a use case (one would hope so for you to be implementing this) but you didn't explain your use case. All you really said is you want detached markers. Why? I can't think of a single thing that would be useful for that would be better done some other way. I do not want to add support for this without fully understanding how it is useful.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830335776



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       @carterkozak looks like that `MarkerManager.Log4jMarker` is already string-value:
   
   ```java 
   final Marker marker = (Marker) o;
   return name.equals(marker.getName());
   ```
   
   and Log4J implementation of SLF4J marker is doing:
   
   ```
   final Log4jMarker other = (Log4jMarker) obj;
   		if (!Objects.equals(marker, other.marker)) {
   			return false;
   		}
   		return true;
   ```
   
   did I miss something?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830331326



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;

Review comment:
       I do have a use case :-) 
   
   I will call this method *once* with "STRUCTURE_ARG". This is needed to avoid caching of the incoming `Marker` instance by name (all my markers are value-based classes). 
   
   In `Log4jLogger` there is the following code:
   
   ```java
   @Override
   public void info(final Marker marker, final String s) {
       logger.logIfEnabled(FQCN, Level.INFO, getMarker(marker), s);
   }
   ```
   
   the ultimate goal of `detachedMarkers` is to skip the last line of the `getMarker` method :
   
   ```java
   public Marker getMarker(final Marker marker) {
           if (detachedMarkers.contains(name)) {
               return marker;
           }
           final Marker m = markerMap.get(name);
           if (m != null) {
               return m;
           }
           return addMarkerIfAbsent(name, convertMarker(marker));  <-- I want to skip this processing
   }
   ```
   
   PS
   Of course, it could be abused by calling it with millions of unique strings but that is true also for the non-detached version... 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073047035


   @rgoers @garydgregory thank you for the feedback ;)
   
   Actually we already have a lot of internal libraries and frameworks written against `slf4j-api`, and we do the actual binding on per application basis, some of them even uses quarkus + jboss-logging. The `StructuredArg` marker is needed for one of those intermediate libraries. But I can't disagree with you: the current approach is far from being perfect but sometimes we have to deal with what you have!
   
   @rgoers what we have is indeed very similar to log4j-audit! :-) (maybe a bit less generic)
     


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r829247444



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       1. I've never understood the point of a detached Marker. A marker is essentially nothing more than a String to be compared. Having multiple instances of the String makes no sense to me (which is why Log4j 2 didn't implement it).
   2. My fear with this is that it will lead to a bug report that SLF4J detached markers aren't detached. Saying we don't support them sends a clearer message.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830276791



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       So I think my point is, we may need a different log4j marker implementation than `MarkerManager.Log4jMarker` which handles string-value comparisons rather than identity comparisons.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072998257


   I have a product at work that does almost exactly what @rgoers describes, it works great, it's clean, and easy enough to understand. I encourage you as well to move those parts of your code to Log4j 2! 👍😀


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830646927



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -36,9 +38,11 @@
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final ConcurrentMap<String, Marker> markerMap = new ConcurrentHashMap<>();
+    private final Set<String> detachedMarkers = new CopyOnWriteArraySet<>();

Review comment:
       @ppkarwasz thank you for the review :) maybe the name of the field is misleading: should be "detachedMarkerNames"? Anyway, for my use case, the marker name is always "STRUCTURED_ARG" but every instance has different data (i.e. health check data). 
   
   Basically, log4j is designed to use markers as "glorified strings", instead I would like to use them as "data carrier" (see my my discussion with @rgoers below). I hope it is clear enough!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830338691



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       @carterkozak created unit test:
   
   ```java
       @Test
       void comparisonDetachedAndNonDetachedMarkers() {
           // Given
           Log4jMarkerFactory sut = new Log4jMarkerFactory();
   
           // When
           Marker nonDetachedMarker = sut.getMarker("name");
           Marker detachedMarker = sut.getDetachedMarker("name");
   
           // Then
           assertNotSame(nonDetachedMarker, detachedMarker);
           assertTrue(nonDetachedMarker.equals(detachedMarker));
           assertTrue(detachedMarker.equals(nonDetachedMarker));
       }
   ```
   
   does that looks good for you? (test is green)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830273789



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       I don't think this will work correctly when compared against a standard non-detached marker that log4j uses in filters because they use an instance comparison, not a string value comparison.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072958711


   @rgoers yes, of course :-)  Let me try to explain clearly: in https://www.six-group.com/ there is an internal requirement to use a structured logging format (called unified log event) for all our applications. This format is quite rich and detailed: there are predefined events for auditing, security incidents and so on. 
   
   The initial implement was simply `JsonLayout` + fluent bit configuration (hundreds of lines of configuration): but that is not enough to express certain combination of logs (and quite complex to understand and hard to test).
   
   Then, after some struggling with fluent bit, I tried to write a custom plugin in log4j2 to have a `UnifiedLogEventLayout`. The experiment was successful : now it is possible to unit test all the possible events and I must say was also a learning experience (i.e. log4j plugins are awesome!). 
   
   And here the main point: I'm using custom SLF4J `Marker` instances to pass extra information like structured arg or predefined events. I don't expect to create thousands of markers per second, but usually we create few per day per application node (i.e. I'm not worried about performance at all).
   
   Example:
   ```java
   HealthCheck pojo = ...;
   LOGGER.info(StructuredArg.of(healthPojo), "health check status");
   ```
   
   `StructuredArg` implements `Marker` and `getName` yields always `STRUCTURED_ARG` but data is always different, so it cannot be cached by "name" (the default behaviour we have in log4j 2.17.x):
   
   https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java#L379-L388
   
   My first attempt to workaround the problem was to wrap my marker with `Log4jMarker` (to go via line 383). To implement this workaround I used a custom `org.slf4j.impl.StaticMarkerBinder`: problem is that now all application have 2 `StaticMakerBinders` and, since the order of Jars in the classpath matters, now we have a *second workaround* to manually specify the classpath for all applications (this is working but also very annoying!). 
   
   My second attempt was this one (in the hope that someone else finds this useful too!).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830310516



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       I'm starting to see the problem... let me try to implement such value based marker :)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072974525


   Ok. So your big mistake here is trying to do this with the SLF4J API. It wasn't designed to support structured event logging. I fought for that years ago and gave up and that was one of the reasons I created Log4j 2. You really want to use a MapMessage or extend it. I have teams that are doing exactly this to capture data to display in Kibana dashboards. WE still use Markers, but the Marker simply identifies it a) as an event and b) the type of event. The second could have been captured in the event itself. 
   
   This is the code that logs generates the event.
   
   ```
       public void recordInbound(Inbound in, Vendors vendor) {
           InboundLogEntry inboundLogEntry = new InboundLogEntry();
           inboundLogEntry.setVendor(vendor.name());
           inboundLogEntry.setInbound(true);
           inboundLogEntry.setDestination(in.getDestination());
           inboundLogEntry.setRefId(in.getId());
           inboundLogEntry.setMediaUrls(in.getMediaUrls());
           inboundLogEntry.setSource(in.getSource());
           inbound.ogEntry.setMessageId(in.getMessageId());
           inboundLogEntry.logEvent();
       }
   ```
   and here is the logEvent method.
   ```
       public void logEvent() {
           StackTraceElement element = StackLocatorUtil.getStackTraceElement(2);
           LOGGER.atInfo().withLocation(element).withMarker(INBOUND_EVENT).log(this);
       }
   ```
   Note that a detached marker is not required as all the variable data is in the event.
   
   Finally, in the logging configuration we use JsonTemplateLayout and include in the template
   ```
     "event.action": {
       "$resolver": "marker",
       "field": "name"
     },
     "event.data": {
       "$resolver": "map",
       "stringified": true
     },
   ```
   So the Marker will be logged as the event.action field and all the data will be included as attributes under event.data - such as event.data.vendor, event.data.destination, etc.
   
   So this is exactly why I am not in favor of merging this PR as the Log4j API is much richer than SLF4J. You do not need to convert your whole application to use SLF4J but you should modify anything generating structured events.
   
   I should also mention that @jvz looked into structured event logging with SLF4J and found a framework that does it, but it is a complete hack to get around the limitations of SLF4J.
   
   One other thing. You can find a more extended implementation of this in [Log4j-Audit](https://github.com/apache/logging-log4j-audit). It uses a "catalog" to define the events and the Catalog is converted into Java interfaces, one for each event. A Java Proxy is used to populate the events using the Interface methods. But in the end it ends up publishing an AuditMessage (which extends StructuredDataMessage, which extends MapMessage) and AuditLogger's logEvent method uses an Audit Marker to identify it as an audit event.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830331326



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;

Review comment:
       I do have a use case :-) 
   
   I will call this method *once* with "STRUCTURE_ARG". This is needed to avoid caching of the incoming `Marker` instance by name (all my markers are value-based classes). 
   
   In `Log4jLogger` there is the following code:
   
   ```java
   @Override
   public void info(final Marker marker, final String s) {
       logger.logIfEnabled(FQCN, Level.INFO, getMarker(marker), s);
   }
   ```
   
   the ultimate goal of `detachedMarkers` is to skip the last line of the `getMarker` method :
   
   ```
       public Marker getMarker(final Marker marker) {
           if (detachedMarkers.contains(name)) {
               return marker;
           }
           final Marker m = markerMap.get(name);
           if (m != null) {
               return m;
           }
           return addMarkerIfAbsent(name, convertMarker(marker));  <-- I want to skip this processing
   ```
   
   PS
   Of course, it could be abused by calling it with millions of unique strings but that is true also for the non-detached version... 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073201000


   That would be back to the same thing. Now you would have getMarker returning a detached marker. If I thought detached markers were a good idea then detachedMarker would be where it belongs.
   But remember, Log4jMarker has a public constructor. You are free to use ServiceLoader to locate the SLF4J Service Provider and get the Marker factory and then create Markers to your hearts content.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r829339159



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       thanks @rgoers for your time! In my use case a `Marker` contains health check data, a POJO that gets serialized in JSON for structured logging.  
   
   The only advantage of detached markers is that they can be garbage collected as soon as they not needed anymore... but you're right, this method is not fulfilling this promise (because it is delegating to the `MarkerFactory` that is caching it). 
   
   Let me try to fix that :-)

##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       @rgoers pushed a change: now I'm creating a new fresh instance of marker (not using the internal cache of `MarkerManager`).  It should fully implements the contract defined by SLF4J... right?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1069632985


   @garydgregory hello 👋 would it be possible to have another look? 😊
   Let me know how to improve this code to be included in a log4j release! 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r829348489



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       @rgoers pushed a change: now I'm creating a new fresh instance of marker (not using the internal cache of `MarkerManager`).  It should fully implements the contract defined by SLF4J... right?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830646927



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -36,9 +38,11 @@
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final ConcurrentMap<String, Marker> markerMap = new ConcurrentHashMap<>();
+    private final Set<String> detachedMarkers = new CopyOnWriteArraySet<>();

Review comment:
       @ppkarwasz thank you for the review :) maybe the name of the field is misleading: should be "detachedMarkerNames"! For my use case, the marker name is always "STRUCTURED_ARG" but every instance has different data (in my use case, health check data). 
   
   Basically, log4j is designed to use markers as "glorified strings", instead I would like to use them as "data carrier" (see my my discussion with @rgoers below). I hope it is clear enough!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830364137



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;

Review comment:
       I think the `Log4jLogger` marker conversion fallback (when the slf4j marker implementation is not a Log4jMarker) should use a simple wrapper around the input instance without any caching. Logging would incur object allocation, but this is a special case where markers don't come from the marker factory.
   https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java#L379-L388
   
   What do you think?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830457264



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       implementation is simply: 
   
   ```java
           @Override
           @PerformanceSensitive({"allocation", "unrolled"})
           public boolean isInstanceOf(final String markerName) {
               requireNonNull(markerName, "A marker name is required");
               if (markerName.equals(this.getName())) {
                   return true;
               }
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072958711


   @rgoers yes, of course :-)  Let me try to explain clearly: in https://www.six-group.com/ there is an internal requirement to use a structured logging format (called unified log event) for all our applications. This format is quite rich and detailed: there are predefined events for auditing, security incidents and so on. 
   
   The initial implement was simply `JsonLayout` + fluent bit configuration (hundreds of lines of configuration): but that is not enough to express certain combination of logs (and quite complex to understand and hard to test).
   
   Then, after some struggling with fluent bit, I tried to write a custom plugin in log4j2 to have a `UnifiedLogEventLayout`. The experiment was successful : now it is possible to unit test all the possible events and I must say was also a learning experience (i.e. log4j plugins are awesome!). 
   
   And here the main point: I'm using custom SLF4J `Marker` instances to pass extra information like structured arg or predefined events. I don't expect to create thousands of markers per second, but usually we create few per day per application node (i.e. I'm not worried about performance at all).
   
   Example:
   ```java
   HealthCheck pojo = ...;
   LOGGER.info(StructuredArg.of(healthPojo), "health check status");
   ```
   
   `StructuredArg` implements `Marker` and `getName` yields always `STRUCTURED_ARG` but data is always different, so it cannot be cached by "name" (the default behaviour we have in log4j 2.17.x):
   
   https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java#L379-L388
   
   My first attempt to workaround the problem was to wrap my marker with `Log4jMarker` (to go via line 383). To implement this workaround I used a custom `org.slf4j.impl.StaticMarkerBinder`: problem is that now all application have 2 `StaticMakerBinders` and, since the order of Jars in the classpath matters, now we have a *second workaround* to manually specify the classpath for all applications (this is working but also very annoying!). 
   
   My second attempt was this one (in the hope that someone else finds this useful too!). I hope my use case is clear enough now /cc @carterkozak. 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072958711


   @rgoers yes, of course :-)  Let me try to explain clearly: at @SIX-Group we have an internal requirement to use a structured logging format (called unified log event) for all our applications. This format is quite rich and detailed: there are predefined events for auditing, security incidents and so on. 
   
   The initial implement was simply `JsonLayout` + fluent bit configuration (hundreds of lines of configuration): but that is not enough to express certain combination of logs (and quite complex to understand and hard to test).
   
   Then, after some struggling with fluent bit, I tried to write a custom plugin in log4j2 to have a `UnifiedLogEventLayout`. The experiment was successful : now it is possible to unit test all the possible events and I must say was also a learning experience (i.e. log4j plugins are awesome!). 
   
   And here the main point: I'm using custom SLF4J `Marker` instances to pass extra information like structured arg or predefined events. I don't expect to create thousands of markers per second, but usually we create few per day per application node (i.e. I'm not worried about performance at all).
   
   Example:
   ```java
   HealthCheck pojo = ...;
   LOGGER.info(StructuredArg.of(healthPojo), "health check status");
   ```
   
   `StructuredArg` implements `Marker` and `getName` yields always `STRUCTURED_ARG` but data is always different, so it cannot be cached by "name" (the default behaviour we have in log4j 2.17.x):
   
   https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java#L379-L388
   
   My first attempt to workaround the problem was to wrap my marker with `Log4jMarker` (to go via line 383). To implement this workaround I used a custom `org.slf4j.impl.StaticMarkerBinder`: problem is that now all application have 2 `StaticMakerBinders` and the order matters.
   
   My second attempt was this one (in the hope that someone else finds this useful too!).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830456338



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       @carterkozak but that seems to be fine as well:
   
   ```java
           assertTrue(((Log4jMarker) detachedMarker).getLog4jMarker().isInstanceOf("name"));
           assertTrue(((Log4jMarker) nonDetachedMarker).getLog4jMarker().isInstanceOf("name"));
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r812896243



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -36,9 +38,11 @@
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final ConcurrentMap<String, Marker> markerMap = new ConcurrentHashMap<>();
+    private final Set<String> detachedMarkers = new CopyOnWriteArraySet<>();

Review comment:
       here we are track only the name of the markers




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1079376882


   @rgoers tried also your last suggestion too:
   
   > You are free to use ServiceLoader to locate the SLF4J Service Provider and get the Marker factory and then create Markers to your hearts content." 
   
   but unfortunately it is sensitive to classpath order since SLF4J is picking up the first provider: 
   
   ```java
   List<SLF4JServiceProvider> providersList = findServiceProviders();
   reportMultipleBindingAmbiguity(providersList);
   if (providersList != null && !providersList.isEmpty()) {
       PROVIDER = providersList.get(0); <-- only the first one is considered
       // SLF4JServiceProvider.initialize() is intended to be called here and nowhere else.
       PROVIDER.initialize();
   ```
   
   so far, my workaround is to provide my JAR with my custom layout implementation *before* any other jar in the classpath (this is done for about 20 Dockerfiles, it feels really dirty). 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1047241499


   Hi @dfa1 
   Thank you for the PR.
   You will need to add tests to this PR to prove the code does what you say it does.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1047476853


   @garydgregory  thank you for the review... please have a look now, I wrote few unit tests 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r812876417



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -36,9 +38,11 @@
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final ConcurrentMap<String, Marker> markerMap = new ConcurrentHashMap<>();
+    private final Set<String> detachedMarkers = new CopyOnWriteArraySet<>();

Review comment:
       Doesn't tracking the markers defeat the purpose of them being "detached"? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r828802299



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       @carterkozak thanks for the review! :) 
   
   Yes, this is exactly the point: *I tried to keep Log4j unaware of detached markers*. 
   
   Since Log4j doesn't support detached markers, while SLF4J does, the proposed implementation is wrapping a Lo4j's marker (that is cached internally as you mentioned) with a fresh instance of SLF4J's marker. 
   
   Unless I'm missing something big, there are also few newly written unit tests that can be used to disproof my understanding of the problem.   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072974525


   Ok. So your big mistake here is trying to do this with the SLF4J API. It wasn't designed to support structured event logging. I fought for that years ago and gave up and that was one of the reasons I created Log4j 2. You really want to use a MapMessage or extend it. I have teams that are doing exactly this to capture data to display in Kibana dashboards. WE still use Markers, but the Marker simply identifies it a) as an event and b) the type of event. The second could have been captured in the event itself. 
   
   This is the code that logs generates the event.
   
   ```
       public void recordInbound(Inbound in, Vendors vendor) {
           InboundLogEntry inboundLogEntry = new InboundLogEntry();
           inboundLogEntry.setVendor(vendor.name());
           inboundLogEntry.setInbound(true);
           inboundLogEntry.setDestination(in.getDestination());
           inboundLogEntry.setRefId(in.getId());
           inboundLogEntry.setMediaUrls(in.getMediaUrls());
           inboundLogEntry.setSource(in.getSource());
           inbound.ogEntry.setMessageId(in.getMessageId());
           inboundLogEntry.logEvent();
       }
   ```
   and here is the logEvent method.
   ```
       public void logEvent() {
           StackTraceElement element = StackLocatorUtil.getStackTraceElement(2);
           LOGGER.atInfo().withLocation(element).withMarker(INBOUND_EVENT).log(this);
       }
   ```
   Note that a detached marker is not required as all the variable data is in the event.
   
   Finally, in the logging configuration we use JsonTemplateLayout and include in the template
   ```
     "event.action": {
       "$resolver": "marker",
       "field": "name"
     },
     "event.data": {
       "$resolver": "map",
       "stringified": true
     },
   ```
   So the Marker will be logged as the event.action field and all the data will be included as attributes under event.data - such as event.data.vendor, event.data.destination, etc.
   
   So this is exactly why I am not in favor of merging this PR as the Log4j API is much richer than SLF4J. You do not need to convert your whole application to use SLF4J but you should modify anything generating structured events.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830449257



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;

Review comment:
       @carterkozak yes, I'm already doing this with custom `org.slf4j.impl.StaticMarkerBinder` that is wrapping the incoming marker with `Log4jMarker` (to go via line 383 instead of line 386). 
   
   Problem is that now our applications are sensitive to the order of the jars in the classpath: my binding must come *before* log4j-slf4j-impl. Do you see another way perhaps? (and thank you for the nice review by the way!)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830479861



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       I think the string method is fine, the overload which takes another marker is not




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830646927



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -36,9 +38,11 @@
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final ConcurrentMap<String, Marker> markerMap = new ConcurrentHashMap<>();
+    private final Set<String> detachedMarkers = new CopyOnWriteArraySet<>();

Review comment:
       @ppkarwasz thank you for the review :) maybe the name of the field is misleading: should be "detachedMarkerNames"! For my use case, the marker name is always "STRUCTURED_ARG" but every instance has different data (in my use case, health check data). 
   
   Basically, log4j is designed to use markers as "glorified strings", instead I would like to use them as "data carrier" (after my discussion with @rgoers, see below). I hope it is clear enough!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830276035



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;

Review comment:
       I thought the point of detached markers was so that we can create arbitrarily many without worrying about the impact on heap -- if we hold a collection `detachedMarkers` strings, that is no longer true, and likely results in a memory leak.
   I would suggest against supporting the `boolean detachMarker(final String name) {` method, at least until we have a well understood use-case for it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073293922


   @rgoers I'm already using public ctor in `Log4jMarker`: but in order to do that I need to "override" the code coming from log4j binding and now my application are sensitive to the order of jars in the classpath.  And that is the workaround I would like to  remove with this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1050199009


   @garydgregory is there something that you would like to change/improve in this PR? please let me know :-)  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830267182



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       @carterkozak @rgoers I pushed an additional commit to create fully detached markers (just to satisfy the SLF4J's contract). Is that enough? do you see any additional changes? 
   
   Thanks for your time!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073049642


   @rgoers  @carterkozak @garydgregory 
   
   *Current state*
   - this PR is implementing the SLF4J's contract in a reasonable way (my humble opinion)
       - test coverage is good 
       - it is possible to create detached markers directly
       - it is possible to detach an existing `Marker` (my use case with `StructuredArg`) to avoid any processing by log4j
       - detached markers can be filtered by name  (checked with @carterkozak  in `MarkerFilter`) 
       - `Marker.isInstanceOf(Marker)` is not working properly for detached markers because of identity comparison (this could be a problem for people using detached markers with log4j)
       
   I will be more than happy to:
   - finish this PR, adding anything you think is important
   - add these changes in `log4j-slf4j18-impl` too
   - provide feedback, testing and bug fixing after merging
   - help you to maintain this code 
   
   I don't see the point of using my time to maintain my "private" workarounds, I would be happy (and honored too) to use my time to collaborate with you on log4j 2.x instead! :)
   
   D.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073049642


   @rgoers  @carterkozak @garydgregory 
   
   *Current state*
   - this PR is implementing the SLF4J's contract is a reasonable way (my humble opinion)
       - test coverage is good 
       - it is possible to create directly detached markers 
       - it is possible to detach an existing `Marker` (my use case with `StructuredArg`) to avoid any processing by log4j
       - detached markers can be filtering by name  (checked with @carterkozak  in `MarkerFilter`) 
       - `Marker.isInstanceOf(Marker)` is not working properly for detached markers because of identify comparison (this could be a problem for people using detached markers with log4j)
       
   I will be more than happy to:
   - finish this PR, adding anything you think is important
   - add these changes in `log4j-slf4j18-impl` too
   - provide feedback, testing and bug fixing after merging
   - help you to maintain this code 
   
   I don't see the point in using my time to maintain my "private" workarounds, I would be happy (and honored too) to use my time to collaborate with you on log4j 2.x instead! :)
   
   My 2 cents,
   D.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830310516



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       I start to see the problem... let me try to implement such value based marker :)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830338691



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       @carterkozak just created a unit test to be sure:
   
   ```java
       @Test
       void comparisonDetachedAndNonDetachedMarkers() {
           // Given
           Log4jMarkerFactory sut = new Log4jMarkerFactory();
   
           // When
           Marker nonDetachedMarker = sut.getMarker("name");
           Marker detachedMarker = sut.getDetachedMarker("name");
   
           // Then
           assertNotSame(nonDetachedMarker, detachedMarker);
           assertTrue(nonDetachedMarker.equals(detachedMarker));
           assertTrue(detachedMarker.equals(nonDetachedMarker));
       }
   ```
   
   does that looks good for you? (test is green)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830456338



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       @carterkozak but that seems to be fine as well (see e513e737eed7a0dfa33662697efdd8a396555e32):
   
   ```java
           assertTrue(((Log4jMarker) detachedMarker).getLog4jMarker().isInstanceOf("name"));
           assertTrue(((Log4jMarker) nonDetachedMarker).getLog4jMarker().isInstanceOf("name"));
   ```
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1072974525


   Ok. So your big mistake here is trying to do this with the SLF4J API. It wasn't designed to support structured event logging. I fought for that years ago and gave up and that was one of the reasons I created Log4j 2. You really want to use a MapMessage or extend it. I have teams that are doing exactly this to capture data to display in Kibana dashboards. WE still use Markers, but the Marker simply identifies it a) as an event and b) the type of event. The second could have been captured in the event itself. 
   
   This is the code that logs generates the event.
   
   ```
       public void recordInbound(Inbound in, Vendors vendor) {
           InboundLogEntry inboundLogEntry = new InboundLogEntry();
           inboundLogEntry.setVendor(vendor.name());
           inboundLogEntry.setInbound(true);
           inboundLogEntry.setDestination(in.getDestination());
           inboundLogEntry.setRefId(in.getId());
           inboundLogEntry.setMediaUrls(in.getMediaUrls());
           inboundLogEntry.setSource(in.getSource());
           inbound.ogEntry.setMessageId(in.getMessageId());
           inboundLogEntry.logEvent();
       }
   ```
   and here is the logEvent method.
   ```
       public void logEvent() {
           StackTraceElement element = StackLocatorUtil.getStackTraceElement(2);
           LOGGER.atInfo().withLocation(element).withMarker(INBOUND_EVENT).log(this);
       }
   ```
   Note that a detached marker is not required as all the variable data is in the event.
   
   Finally, in the logging configuration we use JsonTemplateLayout and include in the template
   ```
     "event.action": {
       "$resolver": "marker",
       "field": "name"
     },
     "event.data": {
       "$resolver": "map",
       "stringified": true
     },
   ```
   So the Marker will be logged as the event.action field and all the data will be included as attributes under event.data - such as event.data.vendor, event.data.destination, etc.
   
   So this is exactly why I am not in favor of merging this PR as the Log4j API is much richer than SLF4J. You do not need to convert your whole application to use SLF4J but you should modify anything generating structured events.
   
   I should also mention that @jvz looked into structured event logging with SLF4J and found a framework that does it, but it is a complete hack to get around the limitations of SLF4J.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r812896243



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -36,9 +38,11 @@
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final ConcurrentMap<String, Marker> markerMap = new ConcurrentHashMap<>();
+    private final Set<String> detachedMarkers = new CopyOnWriteArraySet<>();

Review comment:
       here we are track only the name of the markers. Every markers has a "shared name": we don't track specific instances.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r829247444



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       1. I've never understood the point of a detached Marker. A marker is essentially nothing more than a String to be compared. Having multiple instances of the String makes no sense to me (which is why Log4j 2 didn't implement it).
   2. My fear with this is that it will lead to a bug report that SLF4J detached markers aren't detached. Saying we don't support them sends a clearer message.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r829339159



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,19 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        final org.apache.logging.log4j.Marker log4jMarker = MarkerManager.getMarker(name);

Review comment:
       thanks @rgoers for your time! In my use case a `Marker` contains health check data, a POJO that gets serialized in JSON for structured logging.  
   
   The only advantage of detached markers is that they can be garbage collected as soon as they not needed anymore... but you're right, this method is not fulfilling this promise (because it is delegating to the `MarkerFactory` that is caching it). 
   
   Let me try to fix that :-)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830645307



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -36,9 +38,11 @@
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final ConcurrentMap<String, Marker> markerMap = new ConcurrentHashMap<>();
+    private final Set<String> detachedMarkers = new CopyOnWriteArraySet<>();

Review comment:
       What about a `Map<String, WeakReference<Marker>>`?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830357475



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       Most checks don't use `.equals(`, rather `.isInstanceOf(`, for example https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/MarkerFilter.java#L70-L72




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 commented on a change in pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 commented on a change in pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#discussion_r830338691



##########
File path: log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jMarkerFactory.java
##########
@@ -113,26 +125,18 @@ public boolean exists(final String name) {
         return markerMap.containsKey(name);
     }
 
-    /**
-     * Log4j does not support detached Markers. This method always returns false.
-     * @param name The Marker name.
-     * @return {@code false}
-     */
     @Override
     public boolean detachMarker(final String name) {
-        return false;
+        if (name == null) {
+            return false;
+        }
+        detachedMarkers.add(name);
+        return true;
     }
 
-    /**
-     * Log4j does not support detached Markers for performance reasons. The returned Marker is attached.
-     * @param name The Marker name.
-     * @return The named Marker (unmodified).
-     */
     @Override
     public Marker getDetachedMarker(final String name) {
-        LOGGER.warn("Log4j does not support detached Markers. Returned Marker [{}] will be unchanged.", name);
-        return getMarker(name);
+        return new Log4jMarker(new MarkerManager.Log4jMarker(name));

Review comment:
       @carterkozak just created a unit test to make sure:
   
   ```java
       @Test
       void comparisonDetachedAndNonDetachedMarkers() {
           // Given
           Log4jMarkerFactory sut = new Log4jMarkerFactory();
   
           // When
           Marker nonDetachedMarker = sut.getMarker("name");
           Marker detachedMarker = sut.getDetachedMarker("name");
   
           // Then
           assertNotSame(nonDetachedMarker, detachedMarker);
           assertTrue(nonDetachedMarker.equals(detachedMarker));
           assertTrue(detachedMarker.equals(nonDetachedMarker));
       }
   ```
   
   does that looks good for you? (test is green)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] rgoers edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
rgoers edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073168934


   @dfa1 I do understand your dilemma. You already have existing code that has implemented this in the only way possible using SLF4J, which is a hack, However, I have a real problem implementing something that encourages doing things the wrong way. I don't want to see you have to maintain your private workaround. I'd rather see you spend time improving what Log4j does to make doing it the right way even easier. This change doesn't do that. The question I would ask is if in the long run you would be better off maintaining your own fork of the slf4j-log4j bridge or in spending the effort to do it the right way. Oh, and FYI I have to now create log4j-slf4j20-impl to support changes introduced there.
   
   FWIW, I did a search of Jira and only one other issue (LOG4J2-585) mentions detached Markers and that was to agree they should not be implemented in Log4j 2. To be honest, a surprisingly low number of users use Markers despite the utility they provide. But you are the first to have ever requested support for SLF4J's detached markers. 
   
   I guess I am at the point with this PR that I personally don't want to merge it. But if another committer disagrees I won't veto it if they do. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [logging-log4j2] dfa1 edited a comment on pull request #770: Initial support for detached markers in SLF4J binding

Posted by GitBox <gi...@apache.org>.
dfa1 edited a comment on pull request #770:
URL: https://github.com/apache/logging-log4j2/pull/770#issuecomment-1073193347


   @rgoers thanks for yours the honest feedback! :) Maintaining my own fork(s) of SLF4J is what I wanted to avoid by proposing this PR... I don't want to go there!
   
   After this long discussion, I realized that what **I really** need is just a way to avoid:
   
   ```java
   return addMarkerIfAbsent(name, convertMarker(marker));
   ```
   
   do you see a clean way to skip this conversion? maybe a feature toggle (config)? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org