Is it worth thinking about a standalone StatusLogger? Gary
On Sun, Feb 21, 2016 at 5:50 PM, Remko Popma <[email protected]> wrote: > The recursive problem is that StatusLogger extends AbstractLogger... > > On Mon, Feb 22, 2016 at 10:40 AM, Gary Gregory <[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]> 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]> >>> 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]> >>> >> >> >> >> -- >> E-Mail: [email protected] | [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 >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> > > -- E-Mail: [email protected] | [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 Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
