Re: Change semantics of `Throwable` parameters
I’ll note that MessageSupplier is deprecated, and we eventually added support for Supplier, so that’s already there. Since it sounds like we have some good ideas on what a minimal v3.Logger API would look like, it would be cool if anyone would like to propose a full API. Here’s another Logger API to consider (from OpenTelemetry, something I’ve been considering for future integration purposes, though this might be mostly on the Flume side of things): https://javadoc.io/doc/io.opentelemetry/opentelemetry-api-logs/latest/io/opentelemetry/api/logs/LogRecordBuilder.html (their Logger returns this builder class). — Matt Sicker > On Nov 23, 2023, at 03:31, Piotr P. Karwasz wrote: > > Hi Matt, > > On Tue, 21 Nov 2023 at 23:22, Matt Sicker wrote: >> >> This sounds like it might be a good basis for figuring out a parallel v3 API >> for a “hard to mis-use” style API. However, once you go that route, you >> start to wonder how useful templated log messages are when you can capture a >> lambda instead. Parameterized log messages might work better as structured >> log messages, something that is awkward to use in the API at the moment. > > If we'll create a separate `v3.Logger` interface I would clean it up > from many methods, e.g.: > > * getLevel() and getName(): how are these useful for the user? An `if > (logger.getLevel() == Level.INFO)` should be replaced by `isEnabled`, > * getMessageFactory() and getFlowMessageFactory() (the latter is my > fault): again these are not useful to the user. If I need a message > factory, it will be a different message factory, > * printf(): a better approach is to use StringFormatterMessageFactory > for the logger, > * catching(Throwable): can be replaced with `error(Object)` and the > semantics described in this thread, > * throwing: does anybody use it? Maybe it could stay, > * entry/exit, traceEntry/traceExit: I can not imagine using these on > each method (or important method). I'd rather use AspectJ pointcuts > instead (or a @LogTrace annotation), > * methods that use `MessageSupplier` like `info(MessageSupplier)`: > couldn't these be integrated into the logic of `info(Supplier)`? > * the `is*Enabled` methods are prone to misuse: a snippet like: > > if (logger.isDebugEnabled()) { > logger.debug(MARKER, "Hello world!"); > } > > will not print any message if the level of the logger is less specific > than DEBUG, even if the user asks for **all** MARKER messages to be > printed. > > IMHO opinion `v3.AbstractLogger` should only have 2 abstract methods: > * logMessage(Level level, Marker marker, String fqcn, > StackTraceElement location, Message message, Throwable throwable) > * isEnabled(Level level, Marker marker, String fqcn, > StackTraceElement location, Message message, Throwable throwable) > > Piotr
Re: Change semantics of `Throwable` parameters
I agree with everything Remko said plus these extra comments. Throwing is a convenience method to standardize the output when an application will be throwing an exception. That, of course, means it isn’t a required method. I doubt it is used very much. Of course, catching is its companion but I would bet it is used even less. We use the entry/exit stuff everywhere. I have not seen a replacement for it that provides the same functionality so I would not want it removed. I agree that AbstractLogger should have as few methods as possible that need to be overridden by the implementation. Ralph > On Nov 23, 2023, at 4:31 AM, Piotr P. Karwasz wrote: > Hi Matt, > > On Tue, 21 Nov 2023 at 23:22, Matt Sicker wrote: >> >> This sounds like it might be a good basis for figuring out a parallel v3 API >> for a “hard to mis-use” style API. However, once you go that route, you >> start to wonder how useful templated log messages are when you can capture a >> lambda instead. Parameterized log messages might work better as structured >> log messages, something that is awkward to use in the API at the moment. > > If we'll create a separate `v3.Logger` interface I would clean it up > from many methods, e.g.: > > * getLevel() and getName(): how are these useful for the user? An `if > (logger.getLevel() == Level.INFO)` should be replaced by `isEnabled`, > * getMessageFactory() and getFlowMessageFactory() (the latter is my > fault): again these are not useful to the user. If I need a message > factory, it will be a different message factory, > * printf(): a better approach is to use StringFormatterMessageFactory > for the logger, > * catching(Throwable): can be replaced with `error(Object)` and the > semantics described in this thread, > * throwing: does anybody use it? Maybe it could stay, > * entry/exit, traceEntry/traceExit: I can not imagine using these on > each method (or important method). I'd rather use AspectJ pointcuts > instead (or a @LogTrace annotation), > * methods that use `MessageSupplier` like `info(MessageSupplier)`: > couldn't these be integrated into the logic of `info(Supplier)`? > * the `is*Enabled` methods are prone to misuse: a snippet like: > > if (logger.isDebugEnabled()) { > logger.debug(MARKER, "Hello world!"); > } > > will not print any message if the level of the logger is less specific > than DEBUG, even if the user asks for **all** MARKER messages to be > printed. > > IMHO opinion `v3.AbstractLogger` should only have 2 abstract methods: > * logMessage(Level level, Marker marker, String fqcn, > StackTraceElement location, Message message, Throwable throwable) > * isEnabled(Level level, Marker marker, String fqcn, > StackTraceElement location, Message message, Throwable throwable) > > Piotr
Re: Change semantics of `Throwable` parameters
> On Nov 23, 2023, at 18:31, Piotr P. Karwasz wrote: > > Hi Matt, > >> On Tue, 21 Nov 2023 at 23:22, Matt Sicker wrote: >> >> This sounds like it might be a good basis for figuring out a parallel v3 API >> for a “hard to mis-use” style API. However, once you go that route, you >> start to wonder how useful templated log messages are when you can capture a >> lambda instead. Parameterized log messages might work better as structured >> log messages, something that is awkward to use in the API at the moment. > > If we'll create a separate `v3.Logger` interface I would clean it up > from many methods, e.g.: > > * getLevel() and getName(): how are these useful for the user? An `if > (logger.getLevel() == Level.INFO)` should be replaced by `isEnabled`, Sorry but I disagree. The getLevel method is useful when the level to compare to is not known in advance. > * getMessageFactory() and getFlowMessageFactory() (the latter is my > fault): again these are not useful to the user. If I need a message > factory, it will be a different message factory, Agreed. > * printf(): a better approach is to use StringFormatterMessageFactory > for the logger, I see your point, but in my experience the printf method is rarely used, only for specific use cases; most logging in the same class would use the info(), debug() etc methods. Without printf users would need to declare a separate logger. The printf method is a nice convenience. > * catching(Throwable): can be replaced with `error(Object)` and the > semantics described in this thread, Agreed. > * throwing: does anybody use it? Maybe it could stay, I never use it. > * entry/exit, traceEntry/traceExit: I can not imagine using these on > each method (or important method). I'd rather use AspectJ pointcuts > instead (or a @LogTrace annotation), Agreed. > * methods that use `MessageSupplier` like `info(MessageSupplier)`: > couldn't these be integrated into the logic of `info(Supplier)`? Yes. > * the `is*Enabled` methods are prone to misuse: a snippet like: > > if (logger.isDebugEnabled()) { > logger.debug(MARKER, "Hello world!"); > } > > will not print any message if the level of the logger is less specific > than DEBUG, even if the user asks for **all** MARKER messages to be > printed. Interesting. By the way; isn’t that behavior the same even if there’s no “if” condition around the logger.debug(MARKER, "Hello world!"); statement? > > IMHO opinion `v3.AbstractLogger` should only have 2 abstract methods: > * logMessage(Level level, Marker marker, String fqcn, > StackTraceElement location, Message message, Throwable throwable) > * isEnabled(Level level, Marker marker, String fqcn, > StackTraceElement location, Message message, Throwable throwable) > > Piotr
Re: Change semantics of `Throwable` parameters
Hi Matt, On Tue, 21 Nov 2023 at 23:22, Matt Sicker wrote: > > This sounds like it might be a good basis for figuring out a parallel v3 API > for a “hard to mis-use” style API. However, once you go that route, you start > to wonder how useful templated log messages are when you can capture a lambda > instead. Parameterized log messages might work better as structured log > messages, something that is awkward to use in the API at the moment. If we'll create a separate `v3.Logger` interface I would clean it up from many methods, e.g.: * getLevel() and getName(): how are these useful for the user? An `if (logger.getLevel() == Level.INFO)` should be replaced by `isEnabled`, * getMessageFactory() and getFlowMessageFactory() (the latter is my fault): again these are not useful to the user. If I need a message factory, it will be a different message factory, * printf(): a better approach is to use StringFormatterMessageFactory for the logger, * catching(Throwable): can be replaced with `error(Object)` and the semantics described in this thread, * throwing: does anybody use it? Maybe it could stay, * entry/exit, traceEntry/traceExit: I can not imagine using these on each method (or important method). I'd rather use AspectJ pointcuts instead (or a @LogTrace annotation), * methods that use `MessageSupplier` like `info(MessageSupplier)`: couldn't these be integrated into the logic of `info(Supplier)`? * the `is*Enabled` methods are prone to misuse: a snippet like: if (logger.isDebugEnabled()) { logger.debug(MARKER, "Hello world!"); } will not print any message if the level of the logger is less specific than DEBUG, even if the user asks for **all** MARKER messages to be printed. IMHO opinion `v3.AbstractLogger` should only have 2 abstract methods: * logMessage(Level level, Marker marker, String fqcn, StackTraceElement location, Message message, Throwable throwable) * isEnabled(Level level, Marker marker, String fqcn, StackTraceElement location, Message message, Throwable throwable) Piotr
Re: Change semantics of `Throwable` parameters
This sounds like it might be a good basis for figuring out a parallel v3 API for a “hard to mis-use” style API. However, once you go that route, you start to wonder how useful templated log messages are when you can capture a lambda instead. Parameterized log messages might work better as structured log messages, something that is awkward to use in the API at the moment. — Matt Sicker > On Nov 21, 2023, at 08:16, Piotr P. Karwasz wrote: > > Hi, > > Profiting from release 3.x we could slightly change the interpretation > of throwable parameters used in logging methods. > > IIRC in one of our online meetings, we raised the issue of logging > statements like this: > > logger.info("Error nr {}: {}", nr, throwable); > > which is not considered best practice and is probably not what the > user intended: > > logger.info("Error nr {}: {}", nr, throwable.getMessage(), throwable); > > A similar problem was reported in PR #1050[2]. > > Regarding this kind of statements, the OSGi Log[1] Service provides a > very nice alternative interpretation on how to deal with throwable > parameters: > > "If the last argument is a Throwable or a ServiceReference, it is > added to the generated LogEntry and then, if the next to last argument > is a ServiceReference or Throwable and not the same type as the last > argument, it is also added to the generated LogEntry. These arguments > will not be used as message arguments." > > Should we adopt a similar interpretation in 3.x? The advantages I see are: > > 1. We fix some inconsistencies in the API: > * we have a pair `info(String, Throwable)/info(String, Object)` > methods that prevents users from mistakenly pass a `Throwable` as a > log message argument, > * we only have `info(Object)`, > * we only have `info(String, Object, Object)`. > 2. We simplify our logic: right now we need logic to deal with > throwables in `Message`. On the other hand throwables are not > considered part of a message by any layout and are also included > separately in LogEvent. If we don't need to chase throwables in > parameterized messages, we don't need to parse the format string until > it is required. > 3. There might be some cases when including throwables in a log > message leads to information disclosure (e.g. when a user sees a log > message, but not the attached throwable). This way we'll have an API > which is easy to use and hard to misuse. > > Piotr > > [1] > https://docs.osgi.org/javadoc/osgi.core/8.0.0/org/osgi/service/log/Logger.html > [2] https://github.com/apache/logging-log4j2/pull/1050