I see two new properties to allow users to override the default MessageFactory and FlowMessageFactory. It seems very unlikely they will ever get used, but they should NOT be calling System.getProperty() directly.
Please remember that wherever adding something to the configuration won’t work you should access it through the log4j2.component.properties file. Values in that file can be overridden via system properties, but users can just create the properties file instead. In general, we should be leveraging that mechanism and not calling System.getProperty(). We also need to document each of these properties in a clear way. JAXB or Jackson isn’t going to make anything any easier. Ralph > On Feb 19, 2016, at 12:20 PM, Gary Gregory <[email protected]> wrote: > > > On Fri, Feb 19, 2016 at 9:24 AM, Remko Popma <[email protected] > <mailto:[email protected]>> wrote: > > > > FlowMessageFactory is now extracted. I'm quite happy with the result. > > Please take a look at https://issues.apache.org/jira/browse/LOG4J2-1255 > > <https://issues.apache.org/jira/browse/LOG4J2-1255> for further follow-up. > > OK, that seems fine. Thank you for doing the work. > > The only thing I am not happy about is the use of system properties instead > of the config file. > > We are perpetuating a mess here. > > What is the role of properties files vs a configuration file? Which one > overrides the other? Are they mutually exclusive? > > I could see sys props set on a command line used to override all config > files. Or the other way around? > > In the long run, the use of sys props is bad. Some users configure only via > files saved and moved around machines. You can't do that with sys props. > > Please, let's not make it worse by adding MORE sys props. > > Is the real issue that it is too much of a PITA to update our config code for > XML, JSON, and YAML to support a new setting? > > This tells me we're doing it wrong. I know we do not want to many deps, our > current scheme is too hard to maintain. We could use JAXB or Jackson instead. > > Gary > > > > > On Fri, Feb 19, 2016 at 2:40 PM, Remko Popma <[email protected] > > <mailto:[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] > >> <mailto:[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] > >>>> <mailto:[email protected]>> wrote: > >>>> > >>>> On Feb 18, 2016 5:38 PM, "Remko Popma" <[email protected] > >>>> <mailto:[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] > >>>> > <mailto:[email protected]>> wrote: > >>>> >> > >>>> >> On Thu, Feb 18, 2016 at 4:22 PM, Ralph Goers > >>>> >> <[email protected] <mailto:[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] > >>>> >>>> <mailto:[email protected]>> wrote: > >>>> >>>> > >>>> >>>> On Thu, Feb 18, 2016 at 2:13 PM, Remko Popma <[email protected] > >>>> >>>> <mailto:[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] > >>>> >>>>> <mailto:[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] > >>>> >>>>>> <mailto:[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] <mailto:[email protected]> | > >>>> >>>> [email protected] <mailto:[email protected]> > >>>> >>>> Java Persistence with Hibernate, Second Edition > >>>> >>>> JUnit in Action, Second Edition > >>>> >>>> Spring Batch in Action > >>>> >>>> Blog: http://garygregory.wordpress.com > >>>> >>>> <http://garygregory.wordpress.com/> > >>>> >>>> Home: http://garygregory.com/ <http://garygregory.com/> > >>>> >>>> Tweet! http://twitter.com/GaryGregory > >>>> >>>> <http://twitter.com/GaryGregory> > >>>> >>> > >>>> >>> > >>>> >> > >>>> >> > >>>> >> > >>>> >> -- > >>>> >> E-Mail: [email protected] <mailto:[email protected]> | > >>>> >> [email protected] <mailto:[email protected]> > >>>> >> Java Persistence with Hibernate, Second Edition > >>>> >> JUnit in Action, Second Edition > >>>> >> Spring Batch in Action > >>>> >> Blog: http://garygregory.wordpress.com > >>>> >> <http://garygregory.wordpress.com/> > >>>> >> Home: http://garygregory.com/ <http://garygregory.com/> > >>>> >> Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory> > >>> > >>> > > > > > -- > E-Mail: [email protected] <mailto:[email protected]> | > [email protected] <mailto:[email protected]> > Java Persistence with Hibernate, Second Edition > JUnit in Action, Second Edition > Spring Batch in Action > Blog: http://garygregory.wordpress.com <http://garygregory.wordpress.com/> > Home: http://garygregory.com/ <http://garygregory.com/> > Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory>
