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