Hello all, thanks for integrating this pull request.
Any chance that someone reviews my other pull request https://github.com/apache/logging-log4j2/pull/224 that fixes https://issues.apache.org/jira/browse/LOG4J2-2403 and hopefully integrate it as well? Best regards Marco On Sun, Nov 04, 2018 at 11:12:51AM +0900, Remko Popma wrote: > The updated PR looks good to me. > > Remko. > > (Shameless plug) Every java main() method deserves http://picocli.info > > > On Nov 4, 2018, at 1:11, Carter Kozak <[email protected]> wrote: > > > > The PR looks good to me as well. > > > > As a future improvement we may want to fall back on a failed lookup on > > Level.getName() to Level.getStandardLevel().getName() for the closest > > matching standard level style. > >> On Sat, Nov 3, 2018 at 11:47 AM Gary Gregory <[email protected]> > >> wrote: > >> > >> Marco: Thank you for updating your PR. > >> Remko: The PR has been changed to use String keys instead of Levels. LGTM. > >> WDYT? > >> > >> Gary > >> > >>> On Thu, Oct 25, 2018 at 12:57 AM Marco Herrn <[email protected]> wrote: > >>> > >>> Hello, > >>> > >>> I expected that the second approach would be desired. > >>> > >>> I have changed the pull requests accordingly. Can you please recheck? > >>> > >>> Best regards > >>> Marco > >>> > >>> > >>>> On Thu, Oct 25, 2018 at 07:29:28AM +0900, Remko Popma wrote: > >>>> Creating custom Levels in HighLightConverter feels wrong to me. > >>>> > >>>> It’s possible to register custom log levels programmatically as well as > >>> in configuration ( > >>> https://logging.apache.org/log4j/2.x/manual/customloglevels.html). > >>>> > >>>> If the user has not defined the log levels by the time the > >>> HighLightConverter is used, the user made a mistake. > >>>> > >>>> The existing code handles this mistake in a strict manner, and logs an > >>> error to the console. I think that’s good, it’s the best we can do to > >>> notify the user of their mistake. > >>>> (Please revert the error logging. ) > >>>> > >>>> Now, how to improve things to handle this situation gracefully? > >>>> What about changing the map of styles in HighLightConverter to use the > >>> level _name_ (String) as key instead of Level objects? > >>>> > >>>> That would allow us to register the style even though the custom Level > >>> has not been defined yet. If this Level is defined and used later, the > >>> name-based lookup will find the associated style. > >>>> > >>>> Remko. > >>>> > >>>> (Shameless plug) Every java main() method deserves http://picocli.info > >>>> > >>>>> On Oct 24, 2018, at 21:31, Marco Herrn <[email protected]> wrote: > >>>>> > >>>>> Hello, > >>>>> > >>>>> I have prepared a pull request for LOG4J2-2405. > >>>>> There is still one decision to be made how to handle custom log levels. > >>>>> I added my comments to the pull request, but duplicate it here to have > >>> a > >>>>> broader audience for discussing it: > >>>>> > >>>>> To allow styles for unknown log levels the HighlightConverter creates > >>> these new > >>>>> log levels itself with an int value of Integer.MAX_VALUE. The problem > >>> is, when > >>>>> the actual application code tries to register this log level itself, > >>> the given > >>>>> int value will be ignored and Integer.MAX_VALUE remains. This is due > >>> to the > >>>>> implementation of Level.forName(String, int), which specifies that the > >>> intValue > >>>>> is ignored when called a second time with the same level name. (This > >>> is > >>>>> actually not a problem when the unknown log level is registered before > >>> parsing > >>>>> the log4j2 config file, but we can not enforce this.) > >>>>> > >>>>> I can think of two solutions to this problem: > >>>>> > >>>>> 1. Rewrite Level.forName(String, int) to always update the intValue > >>> with the given > >>>>> one. > >>>>> 2. Change the HighlightConverter to store the mapping of levels to > >>> styles > >>>>> not as Map<Level, String>, but instead as Map<String, String> where > >>> the key is > >>>>> the levels name. > >>>>> > >>>>> I see option 1 as the "more correct" one, since I think when > >>>>> calling Level.forName(String, intValue) the user would expect that the > >>> last > >>>>> definition wins, not the first one (or at least an Exception should be > >>> thrown > >>>>> and there needs to be a ways to override the existing definition of a > >>> log > >>>>> level). However it is also a more intrusive change as it changes one > >>> of the > >>>>> base classes of the API and changes the behaviour of a public method. > >>>>> > >>>>> Option 2 is less intrusive as it only changes some internals of > >>>>> HighlightConverter. However I still think that the current behaviour of > >>>>> Level.forName(String, intValue) is incorrect and should actually be > >>> changed. > >>>>> > >>>>> ---- > >>>>> > >>>>> Please share your idea about this. > >>>>> > >>>>> JIRA-Ticket: https://issues.apache.org/jira/browse/LOG4J2-2405 > >>>>> Pull-Request: https://github.com/apache/logging-log4j2/pull/225 > >>>>> > >>>>> Best regards > >>>>> Marco > >>>
