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