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