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