logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Remko Popma <remko.po...@gmail.com>
Subject Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Date Sun, 14 Feb 2016 04:24:45 GMT
On Sunday, 14 February 2016, Gary Gregory <garydgregory@gmail.com> wrote:

> On Sat, Feb 13, 2016 at 4:25 PM, Remko Popma <remko.popma@gmail.com
> <javascript:_e(%7B%7D,'cvml','remko.popma@gmail.com');>> wrote:
>
>>
>>
>> On Sunday, 14 February 2016, Gary Gregory <garydgregory@gmail.com
>> <javascript:_e(%7B%7D,'cvml','garydgregory@gmail.com');>> wrote:
>>
>>> 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.
>>>
>>
>> Exactly! LOG4J2-33, right?
>>
>
> Yes: https://issues.apache.org/jira/browse/LOG4J2-33
>
>
>> It seems to me that this Jira (if we can get it to work) can provide a
>> much more elegant solution that would not only allow inserting
>> "entry"/"exit" but also insert method name and line number without runtime
>> overhead! (Since code is generated at compile time.)
>>
>
> Hm, wouldn't this be done by weaving traceEntry/Exit on existing .class
> files? Or does Java annotation processing allow you to edit bytes code
> during compilation? I need to read up on this...
>

I was assuming something like that but to honest I also need to study
what's possible.


>
>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>> * 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.
>>>
>>
>> I'm pretty much in that corner, I consider inheritance overused. :-)
>>
>>
>>>
>>> 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?
>>>
>>
>> I'm not sure what the type of the result variable is (away from PC), but
>> assuming it is Message, can't the test assertion be made on
>> results.get(0).getFormattedString() ?
>>
>
> Silly mistake on my part, the test is now fixed and no longer @Ignore'd.
>

That's good news. So we can remove AbstractMessage?


> Gary
>
>
>>
>>> 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.
>>>>>
>>>> One reason is that users may have released software that included a
>> custom MessageFactory, and that will break when their users upgrade to
>> Log4j-2.6.
>>
>> Also, further modifications similar to making the "exit"/"enter" message
>> customizable (i18n, etc), will not have to touch 6 message factory
>> implementations, which is nice.
>>
>>
>>>>>
>>>>>> * 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
>>>
>>
>
>
> --
> E-Mail: garydgregory@gmail.com
> <javascript:_e(%7B%7D,'cvml','garydgregory@gmail.com');> | ggregory@apache.org
> <javascript:_e(%7B%7D,'cvml','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