On Sat, Feb 13, 2016 at 4:25 PM, Remko Popma <[email protected]> wrote:
> > > On Sunday, 14 February 2016, Gary Gregory <[email protected]> wrote: > >> On Sat, Feb 13, 2016 at 3:38 AM, Remko Popma <[email protected]> >> wrote: >> >>> >>> >>> On Saturday, 13 February 2016, Gary Gregory <[email protected]> >>> wrote: >>> >>>> Hi Remko, >>>> >>>> Thank you for your constructive feedback :-) >>>> >>>> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <[email protected]> >>>> 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: [email protected] | [email protected] >>>> 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: [email protected] | [email protected] >> 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: [email protected] | [email protected] 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
