Considering every log message in PropertiesUtil is an error message, maybe that class could just use System.err instead.
On 21 February 2016 at 19:12, Ralph Goers <[email protected]> wrote: > It might be. > > Ralph > > On Feb 21, 2016, at 4:19 PM, Matt Sicker <[email protected]> wrote: > > Maybe PropertiesUtil is too low-level to use StatusLogger? I remember > fighting with a similar problem a while ago due to cyclic dependencies like > that. > > On 21 February 2016 at 07:13, Remko Popma <[email protected]> wrote: > >> I tried again to use PropertiesUtil.getProperties().getStringProperty() >> instead of System.getProperty, without success. >> The reason is that this logic is executed to initialize a static field in >> AbstractLogger; PropertiesUtil internally uses StatusLogger, which extends >> AbstractLogger. >> >> I see many log4j-api tests fail: it starts with >> java.lang.NoClassDefFoundError: Could not initialize class >> org.apache.logging.log4j.status.StatusLogger >> and goes downhill from there... >> >> Perhaps there is some way around this but I don't see it... >> >> >> On Sat, Feb 20, 2016 at 7:18 AM, Remko Popma <[email protected]> >> wrote: >> >>> I documented the new properties in the Configuration manual page. Did I >>> forget to commit that? >>> >>> >>> On Saturday, 20 February 2016, Remko Popma <[email protected]> >>> wrote: >>> >>>> 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]> >>>>> 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]> >>>>> wrote: >>>>> >>>>> >>>>> On Fri, Feb 19, 2016 at 9:24 AM, Remko Popma <[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]> >>>>> 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 >>>>> >>> >>>>> >>> >>>>> > >>>>> >>>>> -- >>>>> 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 >>>>> >>>>> >>>>> >>>>> >> > > > -- > Matt Sicker <[email protected]> > > > -- Matt Sicker <[email protected]>
