Thinking more on this issue made me question the ideal way to pass
Java string literals to plugins.
I first gave Matt's suggestion a try, set eventDelimiter="�" in the
XML and got the following error:
ERROR StatusLogger Error parsing /home/vy/...Logging.xml
org.xml.sax.SAXParseException; systemId:
file:///home/vy/...Logging.xml; lineNumber: 29; columnNumber: 55;
Character reference "&#
at
com.sun.org.apache.xerces.internal.parsers.DOMParser.parse(DOMParser.java:257)
Next I tried passing escapes from a properties files instead. Setting
eventDelimiter=\r\n\0 results in string "\r\n0".
Hence, I am pretty confident that the user input needs to be escaped
and I need to unescape it upon reception. There is an ad-hoc
`unescapeJava()` method shared in a StackOverflow post[1]. The thread
also references StringEscapeUtils.unescapeJava() (from commons) and
String#translateEscapes() (Java 13+) alternatives along with their
caveats.
At this stage, I am inclined to pick the solution shared in
StackOverflow[1]. Note that accepting escape characters will void the
need to nullEventDelimiterEnabled flag. To sum it up,
1. May I have your blessing for using the StackOverflow snippet?
2. Is it okay with regards to licensing?
[1] https://stackoverflow.com/a/4298836/1278899
On Sun, Jul 12, 2020 at 7:35 PM Ralph Goers <ralph.go...@dslextreme.com> wrote:
>
> Maybe. But it isn’t obvious and would have to be explicitly documented.
>
> Ralph
>
> > On Jul 12, 2020, at 8:09 AM, Matt Sicker <boa...@gmail.com> wrote:
> >
> > Wouldn’t that be � in XML?
> >
> > On Sat, Jul 11, 2020 at 16:26 Ralph Goers <ralph.go...@dslextreme.com>
> > wrote:
> >
> >> I guess what you mean is that where you have $resolver: “message” that
> >> would somehow be replaced by $resolver” “pattern”? That would make sense.
> >> I have no problem with the changes you are suggesting. However, you should
> >> also change the configuration shown in cloud.md and the log4j2.xml in
> >> log4j-spring-cloud-config/log4j-spring-cloud-samples/log4j-spring-cloud-config-sample-server/src/main/config-repo
> >> to match.
> >>
> >> Thanks for determining that it was XML parsing that was messing with the
> >> “\0” value. That implies there would have been an XMLish way to bypass that
> >> but using an explicit attribute is just a lot easier. The GelfLayout uses
> >> “includeNullDelimitor”. You might consider using that name for consistency.
> >>
> >> Ralph
> >>
> >>> On Jul 11, 2020, at 1:46 PM, Volkan Yazıcı <volkan.yaz...@gmail.com>
> >> wrote:
> >>>
> >>> I can't believe how were we expecting somebody to pass a null
> >>> character from an XML file! Long story short, it was the XML parser,
> >>> doing the right thing, that is, converting XML attribute value "\0" to
> >>> Java String "\\0". Okay, I am not gonna whine about this anymore. I
> >>> have just pushed a commit adding `nullEventDelimiterEnabled` flag
> >>> along with a unit test. (Ralph, I've replaced your
> >>> `eventDelimiter="null"` handling as well.)
> >>>
> >>> Note that my question about reverting the changes to `MessageResolver`
> >>> still stands.
> >>>
> >>> On Sat, Jul 11, 2020 at 8:37 PM Volkan Yazıcı <volkan.yaz...@gmail.com>
> >> wrote:
> >>>>
> >>>> Hello Ralph!
> >>>>
> >>>> Sorry that it took me this long to reply to you back in detail, but I
> >>>> have just found time to do so. Let me try to address the points you
> >>>> raised:
> >>>>
> >>>>> The first problem I ran into is that additional fields don’t work.
> >>>>> I don’t see any configurations that have them and the Builder doesn’t
> >>>>> have annotations on the key and value fields so I suspect it just
> >>>>> doesn’t work.
> >>>>
> >>>> Yes, the annotations... :facepalm: Good catch.
> >>>>
> >>>>> Why didn’t you just enhance KeyValuePair to add the type attribute?
> >>>>
> >>>> I was a little bit hesitant to do that, since `type` is mostly a
> >>>> concern in the context of JTL. Would you rather recommend me to extend
> >>>> `KeyValuePair` instead?
> >>>>
> >>>>> I thought you said you added the ability to format the message using
> >>>>> a pattern, but I don’t see that in the documentation or in
> >>>>> MessageResolver. Was that not included?
> >>>>
> >>>> Dang it! Indeed I forgot to add `PatternResolver` into the docs. (I
> >>>> will talk about this again in the following lines.)
> >>>>
> >>>>> I have added the ability to format the message field using a pattern.
> >>>>
> >>>> Doh! I totally misinterpreted your request. I thought you wanted to be
> >>>> able to *only* employ `PatternLayout` in JTL, hence I created
> >>>> `PatternResolver`. Though what you asked for is to treat the message
> >>>> as a pattern. (Looking back right now, your request was pretty clear,
> >>>> it was me who was confused.)
> >>>>
> >>>> I have reviewed the changes and I want to share two remarks:
> >>>>
> >>>> 1. I've seen that you have used a thread-local. That functionality is
> >>>> totally delegated to the `RecyclerFactory` employed, which can be
> >>>> provided by the user. (I will change this accordingly.)
> >>>>
> >>>> 2. I've noticed `PatternResolver` is missing the `includeStacktrace`
> >>>> flag. I will add it.
> >>>>
> >>>>> I have modified the code so that specifying `eventDelimiter=“null”`
> >>>>> will insert a null at the end of the message as specifying
> >>>>> eventDelimiter=“\0” or “\u000” does not work (It ends up being “\\0”
> >>>>> or “\u000”.
> >>>>
> >>>> I am really puzzled about this. Let me explain it through the sources.
> >> These
> >>>> are the lines you have added:
> >>>>
> >>>> if (eventDelimiter != null &&
> >> eventDelimiter.equalsIgnoreCase("null")) {
> >>>> stringBuilder.append('\0');
> >>>> } else {
> >>>> stringBuilder.append(eventDelimiter);
> >>>> }
> >>>>
> >>>> Nowhere in the code `eventDelimiter` is JSON quoted. I am totally
> >>>> clueless how setting `eventDelimiter` to `\0` doesn't create the
> >>>> effect you manually execute here. Further, if it doesn't, how come
> >>>> `JsonTemplateLayoutTest#test_null_eventDelimiter()` test passes? I see
> >>>> nothing else but to blame two other suspects:
> >>>>
> >>>> 1. Either setting the `eventDelimiter` property to `\0` causes the
> >>>> value to get quoted.
> >>>>
> >>>> 2. Or there is something else going on while passing the value from
> >>>> the layout to the appender.
> >>>>
> >>>> On the other hand, I am a little bit distant to creating special cases
> >>>> for certain values (e.g., `null`) of `eventDelimiter`. What if the
> >>>> user literally wants to dump `null` (literally!) after every event?
> >>>>
> >>>>> As I was implementing stuff I noticed you implemented stuff using
> >>>>> lots of static methods. I am not really sure why since almost all of
> >>>>> those methods are only invoked from an instance of the object.
> >>>>
> >>>> I generally choose the strictest modifier combination, unless there is
> >>>> a reason not to do so. Following this principle, if a method/field
> >>>> doesn't have any instance dependencies, I make it static. Am I doing
> >>>> something wrong with this convention?
> >>>>
> >>>>> Also, I would like to be able to specify the pattern I use for the
> >>>>> message in the log4j configuration so it is easier to change
> >>>>> dynamically but I couldn’t figure out how to do that and get the
> >>>>> pattern passed to the MessageResolver.
> >>>>
> >>>> Doesn't `PatternResolver` fit the bill here?
> >>>>
> >>>> Cheers!
> >>>>
> >>>> On Wed, Jul 8, 2020 at 7:42 PM Ralph Goers <ralph.go...@dslextreme.com>
> >> wrote:
> >>>>>
> >>>>> A couple other things. As I was implementing stuff I noticed you
> >> implemented stuff using lots of static methods. I am not really sure why
> >> since almost all of those methods are only invoked from an instance of the
> >> object. I had to change that to get my enhancements to work.
> >>>>>
> >>>>> Also, I would like to be able to specify the pattern I use for the
> >> message in the log4j configuration so it is easier to change dynamically
> >> but I couldn’t figure out how to do that and get the pattern passed to the
> >> MessageResolver.
> >>>>>
> >>>>> Ralph
> >>>>>
> >>>>>> On Jul 8, 2020, at 7:16 AM, Volkan Yazıcı <volkan.yaz...@gmail.com>
> >> wrote:
> >>>>>>
> >>>>>> Hey Ralph,
> >>>>>>
> >>>>>> Thanks so much for taking time, really appreciated. Unfortunately the
> >>>>>> last few days I was pretty busy both at home and at work. I will check
> >>>>>> out your changes and reply back as soon as I find some time.
> >>>>>>
> >>>>>> Kind regards.
> >>>>>>
> >>>>>> On Wed, Jul 8, 2020 at 1:08 AM Ralph Goers <
> >> ralph.go...@dslextreme.com> wrote:
> >>>>>>>
> >>>>>>> Volkan,
> >>>>>>>
> >>>>>>> I have updated the relevant documentation. I am done with all the
> >> changes I wanted to make.
> >>>>>>>
> >>>>>>> Ralph
> >>>>>>>
> >>>>>>>> On Jul 6, 2020, at 2:50 PM, Ralph Goers <ralph.go...@dslextreme.com>
> >> wrote:
> >>>>>>>>
> >>>>>>>> I have fixed the issue with the annotations. I have added the
> >> ability to format the message field using a pattern and I have modified the
> >> code so that specifying eventDelimiter=“null” will insert a null at the end
> >> of the message as specifying eventDelimiter=“\0” or “\u000” does not work
> >> (It ends up being “\\0” or “\u000”.
> >>>>>>>>
> >>>>>>>> I have verified that with these changes the message is formatted
> >> properly with Gelf. I am still testing though to see how the ThreadContext
> >> is being handled.
> >>>>>>>>
> >>>>>>>> Ralph
> >>>>>>>>
> >>>>>>>>> On Jul 5, 2020, at 11:01 PM, Ralph Goers <
> >> ralph.go...@dslextreme.com> wrote:
> >>>>>>>>>
> >>>>>>>>> I finally got some time to start testing JsonTemplateLayout
> >> against the logging in the cloud documentation. The first problem I ran
> >> into is that additional fields don’t work. I don’t see any configurations
> >> that have them and the Builder doesn’t have annotations on the key and
> >> value fields so I suspect it just doesn’t work. Why didn’t you just
> >> enhance KeyValuePair to add the type attribute?
> >>>>>>>>>
> >>>>>>>>> After I corrected the EventTemplateAdditionalField plugin I still
> >> can’t get stack traces the way I want them. I thought you said you added
> >> the ability to format the message using a pattern, but I don’t see that in
> >> the documentation or in MessageResolver. Was that not included? As I said,
> >> I require the stack trace to print in the message field in Kibana, not just
> >> be a key in the additional data.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Ralph
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>
> >>
> >> --
> > Matt Sicker <boa...@gmail.com>
>
>
