RE: Feedback for log4net.Contrib.AspNetPatternConverters

[Nicko Cadell <ni...@neoworks.com>]

Ron,

This does look like a very nice idea and does provide additional
functionality that log4net does not have out of the box. 

I think that it makes sense to be able to register new converters into
the pattern layout on a per-repository basis. I will need to think about
where that would be stored, but I think that it would make the
registration of this kind of large pack of additional patterns much
simpler. E.g. it could be done with a single <plugin type="xxx" /> line
in the config file.


> Should there be try/catch blocks around the code or is 
> checking for null values good enough?

That depends. Checking for null may be good enough if no other
exceptions can be thrown. If you don't know it is better to catch any
exception and output an error into the output; otherwise the log message
will not be output, or it will be corrupt.


> I'm not sure how to handle access to certain properties like 
> the Count property of the Cache object. If I use this 
> conversion pattern:
> 
>  <conversionPattern value="%cache{Count} - %m%n" />
> 
> I'm unable to determine which of the following the user wants
> displayed:
> 
>  Cache.Count
>  Cache["Count"]

I think that to be consistent the %cache{Count} must mean
Cache["Count"]. You can always have another %cache-count pattern to get
the property value.


> Is there a more appropriate naming convention for my classes? 
> The design for the inner classes is based off of 
> FileAppender's inner file locking classes.

The inner classes in the appenders are not exactly best practice
according to the MS design guidelines. The guidelines say that inner
classes should not be used as a way of grouping related classes together
- which is sort of what that are used for in the appenders. The
guidelines recommend using nested namespaces to achieve this.


> 
> I'd like to hear some feedback and have an chance to make 
> somemore minor changes to the code before I make it availabe 
> for download.
> 
> Comments, questions, cool, not cool, don't care? 

Definitely cool.

Nicko


> 
> - Ron
> 
> 

RE: Feedback for log4net.Contrib.AspNetPatternConverters

[Ron Grabowski <ro...@yahoo.com>]

I don't know why I didn't think of this sooner...if you have groups of
related converters that you'd like to register the amount of additional
xml nodes makes for some ugly xml:

<layout type="log4net.Layout.PatternLayout">
 <converter>
  <name value="aspnet-response" />
  <type
value="log4netContrib.AspNetPatternConverters.ResponsePatternConverter,
log4net.Contrib.AspNetPatternConverters" />
 </converter>
 <converter>
  <name value="aspnet-response-cookies" />
  <type
value="log4netContrib.AspNetPatternConverters.ResponsePatternConverter+Cookies+Value,
log4net.Contrib.AspNetPatternConverters" />
 </converter>
 <conversionPattern value="%p %d %aspnet-response{ContentType}
%aspnet-response-cookies-value{MyCookie} - %m%n" />
</layout>

A cleaner alternative is to group related converters into a layout:

public class AspNetPatternLayout : PatternLayout
{
 public override void ActivateOptions()
 {
  AddConverter("aspnet-cache", typeof(AspNetCachePatternConverter));
  AddConverter("aspnet-context",
typeof(AspNetContextPatternConverter));
  AddConverter("aspnet-request",
typeof(AspNetRequestPatternConverter));
  AddConverter("aspnet-session",
typeof(AspNetSessionPatternConverter));
  // snip
  ActivateOptions();
 }

The confusing xml is now reduced to:

 <layout
  type="log4netContrib.Layout.AspNetPatternLayout, log4netContrib"
  value="%aspnet-request{WidgetId} %aspnet-context{ProductId}" />

--- Nicko Cadell <ni...@neoworks.com> wrote:

> Ron,
> 
> This does look like a very nice idea and does provide additional
> functionality that log4net does not have out of the box. 
> 
> I think that it makes sense to be able to register new converters
> into
> the pattern layout on a per-repository basis. I will need to think
> about
> where that would be stored, but I think that it would make the
> registration of this kind of large pack of additional patterns much
> simpler. E.g. it could be done with a single <plugin type="xxx" />
> line
> in the config file.
> 
> 
> > Should there be try/catch blocks around the code or is 
> > checking for null values good enough?
> 
> That depends. Checking for null may be good enough if no other
> exceptions can be thrown. If you don't know it is better to catch any
> exception and output an error into the output; otherwise the log
> message
> will not be output, or it will be corrupt.
> 
> 
> > I'm not sure how to handle access to certain properties like 
> > the Count property of the Cache object. If I use this 
> > conversion pattern:
> > 
> >  <conversionPattern value="%cache{Count} - %m%n" />
> > 
> > I'm unable to determine which of the following the user wants
> > displayed:
> > 
> >  Cache.Count
> >  Cache["Count"]
> 
> I think that to be consistent the %cache{Count} must mean
> Cache["Count"]. You can always have another %cache-count pattern to
> get
> the property value.
> 
> 
> > Is there a more appropriate naming convention for my classes? 
> > The design for the inner classes is based off of 
> > FileAppender's inner file locking classes.
> 
> The inner classes in the appenders are not exactly best practice
> according to the MS design guidelines. The guidelines say that inner
> classes should not be used as a way of grouping related classes
> together
> - which is sort of what that are used for in the appenders. The
> guidelines recommend using nested namespaces to achieve this.
> 
> 
> > 
> > I'd like to hear some feedback and have an chance to make 
> > somemore minor changes to the code before I make it availabe 
> > for download.
> > 
> > Comments, questions, cool, not cool, don't care? 
> 
> Definitely cool.
> 
> Nicko
> 
> 
> > 
> > - Ron
> > 
> > 
>