Uploaded image for project: 'Log4net'
  1. Log4net
  2. LOG4NET-553

DebugAppender configuration should give the possibility to disable outputting loggerName as category

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.7
    • Fix Version/s: 2.0.8
    • Component/s: Appenders
    • Labels:
      None

      Description

      The DebugAppender always uses the System.Diagnostics.Debug.Write(string,string) overload passing in the loggername as the category parameter.

      It should be possible to omit the category parameter if having the loggername as category is not desired.

      Attached is a patch implementing the desired new feature, which would be activated by configuration:

      <appender name="DEBUG" type="log4net.Appender.DebugAppender">
          <outputCategory value="false"/>
          <layout type="log4net.Layout.PatternLayout">
      	  <conversionPattern value="%-5level: %message%newline"/>
          </layout>
        </appender>
      

      In my patch, the outputCategory parameter defaults to true, so the behavior doesnt change unless the new configuration is used.

      1. OutputCategoryParam-DebugAppender.cs.patch
        2 kB
        Jean-Francois Beaulac
      2. log4net-DebugAppenderCategory3.patch
        11 kB
        Jean-Francois Beaulac
      3. log4net-DebugAppenderCategory2.patch
        18 kB
        Jean-Francois Beaulac
      4. log4net-DebugAppenderCategory.patch
        18 kB
        Jean-Francois Beaulac

        Activity

        Hide
        bodewig Stefan Bodewig added a comment -

        Many thanks, Jean-Francois, I've committed you patch. I had to slightly amend it to exclude the tests in .NET Standard again as there is no Listeners property on Debug there.

        Show
        bodewig Stefan Bodewig added a comment - Many thanks, Jean-Francois, I've committed you patch. I had to slightly amend it to exclude the tests in .NET Standard again as there is no Listeners property on Debug there.
        Hide
        drake2k Jean-Francois Beaulac added a comment -

        Thanks for taking the time to help me with this patch.

        I'd sure like to get more involved.

        Show
        drake2k Jean-Francois Beaulac added a comment - Thanks for taking the time to help me with this patch. I'd sure like to get more involved.
        Hide
        nachbarslumpi Dominik Psenner added a comment -

        Finally had the time to read through your last patch. From reading it, it looks fine. I'm currently on the move and got only my phone with me, therefore I'm unable to apply the patch and do a thorough testing. I'll try to do that within the next 72 hours. Thanks for your patience and efforts! Would you like to get involved further? Log4net needs a man of your skills.

        Show
        nachbarslumpi Dominik Psenner added a comment - Finally had the time to read through your last patch. From reading it, it looks fine. I'm currently on the move and got only my phone with me, therefore I'm unable to apply the patch and do a thorough testing. I'll try to do that within the next 72 hours. Thanks for your patience and efforts! Would you like to get involved further? Log4net needs a man of your skills.
        Hide
        drake2k Jean-Francois Beaulac added a comment -

        Uploaded another patch containing the most recent proposal.

        Its my pleasure to be able to contribute a little to a great project we use everyday

        Show
        drake2k Jean-Francois Beaulac added a comment - Uploaded another patch containing the most recent proposal. Its my pleasure to be able to contribute a little to a great project we use everyday
        Hide
        nachbarslumpi Dominik Psenner added a comment - - edited

        Largely agreed, I would further handle the special case of empty category:

        if m_category == null
          call Debug.Write(string)
        else
          category = m_category.Format(loggingEvent)
          if string.Empty.Equals(category)
            call Debug.Write(string)
          else
            call Debug.Write(string, category)
        

        Sorry for the additional roundtrip and thanks for your time!

        Show
        nachbarslumpi Dominik Psenner added a comment - - edited Largely agreed, I would further handle the special case of empty category: if m_category == null call Debug.Write(string) else category = m_category.Format(loggingEvent) if string.Empty.Equals(category) call Debug.Write(string) else call Debug.Write(string, category) Sorry for the additional roundtrip and thanks for your time!
        Hide
        drake2k Jean-Francois Beaulac added a comment -

        Of course it would be simpler to just have a PatternLayout for the Category in the DebugAppender. By default it would be initialized to "%logger" and users could set the pattern to empty string to not output anything in the category, I created the ReturnsNullLayout class as per my understanding of the first proposal to have a SecondaryLayout that always returns null.

        As for passing null in the category parameter of the Debug.Write() method, there is nothing specific about it in the documentation. The TraceListener class handles it like so (from reference source 4.6.2):

        public virtual void Write(string message, string category) {
            if (Filter != null && !Filter.ShouldTrace(null, "", TraceEventType.Verbose, 0, message)) 
                return;
        
            if (category == null)
                Write(message);
            else
                Write(category + ": " + ((message == null) ? string.Empty : message));
        }
        

        But maybe it would be safer to call the Debug.Write(string) method if its null, since anyone can plug in their own TraceListener and the Write(string, string) method is virtual, someone could override it in a way that does not handle null category correctly.

        So what do you think if my patch:

        • Change Category property of DebugAppender to PatternLayout type, initialized with the pattern ""%logger"" to preserve actual behavior. Users can set the pattern to an empty string or the field to null to not have the category outputted.
        • if m_category == null, call Debug.Write(string)
        • if m_category != null, call Debug.Write(string, string)
        • No modifications to LayoutSkeleton (no need for the virtual specifier in the Format(LoggingEvent) method)
        • No need for ReturnsNullLayout class
        • Update tests and solution files accordingly
        Show
        drake2k Jean-Francois Beaulac added a comment - Of course it would be simpler to just have a PatternLayout for the Category in the DebugAppender. By default it would be initialized to "%logger" and users could set the pattern to empty string to not output anything in the category, I created the ReturnsNullLayout class as per my understanding of the first proposal to have a SecondaryLayout that always returns null. As for passing null in the category parameter of the Debug.Write() method, there is nothing specific about it in the documentation. The TraceListener class handles it like so (from reference source 4.6.2): public virtual void Write(string message, string category) { if (Filter != null && !Filter.ShouldTrace( null , "", TraceEventType.Verbose, 0, message)) return ; if (category == null ) Write(message); else Write(category + ": " + ((message == null ) ? string.Empty : message)); } But maybe it would be safer to call the Debug.Write(string) method if its null, since anyone can plug in their own TraceListener and the Write(string, string) method is virtual, someone could override it in a way that does not handle null category correctly. So what do you think if my patch: Change Category property of DebugAppender to PatternLayout type, initialized with the pattern ""%logger"" to preserve actual behavior. Users can set the pattern to an empty string or the field to null to not have the category outputted. if m_category == null, call Debug.Write(string) if m_category != null, call Debug.Write(string, string) No modifications to LayoutSkeleton (no need for the virtual specifier in the Format(LoggingEvent) method) No need for ReturnsNullLayout class Update tests and solution files accordingly
        Hide
        nachbarslumpi Dominik Psenner added a comment -

        I have a mixed feeling about the requirement to make the LayoutSkeleton.Format(loggingEvent) method virtual and the need of a ReturnsNullLayout class. From a semantical point of view, category := null is equivalent to category := string.Empty. Should the DebugAppender detect empty categories and invoke Debug.Write(message) [2] in that case? Not having null values would make the patch way more trivial, what do you think?

        I'm not so familiar with Debug.Write(message, category) [1]. If category is expected to be null (or empty), is the invocation of Debug.Write(message) [2] to be preferred? According to the documentation, it will invoke TraceListener.Write(message, category) [3] which mentions no limitations to the allowed argument values.

        References:
        [1] https://msdn.microsoft.com/en-us/library/a1z6t089(v=vs.110).aspx
        [2] https://msdn.microsoft.com/en-us/library/54ffa64k(v=vs.110).aspx
        [3] https://msdn.microsoft.com/en-us/library/sawdbchb(v=vs.110).aspx

        Show
        nachbarslumpi Dominik Psenner added a comment - I have a mixed feeling about the requirement to make the LayoutSkeleton.Format(loggingEvent) method virtual and the need of a ReturnsNullLayout class. From a semantical point of view, category := null is equivalent to category := string.Empty. Should the DebugAppender detect empty categories and invoke Debug.Write(message) [2] in that case? Not having null values would make the patch way more trivial, what do you think? I'm not so familiar with Debug.Write(message, category) [1] . If category is expected to be null (or empty), is the invocation of Debug.Write(message) [2] to be preferred? According to the documentation, it will invoke TraceListener.Write(message, category) [3] which mentions no limitations to the allowed argument values. References: [1] https://msdn.microsoft.com/en-us/library/a1z6t089(v=vs.110).aspx [2] https://msdn.microsoft.com/en-us/library/54ffa64k(v=vs.110).aspx [3] https://msdn.microsoft.com/en-us/library/sawdbchb(v=vs.110).aspx
        Hide
        drake2k Jean-Francois Beaulac added a comment -

        1. No they are not required. I will remove them from the patch.

        2. That makes sense, I always used log4net via XmlConfiguration so it didnt cross my mind that one could programmatically set it to null.

        I uploaded a new patch with the suggestions, I also added a test that checks that the DebugAppender does not throw an exception if m_category is null and correctly passes null to the Write() method.

        Show
        drake2k Jean-Francois Beaulac added a comment - 1. No they are not required. I will remove them from the patch. 2. That makes sense, I always used log4net via XmlConfiguration so it didnt cross my mind that one could programmatically set it to null. I uploaded a new patch with the suggestions, I also added a test that checks that the DebugAppender does not throw an exception if m_category is null and correctly passes null to the Write() method.
        Hide
        nachbarslumpi Dominik Psenner added a comment -

        I just reviewed your patch and it looks good! All these tests give me a good feeling about your patch. However, there are things that need to be discussed before we can continue:

        1. Are the modifications in the TraceAppender required? The issue is unrelated to that appender and therefore this patch should not modify it.
        2. In the class DebugAppender you have added the line:
        System.Diagnostics.Debug.Write(RenderLoggingEvent(loggingEvent), m_category.Format(loggingEvent));
        Should we pipe null into Debug.Write() if m_category happens to be null?

        Show
        nachbarslumpi Dominik Psenner added a comment - I just reviewed your patch and it looks good! All these tests give me a good feeling about your patch. However, there are things that need to be discussed before we can continue: 1. Are the modifications in the TraceAppender required? The issue is unrelated to that appender and therefore this patch should not modify it. 2. In the class DebugAppender you have added the line: System.Diagnostics.Debug.Write(RenderLoggingEvent(loggingEvent), m_category.Format(loggingEvent)); Should we pipe null into Debug.Write() if m_category happens to be null?
        Hide
        drake2k Jean-Francois Beaulac added a comment -

        Sure,

        I have noticed that the TraceAppender already does something similar to what you suggested, so I did something similar.

        I added a Category property to the DebugAppender of type LayoutSkeleton, it defaults to new PatternLayout("%logger")

        I created a LayoutClass named ReturnsNullLayout that derives from LayoutSkeleton, in which all methods are NOOP. In LayoutSkeleton I made the Format(LoggingEvent) method virtual to be able to override it in the ReturnsNullLayout class and avoid creating a StringWriter instance for no reason.

        I wrote tests for the DebugAppender, none existed. I copied the TraceAppender tests. I also wrote a test for the ReturnsNullLayout class. I ran the tests and they all passed.

        My patch also adds the files to the vs2012/2010 and 2008 solutions. I can test that 2012 and 2010 build, but I dont have 2008 to test if it builds. I also modified the .NET core project files which I am not familiar with, but it seems to work in visual studio.

        Let me know what you think.

        Thanks,
        Jf

        Show
        drake2k Jean-Francois Beaulac added a comment - Sure, I have noticed that the TraceAppender already does something similar to what you suggested, so I did something similar. I added a Category property to the DebugAppender of type LayoutSkeleton, it defaults to new PatternLayout("%logger") I created a LayoutClass named ReturnsNullLayout that derives from LayoutSkeleton, in which all methods are NOOP. In LayoutSkeleton I made the Format(LoggingEvent) method virtual to be able to override it in the ReturnsNullLayout class and avoid creating a StringWriter instance for no reason. I wrote tests for the DebugAppender, none existed. I copied the TraceAppender tests. I also wrote a test for the ReturnsNullLayout class. I ran the tests and they all passed. My patch also adds the files to the vs2012/2010 and 2008 solutions. I can test that 2012 and 2010 build, but I dont have 2008 to test if it builds. I also modified the .NET core project files which I am not familiar with, but it seems to work in visual studio. Let me know what you think. Thanks, Jf
        Hide
        nachbarslumpi Dominik Psenner added a comment -

        Would you like to write a patch that implements the outlined proposal?

        Show
        nachbarslumpi Dominik Psenner added a comment - Would you like to write a patch that implements the outlined proposal?
        Hide
        drake2k Jean-Francois Beaulac added a comment -

        I think your idea is much better than mine. It is more flexible while allowing my usecase and still preserving the current behavior.

        Show
        drake2k Jean-Francois Beaulac added a comment - I think your idea is much better than mine. It is more flexible while allowing my usecase and still preserving the current behavior.
        Hide
        nachbarslumpi Dominik Psenner added a comment -

        Hi!

        Thanks for the patch. While I can see the usecase for the feature, a more generic approach is in my opinion to be preferred. For instance, a second layout (i.e. CategoryLayout) property could be introduced in the DebugAppender that is then used to format the category parameter. Once that API is introduced, your usecase can then be implemented by specifying a SecondaryLayout that always returns null. Further, the default CategoryLayout would return the LoggerName in order to preserve the current behavior. What do you think?

        Cheers

        Show
        nachbarslumpi Dominik Psenner added a comment - Hi! Thanks for the patch. While I can see the usecase for the feature, a more generic approach is in my opinion to be preferred. For instance, a second layout (i.e. CategoryLayout) property could be introduced in the DebugAppender that is then used to format the category parameter. Once that API is introduced, your usecase can then be implemented by specifying a SecondaryLayout that always returns null. Further, the default CategoryLayout would return the LoggerName in order to preserve the current behavior. What do you think? Cheers

          People

          • Assignee:
            Unassigned
            Reporter:
            drake2k Jean-Francois Beaulac
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development