RFR 2: JDK-8005263: Logging APIs takes Supplier<String> for message

[Henry Jen]

Jason,

If I understand you correctly, there are two main concerns,

1. You want to encourage MessageFormat style logging, consider the other way is an anti-pattern.
2. The construction of message could be further eliminated when a filter is involved.

Here is what I thought,

For 1, the new API is simply enabling developer to provide a log message for the LogRecord, without forcing any formatting. In fact, because it's a Supplier, it actually enables all possibility about formatting. I can imagine all current MessageFormat style logging capability implemented as a Supplier.

For 2, I don't disagree with you. However, as you said, it's worst case scenario when *all* handlers *never* try to call LogRecord.getMessage(). With serialization implications and compatibility concerns, we don't want to rush a change on LogRecord. Instead, current patch is simple and should be able to address many cases.

Would it be good enough if we can defer the message construction after Logger.filter check, but construct the message before publish to Handlers? As Peter had pointed out earlier, we can add a reference in LogRecord to exchange for filtering at Logger level. But we cannot deferred further down to handler level without side-effects.

Now again, for MessageFormat style logging, we don't lose any capability, and the laziness is already possible via proper use of parameters. The reason I don't include such a helper class for lambda to be used in parameter is that it's trivial. For parameter version, those should probably be customized to work with formatter anyway.

I think it is reasonable to say the current patch is a step forward, and there are probably rooms to improve in the future. Next version I'll enable defer message construction until Logger filter check.

Cheers,
Henry

On Dec 28, 2012, at 7:15 AM, Jason Mehrens <jason_mehrens at hotmail.com> wrote:

> 
> Henry,
> 
> It is beautiful if that code lives only inside of the logger and the caller is unaware of its existence. Same is true for writing a guard statement.  
> 
> The formatter can not always achieve the same laziness. If I converted your example in the API docs to be passed in the params argument I would have to evaluate it to a string before the logger call or create a supplier that overrides toString to return DiagnosisMessages.systemHealthStatus(). We are forcing caller to create the ugly code which the exact opposite of what you are trying to do with this patch.
> 
> As soon as you use a MemoryHandler or install a Handler filter the levels will be the lowest number required. The problem is you end up throwing out or filtering out volumes of LogRecords that are never formatted. This is common when you are hunting for a specific error and what to see the last X number of messages that led up to the error.  That was the technique used to reopen bug id 6840067.  If I created a working proof of concept would that change anything or is this patch a done deal?
> 
> Jason
>> Subject: Re: RFR 2: JDK-8005263: Logging APIs takes Supplier<String> for message
>> From: henry.jen at oracle.com
>> Date: Thu, 27 Dec 2012 18:24:17 -0800
>> CC: brian.goetz at oracle.com; lambda-dev at openjdk.java.net; core-libs-dev at openjdk.java.net
>> To: jason_mehrens at hotmail.com
>> 
>> As stated in the beginning of the review thread, the reason we don't have Supplier version for other API is that a formatter is involved and can achieve same laziness easily.
>> 
>> The key is MessageFormatter can take arguments, which will be used to generate the output. As there is no directly support for Supplier, a possible solution is to have a wrapper class take a Supplier lambda,
>> 
>> class <T> Collector {
>> final Supplier<T> supplier;
>> 
>> Collector(Supplier<T> supplier) {
>> this.supplier = supplier;
>> }
>> 
>> T collect() {
>> return supplier.get();
>> }
>> 
>> @Override
>> String toString() {
>> return supplier.get().toString();
>> }
>> }
>> 
>> It's not entirely beautiful, but is a general solution for applying supplier for MessageFormat.
>> 
>> As the handler issue, I would argue that the logger would set to the lowest number required to be logged, i.e, at least one handler will format the message, thus the timing is not an real issue given the other caching/serialization concern discussed.
>> 
>> Hope that helps, let me know if I missed something.
>> 
>> Cheers,
>> Henry
>> 
>> 
>> On Dec 27, 2012, at 4:16 PM, Jason Mehrens <jason_mehrens at hotmail.com> wrote:
>> 
>>> Brian,
>>> 
>>> It's on my list too for lambdafying I just disagree with current implementation. I understand and agree that having to create a guard should not be required. It's awful to have to do. The point is that patch is still way too eager because you don't want to evaluate a message until the LogRecord arrives at a formatter inside of a handler. I don't want force anyone to use catalogs. I want to force everyone to use message parameterized logging (with or without a catalog) because it is powerful, lazy, and doesn't taste like a vegetable. If your concern is sugary sweets then, add overloaded methods that make message parameterized logging easy for the caller. If patch was rewritten like I proposed, it would be as terse as your example bellow. The difference is it would actually be lazy and would be useful when you need to create a custom filter to find some ghost in a production machine. Relevant or not, my patch would also enable lambdas and message catalogs. That is something that will never happen under the current patch.
>>> 
>>> Jason
>>> 
>>>> I think a significant fraction of the community would disagree with you.
>>>> We ran a survey where we collected suggestions for lambdafying API
>>>> methods, and this one came in top of the list.
>>>> There is a significant fraction of the developer community that uses the
>>>> logging API and doesn't care at all about localization, but does care
>>>> about logging performance. One doesn't have to look very far to see
>>>> that it is common practice to surround logging calls with
>>>> 
>>>> if (logger.isLogging(level))
>>>> logger.log(level, msgExpr)
>>>> 
>>>> to work around the eager evaluation. And such a practice is brittle,
>>>> because it's easy to forget to do it in one place, and lose the benefit.
>>>> Now, your answer might be "force all users to use message catalogs."
>>>> But that's pretty mean. A lot of users really, really don't want to use
>>>> message catalogs. They want to do:
>>>> 
>>>> logger.log(level, () -> String.format(...));
>>>> 
>>>> You're basically saying we should force-feed those users some
>>>> message-catalog vegetables, because it's good for them.
>>> 
>>> 
>>