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
