FlowMessageFactory is now extracted. I'm quite happy with the result. Please take a look at https://issues.apache.org/jira/browse/LOG4J2-1255 for further follow-up.
On Fri, Feb 19, 2016 at 2:40 PM, Remko Popma <[email protected]> wrote: > I see, so there actually is a use case to remove the need for the > isTraceEnabled check with the Supplier param... > > Sent from my iPhone > > On 2016/02/19, at 14:10, Ralph Goers <[email protected]> wrote: > > The use case I wanted to do this for is: > > LOGGER.entry(“Request: “, ()->gson.toJson(request)); > . > LOGGER.exit(response, ()->gson.toJson(response)); > > However this can be handled just as easily by > > LOGGER.entry(new JsonMessage(request)); > . > LOGGER.exit(response, new JsonMessage(response)); > > so I can live without the Supplier. I don’t think MessageSupplier actually > makes any sense. I can’t see why I would want to do: > > LOGGER.entry(()->new JsonMessage(request)); > > since it is just creating one object instead of another. > > Ralph > > On Feb 18, 2016, at 7:52 PM, Gary Gregory <[email protected]> wrote: > > On Feb 18, 2016 5:38 PM, "Remko Popma" <[email protected]> wrote: > > > > I would start with just a default FlowMessageFactory. Configurable with > a system property, so users can swap in their own if they want. > > > > Only if the need arises to configure FlowMessageFactories on a > per-logger basis, we can consider adding the methods to LogManager to > support that. > > > > So no need for additional getLogger methods for now. > > > > The default FlowMessageFactory implementation would be the logic > that's in AbstractMessageFactory now. Gary wrote it so I assume it meets > his needs. > > > > Gary, shall we deprecate MessageSupplier and remove > entry/exitTrace methods using them? > > That's fine with me. > > Gary > > > > > > > On Friday, 19 February 2016, Gary Gregory <[email protected]> > wrote: > >> > >> On Thu, Feb 18, 2016 at 4:22 PM, Ralph Goers < > [email protected]> wrote: > >>> > >>> Is it really necessary to have getLogger support FlowMessageFactory? > These messages are really meant as wrappers for other messages. so I am not > even sure what it would mean for getLogger() to support that. How would it > know what Message it is wrapping? > >>> > >>> > >>> I am really getting sorry that I started this. > >> > >> > >> Well, hopefully, whatever happens, this is getting all of us into > reviewing existing and new code. > >> > >> Another benefit of this conversation is that I fell that we have been > remarkably civil and respectful of each other, at least compared to other > outrageous behavior one can read about on the webs. > >> > >> The use case I want most is in > org.apache.logging.log4j.LoggerTest.flowTracingString_ObjectArray2_ParameterizedMessageFactory() > >> > >> Which can be summarized as: > >> > >> Logger myLogger = LogManager.getLogger("Some.Logger", new > ParameterizedMessageFactory("Enter", "Exit")); > >> EntryMessage msg = myLogger.traceEntry("doFoo(a={}, b={})", 1, 2); > >> myLogger.traceExit(msg, 3); > >> > >> If I cannot pass in my flow message factory or if there are now two > factories, I need to be able to set it somehow. > >> > >> I do not like the idea of have a setFlowMessageFactory on a Logger > because I'd never want to change it. > >> > >> Gary > >> > >>> > >>> Ralph > >>> > >>>> On Feb 18, 2016, at 3:31 PM, Gary Gregory <[email protected]> > wrote: > >>>> > >>>> On Thu, Feb 18, 2016 at 2:13 PM, Remko Popma <[email protected] > > wrote: > >>>>> > >>>>> I think preserving binary compatibility on its own is a strong > reason for doing this, but it's more than that. > >>>> > >>>> > >>>> OK, since org.apache.logging.log4j.message.MessageFactory is in > log4j-api that's important. I can buy that. BUT, we are also adding methods > to Logger so that would break some things too. I guess less breakage is > better than more in this case! > >>>> > >>>> Overall, I not convinced that this is the best approach but I can > appreciate that you seem to feel about it stronger that I do. > >>>> > >>>>> > >>>>> > >>>>> Having a separate factory for flow messages makes both factories > more cohesive (single responsibility principle). No need for one factory to > extend the other in my view. > >>>> > >>>> > >>>> The distinction is pretty subtle here IMO. We are still talking about > creating messages, but I get your point. For me, the only reason for this > is to minimize the risk of API breakage, a nobe goal for the log4j-api > module, if not a requirement. > >>>> > >>>>> > >>>>> > >>>>> The logger would have separate instances so users can configure them > separately: lower coupling. > >>>> > >>>> > >>>> OK. So now we have: > >>>> > >>>> org.apache.logging.log4j.LogManager.getLogger(Class<?>, > MessageFactory) > >>>> org.apache.logging.log4j.LogManager.getLogger(Object, MessageFactory) > >>>> org.apache.logging.log4j.LogManager.getLogger(String, MessageFactory) > >>>> > >>>> We would add: > >>>> > >>>> org.apache.logging.log4j.LogManager.getLogger(Class<?>, > MessageFactory, FlowMessageFactory) > >>>> org.apache.logging.log4j.LogManager.getLogger(Object, MessageFactory, > FlowMessageFactory) > >>>> org.apache.logging.log4j.LogManager.getLogger(String, MessageFactory, > FlowMessageFactory) > >>>> > >>>> Right? Any other places? > >>>> > >>>>> > >>>>> > >>>>> These are both desirable properties so I believe it would improve > the design. > >>>>> > >>>>> Does this make sense? > >>>> > >>>> > >>>> Sure, even though I am less gun-ho about it than you are. I'd say go > ahead, see how it looks and feels after you refactor. We can keep > discussing it once your changes hits the repo if need be. > >>>> > >>>> Thank you for putting in the work! > >>>> Gary > >>>> > >>>>> Remko > >>>>> > >>>>> Sent from my iPhone > >>>>> > >>>>> On 2016/02/19, at 2:24, Gary Gregory <[email protected]> wrote: > >>>>> > >>>>>> Is a flow message factory a kind of message factory or a different > kind of factory? > >>>>>> > >>>>>> Does a logger need instances of both or just the one? > >>>>>> > >>>>>> Since entry message extends message, should the factory do so as > well? > >>>>>> > >>>>>> Gary, phone, typos. > >>>>>> > >>>>>> On Feb 18, 2016 8:44 AM, "Remko Popma" <[email protected]> > wrote: > >>>>>>> > >>>>>>> Would anyone mind terribly if I factored out the FlowMessage > creation methods from MessageFactory to a new interface FlowMessageFactory? > >>>>>>> > >>>>>>> Concretely, this interface would contain the methods introduced in > LOG4J2-1255: > >>>>>>> > >>>>>>> EntryMessage newEntryMessage(Message message); > >>>>>>> ExitMessage newExitMessage(Object object, Message message); > >>>>>>> ExitMessage newExitMessage(EntryMessage message); > >>>>>>> ExitMessage newExitMessage(Object object, EntryMessage message); > >>>>>>> > >>>>>>> I think flow messages are different enough from normal Messages > that a separate factory makes sense. > >>>>>>> > >>>>>>> It would also insulate users who created a custom MessageFactory > from the changes we made in LOG4J2-1255. > >>>>>>> > >>>>>>> Thoughts? > >>>>>>> > >>>>>>> -Remko > >>>>>>> > >>>>>>> > >>>> > >>>> > >>>> > >>>> -- > >>>> E-Mail: [email protected] | [email protected] > >>>> Java Persistence with Hibernate, Second Edition > >>>> JUnit in Action, Second Edition > >>>> Spring Batch in Action > >>>> 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 > >> JUnit in Action, Second Edition > >> Spring Batch in Action > >> Blog: http://garygregory.wordpress.com > >> Home: http://garygregory.com/ > >> Tweet! http://twitter.com/GaryGregory > > >
