Hi Ivan, I am very in favor of this proposal, as we had occasion to discuss this in the past, the structured logging would be one way to have a better standardized way to attach context to the events, in a way that is human and machine readable.
For me, it became evident after the experience using Logrus, a structured logging library for Go (https://github.com/sirupsen/logrus). There is a lot of good stuff in that API and I think we should look well at it to take inspiration for a new API. I also agree that, for much that I would have liked to, the current SLF4J 2.0 alphas are not that promising in their current state. I just have a few comments: 1. I like the idea of using enum to identify an event, but I fear that the enum name itself will not be, in many cases, as expressive of the context as a complete phrase. I understand that the documentation will have the long form explanation of the event, although: a) It would be good, for a person reasonably familiar with Pulsar, to be able to understand most of the event messages without having to recur to the documentation b) In many cases, for better or for worse, we're using the logging statement also as a form of documentation in the code (eg: we omit what would be a comment, since the log statement is already explanatory enough. So, if we make the logging statement less rich, it would have the risk to also reduce that form of documentation. For example, in the proposed scenario it's used "Events.LOOKUP". There is a lot of context that would be missing there. Eg: is this an HTTP or Protobuf lookup, did we log when the operation started or when it was completed, etc... 2. We should make it easy to automatically generate documentation in HTML for the event. For this, using annotation might be a better option than Javadoc. 3. We should have an easy way to create partial loggers with pre-filled context. This context will be used for all the events created through that logger and at the same time we could add new context specific to the particular event. For example, a `ServerCnx` object should create a new Logger when it gets created, with context = {localAddress="xxx", remoteAddress="yyy"}. This context would be carried by every event that happens from the `ServerCnx`. The same would be super-useful for many other classes, where we want to be consistent in having the precise context in every event. 4. Thread Local context -- It might be difficult to have meaningful TLC between Pulsar & BK code when we do have most of the code being asynchronous with the results coming out of a callback that is triggered in a separate thread. 5. We should borrow some of the good parts of the APIs in SLF4J 2.0. For example, given that it now supports Java 8, we should be also taking `Supplier<Object>` for the arguments so that we can use lambdas to avoid object creations when the logger level is disabled. Similarly, instead of doing : e.attr("result", lookupResult) .info(Events.LOOKUP); we could change the order into: e.atInfo(Events.LOOKUP) .attr("result", lookupResult) .log(); This would avoid doing any extra work if the level info is disabled. 6. Even if we should make structured argument the preferred way to log event, I'd be wary of not permitting to have: a) Custom string messages b) String templating for cases in which adding to context map is not the best choice. Thanks, Matteo -- Matteo Merli <mme...@apache.org> On Thu, Aug 5, 2021 at 11:11 AM Ivan Kelly <iv...@apache.org> wrote: > > Seems like all the feedback is positive. What's the process for moving > the PIP to approved? I don't see anything on the wiki. Nor in the > bylaws (where are they? they're not linked on site menu). > > -Ivan > > On Wed, Aug 4, 2021 at 5:11 PM Ivan Kelly <iv...@apache.org> wrote: > > > > > I wonder how we will be able to enforce such way of logging once we are > > > done with the switch. > > No all logging needs to be done this way. This will coexist with > > traditional slf4j logging. > > > > However, hopefully this will prove useful enough that a string > > interpolated log will > > start to stick out like a sore thumb when seen in reviews. > > Also bad logging doesn't take away from the good logging. We're > > already past the point > > that eyeballing the logs is any use, so if there's noise, it can just > > be ignored with the rest of the noise > > > > -Ivan
