On Sunday, 14 February 2016, Gary Gregory <[email protected]> wrote:
> On Sat, Feb 13, 2016 at 3:38 AM, Remko Popma <[email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >> >> >> On Saturday, 13 February 2016, Gary Gregory <[email protected] >> <javascript:_e(%7B%7D,'cvml','[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? 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: [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] > <javascript:_e(%7B%7D,'cvml','[email protected]');> | [email protected] > <javascript:_e(%7B%7D,'cvml','[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 >
