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
