congrats! On Tue, May 26, 2020 at 5:45 AM Volkan Yazıcı <volkan.yaz...@gmail.com> wrote:
> BAM! JsonTemplateLayout is merged to master! 🎉🍾 (I needed to wait > some for the GitHub write access to get sync'ed with Apache LDAP.) > > On Mon, May 25, 2020 at 9:46 PM Volkan Yazıcı <volkan.yaz...@gmail.com> > wrote: > > > > I had missed the GitBox Account Linking Matt has mentioned. I have > > just completed it and get acknowledged that I "will" have right access > > to the logging-* repositories. I don't know if it is a timing issue, > > but I still cannot see any "merge" buttons available. I will keep on > > refreshing the page. If there is anything else I need to do, please do > > let me know. > > > > On Mon, May 25, 2020 at 6:40 PM Ralph Goers <ralph.go...@dslextreme.com> > wrote: > > > > > > I see the squash and merge button at the bottom. I just approved the > PR so maybe you can see it now? If not, then the steps Matt has provided > may be needed. We will need to update the new committer email to reflect > this if that is required. > > > > > > If you are still having problems and need me to do it I can merge it, > but it would be great to verify that you can do it. > > > > > > Ralph > > > > > > > On May 25, 2020, at 8:49 AM, Matt Sicker <boa...@gmail.com> wrote: > > > > > > > > Make sure your account is linked properly: > > > > https://gitbox.apache.org/setup/ > > > > > > > > On Mon, May 25, 2020 at 07:23 Volkan Yazıcı <volkan.yaz...@gmail.com> > wrote: > > > > > > > >> Ralph, I can indeed see I am a member of the Apache organization. > > > >> Though I don't see a "merge" option appearing in the PR page. Could > it > > > >> be because of the "change request" of yours? If so, would you mind > > > >> resolving that request, please? (Sorry, I don't see any other visual > > > >> cue to proceed further. Maybe it is just me...) > > > >> > > > >> On Mon, May 25, 2020 at 12:02 AM Ralph Goers < > ralph.go...@dslextreme.com> > > > >> wrote: > > > >>> > > > >>> You should already have it. > > > >>> > > > >>> Ralph > > > >>> > > > >>>> On May 24, 2020, at 2:59 PM, Volkan Yazıcı < > volkan.yaz...@gmail.com> > > > >> wrote: > > > >>>> > > > >>>> Somebody mind giving me write access to the GitHub repo, please? > I am > > > >>>> not able to merge the PR. > > > >>>> > > > >>>> On Fri, May 22, 2020 at 3:53 PM Apache < > ralph.go...@dslextreme.com> > > > >> wrote: > > > >>>>> > > > >>>>> Feel free to merge it. I will test it there when I can. > > > >>>>> > > > >>>>> Ralph > > > >>>>> > > > >>>>> > > > >>>>>> On May 22, 2020, at 4:50 AM, Volkan Yazıcı < > volkan.yaz...@gmail.com> > > > >> wrote: > > > >>>>>> > > > >>>>>> Hey Ralph, > > > >>>>>> > > > >>>>>> Here is my status update: > > > >>>>>> > > > >>>>>> -~- Benchmarks -~- > > > >>>>>> > > > >>>>>> I have removed the benchmarks results. It takes ~8h for a > complete > > > >> run > > > >>>>>> and I don't want to repeat that cycle after every change. I > might > > > >>>>>> revisit this idea once JTL becomes more stable in terms of its > > > >>>>>> features. > > > >>>>>> > > > >>>>>> -~- Flattening of MDC fields -~- > > > >>>>>> > > > >>>>>> I have changed the MDC directive as follows: > > > >>>>>> > > > >>>>>> mdc > > > >>>>>> mdc:flatten[=<prefix>][,stringify] > > > >>>>>> mdc:pattern=<pattern>[,flatten=<prefix>][,stringify] > > > >>>>>> mdc:key=<key>[,stringify] > > > >>>>>> > > > >>>>>> This also allowed JsonTemplateLayout to produce the *exact* > output as > > > >>>>>> GelfLayout and EcsLayout, where MDC keys are flattened and > values are > > > >>>>>> stringified. > > > >>>>>> > > > >>>>>> -~- Null termination, newlines, Logstash, and etc. -~- > > > >>>>>> > > > >>>>>> I have tried to reproduce the experiment you have shared, though > > > >> could > > > >>>>>> not really succeed due to some Docker command failure in > > > >> restartApp.sh. > > > >>>>>> Further, I find the associated Spring setup quite cumbersome to > wrap > > > >> my > > > >>>>>> mind around it. That said, I have done something else: I have > > > >> improved > > > >>>>>> LogstashIT such that > > > >>>>>> > > > >>>>>> 1. Logstash is configured with both "tcp" and "gelf" input > plugins, > > > >>>>>> > > > >>>>>> 2. LogstashIT employs JsonTemplateLayout against both inputs, > repeats > > > >>>>>> the same using GelfLayout and EcsLayout, and verifies the > populated > > > >>>>>> content in Elasticsearch. > > > >>>>>> > > > >>>>>> One can easily validate this on the branch as follows: > > > >>>>>> > > > >>>>>> $ ./mvnw clean package -DskipTests > > > >>>>>> $ ./mvnw \ > > > >>>>>> verify -o -P docker \ > > > >>>>>> -pl > > > >> > log4j-layout-jackson-json,log4j-plugins,log4j-core,log4j-api,log4j-layout-json-template > > > >>>>>> \ > > > >>>>>> -Dtest="Dummy.java" -DtrimStackTrace=false > > > >> -DfailIfNoTests=false > > > >>>>>> > > > >>>>>> One can repeat the very same by running LogstashIT in IDE after > > > >>>>>> starting the containers: > > > >>>>>> > > > >>>>>> $ ./mvnw \ > > > >>>>>> docker:start -o -P docker \ > > > >>>>>> -pl > > > >> > log4j-layout-jackson-json,log4j-plugins,log4j-core,log4j-api,log4j-layout-json-template > > > >>>>>> > > > >>>>>> I don't know what is wrong with the Spring Cloud Config setup > you > > > >> have > > > >>>>>> shared, but I have no reasons to believe that > JsonTemplateLayout is > > > >> the > > > >>>>>> suspect. > > > >>>>>> > > > >>>>>> Regarding your remark about everything getting escaped... This > might > > > >>>>>> happen when Logstash fails to read an input. In such a case, it > puts > > > >>>>>> the entire payload into the "message" field (hence, the > escaping) and > > > >>>>>> stores it like that. > > > >>>>>> > > > >>>>>> From now on, I don't know how to proceed with this problem, in > > > >>>>>> particular, given I believe the problem is in somewhere else but > > > >>>>>> JsonTemplateLayout. > > > >>>>>> > > > >>>>>> -~- Merging branch to master -~- > > > >>>>>> > > > >>>>>> Unless there are objections, I want to merge the branch to > master. > > > >>>>>> There on I will share json-template-layout.md with the > community to > > > >>>>>> collect some feedback on the API. In particular, I have existing > > > >> users > > > >>>>>> of LogstashLayout in mind. > > > >>>>>> > > > >>>>>> Kind regards. > > > >>>>>> > > > >>>>>>> On Mon, May 18, 2020 at 1:18 AM Ralph Goers < > > > >> ralph.go...@dslextreme.com> wrote: > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>>>> On May 17, 2020, at 2:37 PM, Volkan Yazıcı < > > > >> volkan.yaz...@gmail.com> wrote: > > > >>>>>>>> > > > >>>>>>>> Thanks so much for the thorough review Ralph, really > appreciated! I > > > >>>>>>>> will address issues you have raised. > > > >>>>>>>> > > > >>>>>>>> [As a side note, I have pushed changes containing performance > > > >>>>>>>> improvements and benchmark results. The module is still > dependency > > > >>>>>>>> free and performance-wise pretty good.] > > > >>>>>>>> > > > >>>>>>>>> all the default templates separate the message from the > exception > > > >>>>>>>> > > > >>>>>>>> Yes, this I have also realized while studying the source code > of > > > >>>>>>>> GelfLayout. Some random thoughts: > > > >>>>>>>> > > > >>>>>>>> 1. How about introducing a firstNonNull-like operator: > > > >>>>>>>> > ${json:op:firstNonNull:${json:message},${json:exception:message}}. > > > >>>>>>>> Parsing this would not be trivial, given the list of variables > > > >>>>>>>> can include comma in various forms. Though the rest should > > > >>>>>>>> be tractable. (Feels like JsonTemplateLayout directives will > at > > > >>>>>>>> some point catch up the with ones in PatternLayout.) > > > >>>>>>> > > > >>>>>>> The problem here is that I want newlines in the message. I’m > not > > > >> sure what firstNonNull means in the context of a message. The > message > > > >> itself won’t have nulls. The Gelf format dictates the null is at > the end of > > > >> the JSON string. > > > >>>>>>> > > > >>>>>>>> > > > >>>>>>>> 2. How about introducing a fallback parameter to the "message" > > > >>>>>>>> directive: ${json:message:fallback=exception:message}. > > > >>>>>>> > > > >>>>>>> Again, I don’t know what that means or how it solves my > problem of > > > >> wanting newlines in the message. > > > >>>>>>> > > > >>>>>>>> > > > >>>>>>>> 3. May I challenge the request: Do one really need it? You > store > > > >>>>>>>> individual values, i.e, message and exception, in individual > > > >> fields. > > > >>>>>>>> When one is missing and the other is present, isn't it just a > > > >>>>>>>> presentation layer issue to display it properly? > > > >>>>>>> > > > >>>>>>> Picture how a log event looks in a file with a “standard” > > > >> PatternLayout. In Kibana by default you get 3 columns, the > timestamp, a > > > >> field I don’t care about and the message. I want the message to > appear just > > > >> as it would in a log file without the timestamp. Yes, I can click > on each > > > >> message and see its attributes, but that takes longer. And I want > > > >> exceptions to jump out at me when I am visually scrolling through > messages. > > > >> In fact, I typically don’t want the stack trace in a variable of > its own. > > > >> Kibana doesn’t take the pieces of the message and put them together > like > > > >> the pattern layout does. Instead it lets you create columns for > individual > > > >> attributes. The problem with that is the columns waste a lot of > space, > > > >> especially if they frequently are empty as a stack trace would be. > > > >>>>>>> > > > >>>>>>> So yes, I really need it. > > > >>>>>>> > > > >>>>>>>> > > > >>>>>>>>> In the logged event all newlines seem to be stripped from the > > > >> exception. > > > >>>>>>>> > > > >>>>>>>> I really don't get this Ralph. Have you seen the tests in > > > >> LogstashIT? > > > >>>>>>>> I have just added a new one testing only newlines. Would you > mind > > > >>>>>>>> fiddling around with LogstashIT to reproduce the issue, > please? Or > > > >>>>>>>> sharing a recipe for me to reproduce it? > > > >>>>>>> > > > >>>>>>> The way I do it is to install Logstash, ElasticSearch and > Kibana > > > >> locally on my Mac as well as Docker Desktop for Mac. I then run > SpringCloud > > > >> config server from the Log4j Spring Cloud Config project and tailor > the > > > >> log4j2.xml that is in the config-repo directory of that project. > Note that > > > >> all Log4j Lookup variables have to have the $ escaped or Spring > Cloud > > > >> Config will try to resolve them. It also requires having the Socket > > > >> appender connect to a host of “host.docker.internal” to be able to > connect > > > >> to Logstash outside of the docker container. I configure Logstash > using the > > > >> configuration documented at > > > >> http://logging.apache.org/log4j/2.x/manual/cloud.html < > > > >> http://logging.apache.org/log4j/2.x/manual/cloud.html>, at least > when > > > >> using Gelf. I configure Kibana with an index of app-*. > > > >>>>>>> > > > >>>>>>> I then modifythe Spring Cloud Config Sample app as needed to > work > > > >> with this setup and start it using docker/restartApp.sh. > > > >>>>>>> > > > >>>>>>> After starting I do a few things: > > > >>>>>>> 1. Enter “docker logs app-container”. This should show it is > > > >> running and the last message will be the Spring logo. > > > >>>>>>> 2. Look in Kibana and click on the logs. You should see logs > from > > > >> the server. > > > >>>>>>> 3. In a browser enter http://localhost:8080/sample/exception < > > > >> http://localhost:8080/sample/exception> - unless you have changed > the > > > >> port. For some reason I had something running on 8080 and had to > change the > > > >> sample app to use 8090. This will generate a stack trace on the web > page > > > >> and in the log. > > > >>>>>>> 4. Look in Kibana. You should see the exception and the stack > trace > > > >> properly formatted in the message column. It should also display > the thread > > > >> id, log-level and other items configured in the pattern. > > > >>>>>>> > > > >>>>>>>> > > > >>>>>>>>> Logstash does not seem to be getting a null character > > > >>>>>>>>> (I tried with both “\0” and “\u0000”) > > > >>>>>>>> > > > >>>>>>>> This is weird. Below is how GelfLayout does it: > > > >>>>>>>> > > > >>>>>>>> if (includeNullDelimiter) { > > > >>>>>>>> builder.append('\0'); > > > >>>>>>>> } > > > >>>>>>>> > > > >>>>>>>> And here is how JsonTemplateLayout does it: > > > >>>>>>>> > > > >>>>>>>> stringBuilder.append(eventDelimiter); > > > >>>>>>>> > > > >>>>>>>> Further, there is even a test for this: > > > >>>>>>>> JsonTemplateLayout#test_null_eventDelimiter(): > > > >>>>>>>> > > > >>>>>>>> final String serializedLogEvent = > layout.toSerializable(logEvent); > > > >>>>>>>> assertThat(serializedLogEvent).isEqualTo(eventTemplate + > '\0'); > > > >>>>>>>> > > > >>>>>>>> Any ideas on what might I be missing? > > > >>>>>>> > > > >>>>>>> No - I just know that Logstash wasn’t detecting the null. It > looked > > > >> like the whole string was being escaped as all the double quotes > were also > > > >> escaped as you can see from the example. I should note that I > directly > > > >> copied that from the Logstash log that was written to the terminal > window > > > >> with debug logging enabled, so it is possible it was caused by > that, but > > > >> this is also what I see in Kibana. > > > >>>>>>> > > > >>>>>>>> > > > >>>>>>>>> This isn’t valid in the Gelf spec as additional attributes > in Gelf > > > >>>>>>>>> must match the regex - ^[\w\.\-]*$. This means you need to > > > >>>>>>>>> parse the MDC and add each key as an additional to be able > > > >>>>>>>>> to create valid Gelf. Personally, I’d like that option > anyway. > > > >>>>>>>> > > > >>>>>>>> For this purpose, I need to introduce an operator similar to > > > >>>>>>>> unquote-splicing in Lisp to merge the contents of the child > with > > > >> the > > > >>>>>>>> parent. That said, I have my doubts about the practical > benefit of > > > >>>>>>>> such a feature. To the best of my knowledge, almost every > (sanely > > > >>>>>>>> configured?) log pipeline ends up with a structured storage > system > > > >>>>>>>> containing whitelisted fields. Consider ELK stack. Would you > allow > > > >>>>>>>> developers to introduce fields at their will? If so, it is a > > > >> matter of > > > >>>>>>>> time somebody will take down the entire Elasticsearch cluster > by > > > >>>>>>>> flooding it with ContextData.put(userInput, "doh"). Hence, you > > > >> employ > > > >>>>>>>> the identical whitelisting strategy at the layout as well: > > > >>>>>>>> > > > >>>>>>>> { > > > >>>>>>>> "version": "1.1", > > > >>>>>>>> "host": "${hostName}", > > > >>>>>>>> ..., > > > >>>>>>>> "_mdc.loginId": "${json:mdc:loginId}", > > > >>>>>>>> "_mdc.requestId": "${json:mdc:requestId}" > > > >>>>>>>> } > > > >>>>>>>> > > > >>>>>>>> What do you think? Shall we really introduce unquote-splicing? > > > >>>>>>> > > > >>>>>>> I would have preferred a syntax like > > > >>>>>>> > > > >>>>>>> { > > > >>>>>>> “version” “1.1”, > > > >>>>>>> … > > > >>>>>>> ${json.map:mdc:prefix=“_”} > > > >>>>>>> } > > > >>>>>>> > > > >>>>>>> This would cause the whole map to be emitted. Although it > would be > > > >> nice to have a version of that which also could specify the keys > there > > > >> probably isn’t a point to that since you can directly specify them > as you > > > >> have shown. > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> As I said previously, now that I understand what is going on I > am > > > >> comfortable with you merging the PR. It is going to the master > branch so it > > > >> won’t be what users see on the web site right now. By the time it > makes it > > > >> there we can get this all straightened out. If nothing else we > could have > > > >> the current GelfLayout and the JsonTemplateLayout both on the page > and > > > >> explain the differences between them, although in the end I agree > it would > > > >> be better to have the JsonTemplateLayout replace both the > JsonLayout and > > > >> GelfLayout. > > > >>>>>>> > > > >>>>>>> Ralph > > > >>>>>>> > > > >>>>>>>> > > > >>>>>>>> Kind regards. > > > >>>>>>>> > > > >>>>>>>> On Sat, May 16, 2020 at 1:28 AM Ralph Goers < > > > >> ralph.go...@dslextreme.com> wrote: > > > >>>>>>>>> > > > >>>>>>>>> I finally got time to spend looking at the Layout. I now > > > >> understand why it works for you and sort of understand why it > doesn’t work > > > >> for me. > > > >>>>>>>>> > > > >>>>>>>>> First, all the default templates separate the message from > the > > > >> exception. In the logged event all newlines seem to be stripped > from the > > > >> exception. This makes viewing them in Kibana pretty awful, but the > do get > > > >> logged successfully. > > > >>>>>>>>> > > > >>>>>>>>> To include newlines in a log event you either have to play > games > > > >> in Logstash (or Fluentd) and try to put the event back together > again after > > > >> it has been separated into multiple lines or you have to use a > different > > > >> delimiter. Gelf specifies that a null character be used. > > > >>>>>>>>> > > > >>>>>>>>> I then tried to use GelfLayout.json with > > > >>>>>>>>> > > > >>>>>>>>> <Socket name="Logstash" > > > >>>>>>>>> host="\${sys:logstash.host:-host.docker.internal}" > > > >>>>>>>>> port="12222" > > > >>>>>>>>> protocol="tcp" > > > >>>>>>>>> bufferedIo="true"> > > > >>>>>>>>> <JsonTemplateLayout > eventTemplateUri="classpath:GelfLayout.json" > > > >> eventDelimiter="\0" > > > >>>>>>>>> locationInfoEnabled="true"> > > > >>>>>>>>> <EventTemplateAdditionalFields> > > > >>>>>>>>> <KeyValuePair key="hostName" value="${hostName}"/> > > > >>>>>>>>> <KeyValuePair key="_containerId" > > > >> value="\${docker:containerId:-}"/> > > > >>>>>>>>> <KeyValuePair key="_application" > > > >> value="$\${lower:\${spring:spring.application.name:-spring}}"/> > > > >>>>>>>>> </EventTemplateAdditionalFields> > > > >>>>>>>>> </JsonTemplateLayout> > > > >>>>>>>>> </Socket> > > > >>>>>>>>> but this did not work at all. Logstash does not seem to be > > > >> getting a null character (I tried with both “\0” and “\u0000”) and > all the > > > >> events buffer in Logstash until I shutdown the application. Worse > the > > > >> formatting looks like escaped JSON rather than raw json. > > > >>>>>>>>> > > > >>>>>>>>> Also, the GelfTemplate contains > > > >>>>>>>>> { > > > >>>>>>>>> "version": "1.1", > > > >>>>>>>>> "host": "${hostName}", > > > >>>>>>>>> "short_message": "${json:message}", > > > >>>>>>>>> "full_message": "${json:exception:stackTrace:text}", > > > >>>>>>>>> "timestamp": "${json:timestamp:epoch:secs}", > > > >>>>>>>>> "level": "${json:level:severity:code}", > > > >>>>>>>>> "_logger": "${json:logger:name}", > > > >>>>>>>>> "_thread": "${json:thread:name}", > > > >>>>>>>>> "_mdc": "${json:mdc}" > > > >>>>>>>>> } > > > >>>>>>>>> This causes the output to contain > > > >>>>>>>>> > > > >>>>>>>>> > > > >> > \\\"_mdc\\\":{\\\"loginId\\\":\\\"rgoers\\\",\\\"requestId\\\":\\\"266196c1-9702-11ea-9e4a-0242ac120006\\\"} > > > >>>>>>>>> > > > >>>>>>>>> This isn’t valid in the Gelf spec as additional attributes in > > > >> Gelf must match the regex - ^[\w\.\-]*$. This means you need to > parse the > > > >> MDC and add each key as an additional to be able to create valid > Gelf. > > > >> Personally, I’d like that option anyway. > > > >>>>>>>>> > > > >>>>>>>>> I would suggest you attempt to work to make a template that > > > >> matches the result of the GelfLayout I had previously documented. > > > >>>>>>>>> > > > >>>>>>>>> But now that I understand what is going on I am OK with this > > > >> contribution being merged. I would like to get these issues > addressed and > > > >> update the Logging in the Cloud documentation so that it can > document both > > > >> ways of sending to Logstash but we can do that after the code is > merged. > > > >>>>>>>>> > > > >>>>>>>>> Ralph > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>>> On Apr 21, 2020, at 2:23 PM, Ralph Goers < > > > >> ralph.go...@dslextreme.com> wrote: > > > >>>>>>>>>> > > > >>>>>>>>>> Ok. Now that a release has been cut spending time with your > > > >> contribution is at the top of my list. > > > >>>>>>>>>> > > > >>>>>>>>>> Ralph > > > >>>>>>>>>> > > > >>>>>>>>>>> On Apr 21, 2020, at 1:24 PM, Volkan Yazıcı < > > > >> volkan.yaz...@gmail.com> wrote: > > > >>>>>>>>>>> > > > >>>>>>>>>>> Ralph, I've just pushed a commit that adds PatternLayout > > > >> support, FYI. > > > >>>>>>>>>>> > > > >>>>>>>>>>> On Mon, Apr 20, 2020 at 2:08 AM Ralph Goers < > > > >> ralph.go...@dslextreme.com> wrote: > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> I completely understand how messed up people’s lives are > right > > > >> now. I’m lucky, all my kids are long gone out of the house. With > COVID-19 I > > > >> am able to work from home and while that saves me over 2 hours a > day in > > > >> commute time and more time because there isn’t much else to do, I > end up > > > >> spending a lot of the extra time on my $dayjob (and feel lucky I > still have > > > >> it). > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 1. You should care about the checkstyle errors as much as > > > >> everyone else ;-). They don’t seem to be a high priority for people > to fix. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 2. Maybe I didn’t make my need for the Pattern Layout > clear. > > > >> Adding the pattern layout for the JTL is ONLY for the message > attribute > > > >> (just as I did for the GelfLayout). This allows what is displayed > in Kibana > > > >> to look like what you would see in the log file while still allowing > > > >> filtering on all the attributes. For me, this is a big deal - I > won’t use > > > >> the Layout without it. That doesn’t mean it has to be included in > the > > > >> initial commit. I don’t believe JsonLayout supports it either but > I am > > > >> hoping to be able to do that today, although I have several other > things I > > > >> need to do. What you would end up with is a new attribute called > > > >> messagePattern that takes a pattern that is exactly what you would > give to > > > >> PatternLayout. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 3. You can handle the performance issue however you want > but > > > >> if it was me I would just revert back to using Jackson without a > second > > > >> thought. I just don’t see the point of trying to roll your own. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 4. It will take me a while to see what your integration > test > > > >> does and if it resolves my concerns regarding updating the logging > in the > > > >> cloud page. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> Ralph > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>>> On Apr 19, 2020, at 2:30 PM, Volkan Yazıcı < > > > >> volkan.yaz...@gmail.com> wrote: > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> Hello Ralph, > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> [Due to #COVID-19, day cares are closed. I can hardly > spare > > > >> time to > > > >>>>>>>>>>>>> work, including Log4j. Hence, apologies for my slowed > down > > > >> progress & > > > >>>>>>>>>>>>> responses.] > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> I could not put my finger on the performance regression, > yet. > > > >> I might > > > >>>>>>>>>>>>> opt to switch back to Jackson, but I am not gonna do that > > > >> without > > > >>>>>>>>>>>>> spotting that darn pain in the back. (You might have > noticed, > > > >> there is > > > >>>>>>>>>>>>> actually a class for generating a report out of the > available > > > >>>>>>>>>>>>> JSON-emitting layouts. I will skip incorporating its > output > > > >> into the > > > >>>>>>>>>>>>> manually until I address this performance regression.) > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> I've just pushed a commit, i.e., LogstashIT, which tests > > > >>>>>>>>>>>>> JsonTemplateLayout against ELK stack. Would you mind > skimming > > > >> through > > > >>>>>>>>>>>>> that (small) change, please? > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> I am not inclined to incorporate PatternLayout (PL) into > > > >>>>>>>>>>>>> JsonTemplateLayout (JTL). JTL is supposed to (ideally) > > > >> deliver all the > > > >>>>>>>>>>>>> functionality provided by PL but in a structured format, > > > >> i.e., JSON. > > > >>>>>>>>>>>>> If there arises a need to fallback to PL, I'd rather add > that > > > >> missing > > > >>>>>>>>>>>>> functionality to JTL. > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> You've mentioned about the checkstyle errors. How much > shall > > > >> I be > > > >>>>>>>>>>>>> concerned about them? (It even complains about missing > > > >> JavaDocs on > > > >>>>>>>>>>>>> class fields.) Are there any particular ones you want me > to > > > >> address? > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> Besides these... I think I am done. > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> Kind regards. > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>>> On Thu, Apr 16, 2020 at 10:49 PM Ralph Goers < > > > >> ralph.go...@dslextreme.com> wrote: > > > >>>>>>>>>>>>>> > > > >>>>>>>>>>>>>> Volkan, > > > >>>>>>>>>>>>>> > > > >>>>>>>>>>>>>> I’m just wondering if you have had a chance to get to > this. > > > >> As I said, as far as I am concerned this is the only thing getting > in the > > > >> way of merging the PR. If you would prefer you can just remove the > update > > > >> to the “Logging in the Cloud” page and push the rest and then > update that > > > >> page after you have the testing done. > > > >>>>>>>>>>>>>> > > > >>>>>>>>>>>>>> Ralph > > > >>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> On Apr 12, 2020, at 12:01 AM, Ralph Goers < > > > >> ralph.go...@dslextreme.com> wrote: > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> I think I said this before, but I see no reason you > can’t > > > >> have a dependency on Jackson. We currently do for almost all of our > JSON > > > >> processing. I wouldn’t spend any more time trying to improve the > > > >> performance of your custom support, especially since - as I recall > - you > > > >> had to drop support of a feature because it was hard to implement. > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> As for replacing JsonLayout and GelfLayout, I would > > > >> envision that those would continue to exist for backwards > compatibility but > > > >> would simply pass their configuration too JsonTemplateLayout. > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> As for moving JSON out of core, we may end up doing > that. I > > > >> am pretty sure the xml support will require a dependency on the > java.xml > > > >> module so either that will move out of core or we will have a > transitive > > > >> dependency on it. Whichever way we go with that I would expect > would be the > > > >> same with Json and Yaml. > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> Adding support for formatting the message with the > > > >> PatternLayout was pretty trivial. I would prefer to continue to > have that > > > >> in the example so I would hope it could be added if the example > changes. > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> As I said, the issue is Logstash or Fluentd and how > they > > > >> process messages. Adding the null terminator in the configuration > would > > > >> make it work on the log4j side as long as you left the logstash > > > >> configuration alone. My devops team has tried everything they can > think of > > > >> to get exceptions to work and this was the only thing that did > without > > > >> having to invent convoluted parsing rules. But yet, before we > publish that > > > >> I would need to know it had been tested all the way to Kibana. It > was > > > >> pretty easy for me to set it all up in docker containers. I think > the > > > >> log4j-spring-cloud-sample project has reference to the docker setup > I used. > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> Ralph > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> On Apr 11, 2020, at 11:23 PM, Volkan Yazıcı < > > > >> volkan.yaz...@gmail.com> wrote: > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Hey Ralph, > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Thanks so much for taking time to review such a > lengthy > > > >> PR, much > > > >>>>>>>>>>>>>>>> appreciated. Let me try to address your concerns and > share > > > >> some of > > > >>>>>>>>>>>>>>>> mine: > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Does JTL (i.e., JsonTemplateLayout) support Logstash? > I've > > > >> tried it > > > >>>>>>>>>>>>>>>> locally against an actually running Logstash instance > and > > > >> it has > > > >>>>>>>>>>>>>>>> worked fine. Though I did not test newlines. I will do > > > >> that. In any > > > >>>>>>>>>>>>>>>> case, AFAIC, JTL should also support null-termination > via > > > >>>>>>>>>>>>>>>> eventDelimiter="\u0000" configuration. Nevertheless, I > > > >> will try both > > > >>>>>>>>>>>>>>>> and report the outcome. (I wish we would support > Docker in > > > >> ITs to > > > >>>>>>>>>>>>>>>> allow tests against such external services, rather > than us > > > >> doing that > > > >>>>>>>>>>>>>>>> manually.) > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Does "message" directive in JTL support patterns ala > > > >> PatternLayout? > > > >>>>>>>>>>>>>>>> No, but it is on my TODO list. > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Can one extend the existing JTL template resolvers, > i.e., > > > >>>>>>>>>>>>>>>> EventResolverFactories? [I've extracted this remark > from > > > >> your most > > > >>>>>>>>>>>>>>>> recent GitHub comment.] No, but sounds like a great > idea! > > > >> This is not > > > >>>>>>>>>>>>>>>> a necessity for the first release (as you also have > > > >> stated), but I > > > >>>>>>>>>>>>>>>> will see what I can do about it. > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Right now I am busy with a performance regression. > After > > > >> replacing the > > > >>>>>>>>>>>>>>>> Jackson JsonGenerator with the in-house JsonWriter, > I've > > > >> observed a > > > >>>>>>>>>>>>>>>> ~2x performance degradation. I've been busy with it > for > > > >> almost a > > > >>>>>>>>>>>>>>>> month, but it is really difficult to spare time for > it due > > > >> to > > > >>>>>>>>>>>>>>>> full-time parenting enforced by closed day cares. I > do not > > > >> want to > > > >>>>>>>>>>>>>>>> have a release before fixing this issue. > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> I can imagine at some point 2.x will ship 3 contending > > > >> JSON layouts: > > > >>>>>>>>>>>>>>>> GelfLayout, JsonLayout, and JsonTemplateLayout. That > said, > > > >> for 3.x I > > > >>>>>>>>>>>>>>>> suggest to deprecate (maybe even remove?) the former > two. > > > >> Personally, > > > >>>>>>>>>>>>>>>> as a developer, what I find most confusing is a > library > > > >> providing > > > >>>>>>>>>>>>>>>> multiple solutions for the same problem without much > > > >> tangible benefit > > > >>>>>>>>>>>>>>>> between each other. I would rather strain from that > > > >> dilemma in a > > > >>>>>>>>>>>>>>>> library that I contribute to. > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Speaking of 3.x enhancements I have in mind, I am in > favor > > > >> of removing > > > >>>>>>>>>>>>>>>> all JSON logic (i.e., escaping/quoting routines) out > of > > > >> core module. > > > >>>>>>>>>>>>>>>> As a motivation, see my earlier post regarding > > > >> discrepancies between > > > >>>>>>>>>>>>>>>> multiple JSON quoting methods. Further, many (incl. > you) > > > >> also have > > > >>>>>>>>>>>>>>>> stated their concerns about the size of "core". I > don't > > > >> think a logger > > > >>>>>>>>>>>>>>>> should ship JSON encoders, decoders, and such. Though > I > > > >> did not > > > >>>>>>>>>>>>>>>> investigate this issue in detail, that is, what are > the > > > >> existing > > > >>>>>>>>>>>>>>>> dependencies, etc. Nevertheless, I wanted to raise > this > > > >> point. > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> Kind regards. > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>> On Sat, Apr 11, 2020 at 11:22 PM Ralph Goers < > > > >> ralph.go...@dslextreme.com> wrote: > > > >>>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>>> Volkan, > > > >>>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>>> I have looked at the PR and the only remaining thing > I > > > >> have an issue with are the changes you made to the logging in the > cloud > > > >> documentation. In my testing, only the Gelf Layout worked properly > with an > > > >> ELK stack. This is because Logstash (as well as Fluentd) are > parsing the > > > >> input trying to figure out where each log event begins and ends. > They > > > >> typically do that using some sort of pattern but by default look for > > > >> newlines. This will cause log events that contain exceptions, which > > > >> inherently include newlines) or other events that include newlines > in the > > > >> message to be broken into multiple lines in Kibana and make it > impossible > > > >> to filter on them properly. > > > >>>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>>> Have you tested the changes you have made to the > > > >> documentation with a full ELK stack and verified that all log > events, > > > >> including events with exceptions, are displayed correctly? > > > >>>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>>> Also, you will notice that the Gelf Layout includes a > > > >> MessagePattern attribute that allows the message element to be > formatted > > > >> using the PatternLayout. Does the Json Template Layout support > that? I > > > >> don’t see examples of that in any of the template files if it > does. If it > > > >> does not, that is another reason I would prefer leaving the page as > is for > > > >> now. > > > >>>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>>>> Ralph > > > >>>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>>> > > > >>>>>>>>>>>>>> > > > >>>>>>>>>>>>>> > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>>> > > > >>>>> > > > >>>> > > > >>> > > > >>> > > > >> > > > > -- > > > > Matt Sicker <boa...@gmail.com> > > > > > > >
