[PATCH] AbstractAccessLogValve: Add pre/post add log element extension points

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] AbstractAccessLogValve: Add pre/post add log element extension points

Thomas Meyer
a sub class can extend before and after the addElement loop to
establish a context via thread-dependend CharArrayWriter object.
---
 java/org/apache/catalina/valves/AbstractAccessLogValve.java | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/java/org/apache/catalina/valves/AbstractAccessLogValve.java b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
index e23f49d3a5..5e774c2320 100644
--- a/java/org/apache/catalina/valves/AbstractAccessLogValve.java
+++ b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
@@ -685,9 +685,11 @@ public abstract class AbstractAccessLogValve extends ValveBase implements Access
             result = new CharArrayWriter(128);
         }
 
+        preLogAddElement(result);
         for (int i = 0; i < logElements.length; i++) {
             logElements[i].addElement(result, date, request, response, time);
         }
+        postLogAddElement(result);
 
         log(result);
 
@@ -699,6 +701,9 @@ public abstract class AbstractAccessLogValve extends ValveBase implements Access
 
     // -------------------------------------------------------- Protected Methods
 
+    protected void preLogAddElement(CharArrayWriter result) {}
+    protected void postLogAddElement(CharArrayWriter result) {}
+
     /**
      * Log the specified message.
      *
--
2.20.1


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AbstractAccessLogValve: Add pre/post add log element extension points

Romain Manni-Bucau
Hi

Why cant it be added as element earlier at pattern parsing time? Would
avoid to have two particular cases and keep current pattern/impl.
A %java or so sounds more natural no?


Le jeu. 21 janv. 2021 à 13:59, Thomas Meyer <[hidden email]> a écrit :

> a sub class can extend before and after the addElement loop to
> establish a context via thread-dependend CharArrayWriter object.
> ---
>  java/org/apache/catalina/valves/AbstractAccessLogValve.java | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> index e23f49d3a5..5e774c2320 100644
> --- a/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> +++ b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> @@ -685,9 +685,11 @@ public abstract class AbstractAccessLogValve extends
> ValveBase implements Access
>              result = new CharArrayWriter(128);
>          }
>
> +        preLogAddElement(result);
>          for (int i = 0; i < logElements.length; i++) {
>              logElements[i].addElement(result, date, request, response,
> time);
>          }
> +        postLogAddElement(result);
>
>          log(result);
>
> @@ -699,6 +701,9 @@ public abstract class AbstractAccessLogValve extends
> ValveBase implements Access
>
>      // -------------------------------------------------------- Protected
> Methods
>
> +    protected void preLogAddElement(CharArrayWriter result) {}
> +    protected void postLogAddElement(CharArrayWriter result) {}
> +
>      /**
>       * Log the specified message.
>       *
> --
> 2.20.1
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AbstractAccessLogValve: Add pre/post add log element extension points

Thomas Meyer
On Thu, Jan 21, 2021 at 03:28:12PM +0100, Romain Manni-Bucau wrote:
> Hi

Hi,
 
> Why cant it be added as element earlier at pattern parsing time? Would
> avoid to have two particular cases and keep current pattern/impl.
> A %java or so sounds more natural no?

Mhh, maybe I just share my idea of the concrete implementation, so you see
what I want to do:

Premise: The JUL logging is configured to log all below to an own handler.

public class LoggingAccessLogValve extends AbstractAccessLogValve {

        private static final Log log = LogFactory.getLog(LoggingAccessLogValve.class);
        private Map<CharArrayWriter, JsonGenerator> writers = new WeakHashMap<>();

        @Override
        protected void log(CharArrayWriter message) {
                if(log.isInfoEnabled()) {
                        log.info(message);
                }
        }

        protected void preLogAddElement(CharArrayWriter result) {
                JsonGenerator jsonWriter = Json.createGenerator(result);
                synchronized (this) {
                        writers.put(result,jsonWriter);
                }
                jsonWriter.writeStartObject();
        }

        protected void postLogAddElement(CharArrayWriter result) {
                JsonGenerator jsonWriter = null;
                synchronized (this) {
                        jsonWriter = writers.remove(result);
                }
                if(jsonWriter != null) {
                        jsonWriter.writeEnd();
                        jsonWriter.close();
                }
        }

        private class JsonAccessLogElementWrapper implements AccessLogElement {

                private final String jsonAttributeName;
                private final AccessLogElement delegate;

                public JsonAccessLogElementWrapper(AccessLogElement e, String name) {
                        delegate = e;
                        jsonAttributeName = name;
                }

                @Override
                public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
                        JsonGenerator jsonWriter = null;
                        synchronized (this) {
                                jsonWriter = writers.get(buf);
                        }
                        if(jsonWriter == null) {
                                // TODO: add warn
                                return;
                        }

                        // TODO: maybe even use stack/cache of CharyArrayWriter here, too
                        CharArrayWriter writer = new CharArrayWriter();
                        delegate.addElement(writer, date, request, response, time);
                        jsonWriter.write(jsonAttributeName, writer.toString());
                }
        }

        @Override
        protected AccessLogElement createAccessLogElement(String name, char pattern) {
                AccessLogElement e = super.createAccessLogElement(name, pattern);
                return new JsonAccessLogElementWrapper(e, mapName(e) + '-' + name);
        }

        @Override
        protected AccessLogElement createAccessLogElement(char pattern) {
                AccessLogElement e = super.createAccessLogElement(pattern);
                return new JsonAccessLogElementWrapper(e, mapName(e));
        }

        private String mapName(AccessLogElement e) {
                if(e instanceof RemoteAddrElement) {
                        return "remoteAddr";
                } else if(e instanceof HostElement) {
                        return "host";
                // TODO: finish
                }
                throw new IllegalArgumentException();
        }
}

so what do you think about this approach?

mfg
thomas

>
> Le jeu. 21 janv. 2021 à 13:59, Thomas Meyer <[hidden email]> a écrit :
>
> > a sub class can extend before and after the addElement loop to
> > establish a context via thread-dependend CharArrayWriter object.
> > ---
> >  java/org/apache/catalina/valves/AbstractAccessLogValve.java | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > index e23f49d3a5..5e774c2320 100644
> > --- a/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > +++ b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > @@ -685,9 +685,11 @@ public abstract class AbstractAccessLogValve extends
> > ValveBase implements Access
> >              result = new CharArrayWriter(128);
> >          }
> >
> > +        preLogAddElement(result);
> >          for (int i = 0; i < logElements.length; i++) {
> >              logElements[i].addElement(result, date, request, response,
> > time);
> >          }
> > +        postLogAddElement(result);
> >
> >          log(result);
> >
> > @@ -699,6 +701,9 @@ public abstract class AbstractAccessLogValve extends
> > ValveBase implements Access
> >
> >      // -------------------------------------------------------- Protected
> > Methods
> >
> > +    protected void preLogAddElement(CharArrayWriter result) {}
> > +    protected void postLogAddElement(CharArrayWriter result) {}
> > +
> >      /**
> >       * Log the specified message.
> >       *
> > --
> > 2.20.1
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AbstractAccessLogValve: Add pre/post add log element extension points

Romain Manni-Bucau
Hmm, makes me think about two comments:

1. Looks like a custom formatter not matching inline syntax of abstract
logging valve so maybe extract the parser/element factory in a class
reusable by multiple implementations.
2. Your pre and post are specific to json but dont enable native json since
null case will need a default and details like that dont make it too json
friendly. Id rather use element extractors to create a json object i would
jsonb.toJson before logging. It also brings another issue which is in some
cases you will want to batch json log lines in arrays for faster sending -
when not using docker json logging driver typically but directly pushing to
solr/elasticsearch for ex.

So overall a new valve creating a JsonRecord and serialized to string -or
just using jsongenerator - is way easier for your use case imo than making
the default valve abstracted which will require a lot of details.

However, being able to list the elements to log and reusing logging valve
element factory impl can ease it to be configurable so this is the part i
would extract and then i would just do a JsonLoggingValve using a list of
elements - not a real string pattern hut a list. Only code to import for
json support looks like the string escaping (can be copied from johnzon
Strings class or log4j2 equivalent class, it is small and avoids any dep).

Hope it makes sense and helps a bit to solve your need.

Side note: such a valve is already embeddable in your app enabling to have
any custom dependency if it helps.


Le jeu. 21 janv. 2021 à 16:03, Thomas Meyer <[hidden email]> a écrit :

> On Thu, Jan 21, 2021 at 03:28:12PM +0100, Romain Manni-Bucau wrote:
> > Hi
>
> Hi,
>
> > Why cant it be added as element earlier at pattern parsing time? Would
> > avoid to have two particular cases and keep current pattern/impl.
> > A %java or so sounds more natural no?
>
> Mhh, maybe I just share my idea of the concrete implementation, so you see
> what I want to do:
>
> Premise: The JUL logging is configured to log all below to an own handler.
>
> public class LoggingAccessLogValve extends AbstractAccessLogValve {
>
>         private static final Log log =
> LogFactory.getLog(LoggingAccessLogValve.class);
>         private Map<CharArrayWriter, JsonGenerator> writers = new
> WeakHashMap<>();
>
>         @Override
>         protected void log(CharArrayWriter message) {
>                 if(log.isInfoEnabled()) {
>                         log.info(message);
>                 }
>         }
>
>         protected void preLogAddElement(CharArrayWriter result) {
>                 JsonGenerator jsonWriter = Json.createGenerator(result);
>                 synchronized (this) {
>                         writers.put(result,jsonWriter);
>                 }
>                 jsonWriter.writeStartObject();
>         }
>
>         protected void postLogAddElement(CharArrayWriter result) {
>                 JsonGenerator jsonWriter = null;
>                 synchronized (this) {
>                         jsonWriter = writers.remove(result);
>                 }
>                 if(jsonWriter != null) {
>                         jsonWriter.writeEnd();
>                         jsonWriter.close();
>                 }
>         }
>
>         private class JsonAccessLogElementWrapper implements
> AccessLogElement {
>
>                 private final String jsonAttributeName;
>                 private final AccessLogElement delegate;
>
>                 public JsonAccessLogElementWrapper(AccessLogElement e,
> String name) {
>                         delegate = e;
>                         jsonAttributeName = name;
>                 }
>
>                 @Override
>                 public void addElement(CharArrayWriter buf, Date date,
> Request request, Response response, long time) {
>                         JsonGenerator jsonWriter = null;
>                         synchronized (this) {
>                                 jsonWriter = writers.get(buf);
>                         }
>                         if(jsonWriter == null) {
>                                 // TODO: add warn
>                                 return;
>                         }
>
>                         // TODO: maybe even use stack/cache of
> CharyArrayWriter here, too
>                         CharArrayWriter writer = new CharArrayWriter();
>                         delegate.addElement(writer, date, request,
> response, time);
>                         jsonWriter.write(jsonAttributeName,
> writer.toString());
>                 }
>         }
>
>         @Override
>         protected AccessLogElement createAccessLogElement(String name,
> char pattern) {
>                 AccessLogElement e = super.createAccessLogElement(name,
> pattern);
>                 return new JsonAccessLogElementWrapper(e, mapName(e) + '-'
> + name);
>         }
>
>         @Override
>         protected AccessLogElement createAccessLogElement(char pattern) {
>                 AccessLogElement e = super.createAccessLogElement(pattern);
>                 return new JsonAccessLogElementWrapper(e, mapName(e));
>         }
>
>         private String mapName(AccessLogElement e) {
>                 if(e instanceof RemoteAddrElement) {
>                         return "remoteAddr";
>                 } else if(e instanceof HostElement) {
>                         return "host";
>                 // TODO: finish
>                 }
>                 throw new IllegalArgumentException();
>         }
> }
>
> so what do you think about this approach?
>
> mfg
> thomas
>
> >
> > Le jeu. 21 janv. 2021 à 13:59, Thomas Meyer <[hidden email]> a écrit :
> >
> > > a sub class can extend before and after the addElement loop to
> > > establish a context via thread-dependend CharArrayWriter object.
> > > ---
> > >  java/org/apache/catalina/valves/AbstractAccessLogValve.java | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git
> a/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > > b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > > index e23f49d3a5..5e774c2320 100644
> > > --- a/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > > +++ b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
> > > @@ -685,9 +685,11 @@ public abstract class AbstractAccessLogValve
> extends
> > > ValveBase implements Access
> > >              result = new CharArrayWriter(128);
> > >          }
> > >
> > > +        preLogAddElement(result);
> > >          for (int i = 0; i < logElements.length; i++) {
> > >              logElements[i].addElement(result, date, request, response,
> > > time);
> > >          }
> > > +        postLogAddElement(result);
> > >
> > >          log(result);
> > >
> > > @@ -699,6 +701,9 @@ public abstract class AbstractAccessLogValve
> extends
> > > ValveBase implements Access
> > >
> > >      // --------------------------------------------------------
> Protected
> > > Methods
> > >
> > > +    protected void preLogAddElement(CharArrayWriter result) {}
> > > +    protected void postLogAddElement(CharArrayWriter result) {}
> > > +
> > >      /**
> > >       * Log the specified message.
> > >       *
> > > --
> > > 2.20.1
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail: [hidden email]
> > >
> > >
>