You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Remko Popma <re...@gmail.com> on 2016/02/13 09:17:28 UTC
LOG4J2-1255 Initial feedback on Logger/Message API changes
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?
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...)
* 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?
* 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.
* typo: AbstactFlowMessage -> AbstractFlowMessage
* typo: SimpleEntryMessage.DEAULT_TEXT -> DEFAULT_TEXT
* typo: SimpleExitMessage.DEAULT_TEXT -> DEFAULT_TEXT
Grumpily,
Remko
:-)
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Remko Popma <re...@gmail.com>.
On Sunday, 14 February 2016, Gary Gregory <ga...@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 <re...@gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Saturday, 13 February 2016, Gary Gregory <ga...@gmail.com>
>>>> wrote:
>>>>
>>>>> Hi Remko,
>>>>>
>>>>> Thank you for your constructive feedback :-)
>>>>>
>>>>> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <re...@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
>
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Gary Gregory <ga...@gmail.com>.
On Sat, Feb 13, 2016 at 4:25 PM, Remko Popma <re...@gmail.com> wrote:
>
>
> On Sunday, 14 February 2016, Gary Gregory <ga...@gmail.com> wrote:
>
>> On Sat, Feb 13, 2016 at 3:38 AM, Remko Popma <re...@gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Saturday, 13 February 2016, Gary Gregory <ga...@gmail.com>
>>> wrote:
>>>
>>>> Hi Remko,
>>>>
>>>> Thank you for your constructive feedback :-)
>>>>
>>>> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <re...@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 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.
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 | 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
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Remko Popma <re...@gmail.com>.
On Sunday, 14 February 2016, Gary Gregory <ga...@gmail.com> wrote:
> On Sat, Feb 13, 2016 at 3:38 AM, Remko Popma <remko.popma@gmail.com
> <javascript:_e(%7B%7D,'cvml','remko.popma@gmail.com');>> wrote:
>
>>
>>
>> On Saturday, 13 February 2016, Gary Gregory <garydgregory@gmail.com
>> <javascript:_e(%7B%7D,'cvml','garydgregory@gmail.com');>> wrote:
>>
>>> Hi Remko,
>>>
>>> Thank you for your constructive feedback :-)
>>>
>>> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <re...@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?
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.)
>
>>
>>
>>>
>>>> * 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() ?
>
> 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
> <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
>
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Gary Gregory <ga...@gmail.com>.
On Sat, Feb 13, 2016 at 3:38 AM, Remko Popma <re...@gmail.com> wrote:
>
>
> On Saturday, 13 February 2016, Gary Gregory <ga...@gmail.com>
> wrote:
>
>> Hi Remko,
>>
>> Thank you for your constructive feedback :-)
>>
>> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <re...@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
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Gary Gregory <ga...@gmail.com>.
On Sat, Feb 13, 2016 at 7:47 AM, Ralph Goers <ra...@dslextreme.com>
wrote:
> I didn’t create a branch because I didn’t think this was that big of a
> deal. All I was doing was adding new methods to make entry and exit tracing
> more usable. I created the MessageSupplier methods so the would be uniform
> with the debug, trace, etc methods. I see they have now been modified to
> use Supplier<Message> so that seems OK.
>
> I have a use case to log the response object as Json. It can be done in a
> couple of ways:
>
> 1. logger.traceExit(response, gson->toJson(response));
>
I could see this use case being needed if you want to configure Gson to
pretty-print or not.
Gary
2. logger.traceExit(response, new JsonMessage(response));
>
> Personally, I like option 2 for this use case but I can imagine others
> where option 1 might be preferred. Right now we have both a traceExit that
> takes a MessageSupplier and one that takes Supplier<? extends Message>.
> This doesn’t seem correct as their is no way to provide a Supplier that
> allows option 1 above. That said, it appears I neglected to add that when I
> first committed these changes.
>
> The ExitMessage adds “exit” to the start of the message just as the
> EntryMessage adds “entry”. We always want those at the beginning of the
> message so I don’t think ExitMessage should be removed.
>
> Ralph
>
>
>
>
>
> On Feb 13, 2016, at 4:38 AM, Remko Popma <re...@gmail.com> wrote:
>
>
>
> On Saturday, 13 February 2016, Gary Gregory <ga...@gmail.com>
> wrote:
>
>> Hi Remko,
>>
>> Thank you for your constructive feedback :-)
>>
>> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <re...@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.
>
>
>>
>>> * 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.
>
>
>>
>> * 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.
>
>
>>
>>
>>> 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
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Ralph Goers <ra...@dslextreme.com>.
I didn’t create a branch because I didn’t think this was that big of a deal. All I was doing was adding new methods to make entry and exit tracing more usable. I created the MessageSupplier methods so the would be uniform with the debug, trace, etc methods. I see they have now been modified to use Supplier<Message> so that seems OK.
I have a use case to log the response object as Json. It can be done in a couple of ways:
1. logger.traceExit(response, gson->toJson(response));
2. logger.traceExit(response, new JsonMessage(response));
Personally, I like option 2 for this use case but I can imagine others where option 1 might be preferred. Right now we have both a traceExit that takes a MessageSupplier and one that takes Supplier<? extends Message>. This doesn’t seem correct as their is no way to provide a Supplier that allows option 1 above. That said, it appears I neglected to add that when I first committed these changes.
The ExitMessage adds “exit” to the start of the message just as the EntryMessage adds “entry”. We always want those at the beginning of the message so I don’t think ExitMessage should be removed.
Ralph
> On Feb 13, 2016, at 4:38 AM, Remko Popma <re...@gmail.com> wrote:
>
>
>
> On Saturday, 13 February 2016, Gary Gregory <garydgregory@gmail.com <ma...@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 <ma...@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.
>
>
> * 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.
>
>
> * 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.
>
>
>
> Grumpily,
> Remko
> :-)
>
> ;-)
> Gary
>
>
>
> --
> E-Mail: <>garydgregory@gmail.com <ma...@gmail.com> | <>ggregory@apache.org <ma...@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 <http://garygregory.wordpress.com/>
> Home: http://garygregory.com/ <http://garygregory.com/>
> Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory>
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Remko Popma <re...@gmail.com>.
On Saturday, 13 February 2016, Gary Gregory <ga...@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
> <javascript:_e(%7B%7D,'cvml','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.
>
>> * 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.
>
> * 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.
>
>
>> Grumpily,
>> Remko
>> :-)
>>
>
> ;-)
> Gary
>
>
>
> --
> 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
>
Re: LOG4J2-1255 Initial feedback on Logger/Message API changes
Posted by Gary Gregory <ga...@gmail.com>.
Hi Remko,
Thank you for your constructive feedback :-)
On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <re...@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?
> 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.
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 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<?>.
* 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.
> 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