It still has to implement Logger. I think PropertiesUtil is probably just a special case we will have to live with or do something custom for.
Ralph > On Feb 21, 2016, at 9:49 PM, Gary Gregory <[email protected]> wrote: > > Well, it would not have to be as fancy perhaps? > > Gary > > On Sun, Feb 21, 2016 at 6:22 PM, Matt Sicker <[email protected] > <mailto:[email protected]>> wrote: > That's like a thousand extra lines. Yikes! > > On 21 February 2016 at 19:55, Gary Gregory <[email protected] > <mailto:[email protected]>> wrote: > Is it worth thinking about a standalone StatusLogger? > > Gary > > On Sun, Feb 21, 2016 at 5:50 PM, Remko Popma <[email protected] > <mailto:[email protected]>> wrote: > The recursive problem is that StatusLogger extends AbstractLogger... > > On Mon, Feb 22, 2016 at 10:40 AM, Gary Gregory <[email protected] > <mailto:[email protected]>> wrote: > Is there a way StatusLogger could use System.err when it is not fully > initialized? So that everything can still use StatusLogger? > > Gary > > On Sun, Feb 21, 2016 at 5:20 PM, Matt Sicker <[email protected] > <mailto:[email protected]>> wrote: > 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] > <mailto:[email protected]>> wrote: > It might be. > > Ralph > >> On Feb 21, 2016, at 4:19 PM, Matt Sicker <[email protected] >> <mailto:[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] >> <mailto:[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] >> <mailto:[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] >> <mailto:[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 >>>> > <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 >>>> >>>> >>>> <http://garygregory.wordpress.com/> >>>> >>>> >>>> Home: http://garygregory.com/ <http://garygregory.com/> >>>> >>>> >>>> Tweet! http://twitter.com/GaryGregory >>>> >>>> >>>> <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 >>>> >>>> >> <http://garygregory.wordpress.com/> >>>> >>>> >> Home: http://garygregory.com/ <http://garygregory.com/> >>>> >>>> >> Tweet! http://twitter.com/GaryGregory >>>> >>>> >> <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 <http://garygregory.wordpress.com/> >>>> Home: http://garygregory.com/ <http://garygregory.com/> >>>> Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory> >> >> >> >> >> >> -- >> Matt Sicker <[email protected] <mailto:[email protected]>> > > > > > -- > Matt Sicker <[email protected] <mailto:[email protected]>> > > > > -- > E-Mail: [email protected] <mailto:[email protected]> | > [email protected] <mailto:[email protected]> > Java Persistence with Hibernate, Second Edition > <http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > 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 > <http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > 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> > > > -- > Matt Sicker <[email protected] <mailto:[email protected]>> > > > > -- > E-Mail: [email protected] <mailto:[email protected]> | > [email protected] <mailto:[email protected]> > Java Persistence with Hibernate, Second Edition > <http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > 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>
