I should have qualified this to say that the log4j2.component.properties file is managed by the PropertiesUtil class. Properties should be access through its methods.
Ralph > On Feb 19, 2016, at 1:09 PM, Ralph Goers <[email protected]> wrote: > > 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] >> <mailto:[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>
