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