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 >> >
