I initially used PropertiesUtil but this failed somehow. Since this is used while initializing s class constant, the failure resulted in a NoClassDefError... So I reverted to System.getProperties. I can take another look, or if someone else has time, please feel free to replace this with PropertiesUtil.
On Saturday, 20 February 2016, Ralph Goers <[email protected]> wrote: > 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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > > > On Fri, Feb 19, 2016 at 9:24 AM, Remko Popma <[email protected] > <javascript:_e(%7B%7D,'cvml','[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 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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >>>> > >>>> On Feb 18, 2016 5:38 PM, "Remko Popma" <[email protected] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >>>> >> > >>>> >> On Thu, Feb 18, 2016 at 4:22 PM, Ralph Goers < > [email protected] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >>>> >>>> > >>>> >>>> On Thu, Feb 18, 2016 at 2:13 PM, Remko Popma < > [email protected] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');> | > [email protected] <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');> | > [email protected] <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');> | > [email protected] <javascript:_e(%7B%7D,'cvml','[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 > > > >
