logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Date Sat, 13 Feb 2016 17:54:34 GMT
On Sat, Feb 13, 2016 at 3:38 AM, Remko Popma <remko.popma@gmail.com> wrote:

>
>
> On Saturday, 13 February 2016, Gary Gregory <garydgregory@gmail.com>
> wrote:
>
>> Hi Remko,
>>
>> Thank you for your constructive feedback :-)
>>
>> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <remko.popma@gmail.com>
>> wrote:
>>
>>> In the future, can we make big changes like this on a separate branch
>>> first so we can discuss them before they are committed to master?
>>>
>>
>> I considered doing that, but the changes where much smaller than I
>> initially thought. Also, did the new traceEntry/traceExit APIs come from a
>> branch? Arg, did I miss that? I thought it would be OK to work on these new
>> APIs in master since, well, they are new and I do not see a "1255" branch.
>> Maybe it's been removed?
>>
>
> There wasn't one. I guess the scope gradually increased during the course
> of this Jira issue...
>
>
>>
>>
>>> Some concrete feedback:
>>> * Why does ExitMessage keep a reference to the result Object? It is not
>>> used anywhere... (This could easily cause a memory leak...)
>>>
>>
>> The Object is used to create the formatted string.
>>
> Missed that. Okay.
>
>
>>
>> It seems that most (all?) Message implementations compute their formatted
>> strings on demand, this implementation does the same, hence the need to
>> keep track of the Object. Since a Message is only created if a level+marker
>> is enabled, it should be OK to compute formatted strings in ctors. We
>> should discuss this/do it, through another email thread. Would you care to
>> do the honors? But... computing these strings in ctors would make me want
>> to have the ivar be final. Keeping in mind the no-GC epic, would we want a
>> separate set of mutable messages? Or would we change the ones we have now
>> to be more flexible to play in a no-GC use case.
>>
>
> I'll update LOG4J2-1271
> <https://issues.apache.org/jira/browse/LOG4J2-1271>  with details on why
> ParameterizedMessage can be a bit thriftier. Not sure yet about what
> other Message implementations to support for the no-GC epic. Entry and exit
> currently need to walk the stacktrace to be useful (without this you don't
> know _what_ method you entered/exited), which makes them extremely slow...
> So I'm not considering them in scope for the no-GC epic.
>

The use cases I have do not use stack trace snapshots for flow logging.
>From my POV, until Java 9 provides a cheap way to capture methods from a
stack, I see this kind of flow logging as too expensive and kinda broken by
design. I wonder if there is a custom DLL we could write to introspect the
live stack... See the JIRA where I explained how I see it and how @LogFlow
could eliminate the need for looking at the stack altogether.


>
>
>>
>>> * I don't see the point of AbstractMessage. The Message interface
>>> already has a getFormattedMessage() method for obtaining the formatted
>>> string. Why can't we call that method?
>>>
>>
>>  The point of AbstractMessage is that subclasses do not need to implement
>> toString(). In order for messages to play nicely with MessageSupplier (or
>> is Supplier), it needs a toString() to get the formatted String. See also
>> the email thread about MS vs. S<?>.
>>
>
> I saw the email thread but I didn't get it then either. All it said was "a
> debug-oriented toString() does not make sense (to me at least, please see
> the test cases I committed today and play around)", which didn't help me.
>
> Generally I'm against AbstractXxx classes unless there really is no better
> way to do something. Classes like that encourage subclassing just to reuse
> some trivial logic, not a good thing. There are other ways to accomplish
> reuse, and I think sticking to just interfaces is cleaner.
>

I can see it both ways here. You argument is compelling. Not least because
some folks consider inheritance a "hack" which should be used for the right
reason, usually not for reusing common code only.

But let's go back to the basics here, the see what kind of solution to use:
The reason AbstractMessage exists is to make sure all messages work with
toString() and Suplier<?> nicely (for some version of 'nicely'). So a
Message 'should' always toString() as its formatted string.

Tests in org.apache.logging.log4j.LoggerSupplierTest like:

    @Test
    public void flowTracing_SupplierOfFormattedMessage() {
        logger.traceEntry(new Supplier<FormattedMessage>() {
            @Override
            public FormattedMessage get() {
                return new FormattedMessage("int foo={}", 1234567890);
            }
        });
        assertEquals(1, results.size());
        assertThat("Incorrect Entry", results.get(0), startsWith("ENTRY[
FLOW ] TRACE entry"));
        assertThat("Missing entry data", results.get(0),
containsString("(int foo=1234567890)"));
        assertThat("Bad toString()", results.get(0),
not(containsString("FormattedMessage")));
    }

would fail if the message did not implement toString() as
getFormattedString().

The question is: Is LoggerSupplierTest a reasonable use-case?

Gary


>
>>
>> * Why does the responsibility of MessageFactory need to increase with
>>> methods to create ExitMessage and EntryMessage? A separate
>>> ExitMessageFactory/EntryMessageFactory seems more appropriate and flexible.
>>>
>>
>> So org.apache.logging.log4j.spi.AbstractLogger would hold on to two
>> message factories? a "plain" message factory and a "flow" message factory?
>> Why? It seems perfectly natural for a "message factory" to produce all
>> different kinds of messages. I do not see why having two factories is more
>> flexible than one.
>>
>>
>>> * typo: AbstactFlowMessage -> AbstractFlowMessage
>>>
>>
>> Fixing...
>>
>> * typo: SimpleEntryMessage.DEAULT_TEXT -> DEFAULT_TEXT
>>>
>>
>> Fixing...
>>
>>
>>> * typo: SimpleExitMessage.DEAULT_TEXT -> DEFAULT_TEXT
>>>
>>
>> Fixing...
>>
>> FYI: As of now, I do not have a use case for an ExitMessage but I thought
>> it would be good for symmetry. But keeping YAGNI, we could just have
>> EntryMessage extend Message and then remove FlowMessage and ExitMessage.
>>
>
> If it can be removed we probably should. I agree with YAGNI: it might get
> in the way of something else we want to do later.
>

IMO, we should complete our current discussions on this topic and then
start pruning.

Gary

>
>
>>
>>
>>> Grumpily,
>>> Remko
>>> :-)
>>>
>>
>> ;-)
>> Gary
>>
>>
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> Java Persistence with Hibernate, Second Edition
>> <http://www.manning.com/bauer3/>
>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> Spring Batch in Action <http://www.manning.com/templier/>
>> Blog: http://garygregory.wordpress.com
>> Home: http://garygregory.com/
>> Tweet! http://twitter.com/GaryGregory
>>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
View raw message