We can’t “backport” the code from Java 13 as that is licensed under the GPL.
What I was suggesting (without saying it) was that maybe we don’t need the eventDelimiter parameter. Yes, this should be solved across the board so everything is consistent. Ralph > On Jul 15, 2020, at 12:53 AM, Volkan Yazıcı <volkan.yaz...@gmail.com> wrote: > > First let me state that this "how to pass escape characters to plugin > parameters of type string from different configuration sources" > problem applies to every other plugin, e.g., > `AbstractJacksonLayout.Builder#endOfLine`. Hence, nothing specific to > `JsonTemplateLayout`. > > That said, I share your hesitation to include a relatively complex > `unescape()` routine into the code base. Though having both > `eventDelimiter` and `nullEventDelimiterEnabled` parameters while the > same effect can exactly be created with just `eventDelimiter` feels > like a code smell to me. What about just backporting > `translateEscapes()` of Java 13? This will still fit the bill and can > be removed in the future. > > On Tue, Jul 14, 2020 at 7:41 PM Ralph Goers <ralph.go...@dslextreme.com> > wrote: >> >> Do we really need to do this? The only two delimiters that have ever been >> requested are null and newline. Defaulting to newline and having an option >> to use null seems fine to me. >> >> Ralph >> >>> On Jul 14, 2020, at 5:49 AM, Volkan Yazıcı <volkan.yaz...@gmail.com> wrote: >>> >>> 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> >>>> >>>> >>> >> >> >
