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
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> > >
> >
> >
