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