You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Gary Gregory <ga...@gmail.com> on 2018/11/03 15:47:10 UTC
Re: Pull Request for [LOG4J2-2405] Better handling of %highlight
pattern when using jul-bridge
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 <ml...@mherrn.de> 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 <ml...@mherrn.de> 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
>
Re: Pull Request for [LOG4J2-2405] Better handling of %highlight
pattern when using jul-bridge
Posted by Marco Herrn <ml...@mherrn.de>.
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 <c4...@gmail.com> 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 <ga...@gmail.com> 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 <ml...@mherrn.de> 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 <ml...@mherrn.de> 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
> >>>
Re: Pull Request for [LOG4J2-2405] Better handling of %highlight pattern when using jul-bridge
Posted by Remko Popma <re...@gmail.com>.
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 <c4...@gmail.com> 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 <ga...@gmail.com> 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 <ml...@mherrn.de> 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 <ml...@mherrn.de> 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
>>>
Re: Pull Request for [LOG4J2-2405] Better handling of %highlight
pattern when using jul-bridge
Posted by Carter Kozak <c4...@gmail.com>.
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 <ga...@gmail.com> 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 <ml...@mherrn.de> 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 <ml...@mherrn.de> 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
> >