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 >
