Multiple Factories / Appenders #8

Closed
ramonsmits opened this Issue Jun 21, 2012 · 30 comments

Projects

None yet

8 participants

@ramonsmits

I have a situation where I would want to use log4net for colored console output and entlib for persistent logging. I don't see something that allows multiple factories acting like the log4net AppenderCollection. This would be a nice addition and maybe somebody already has made this but I could not find it.

@ramonsmits

In other words, allow for multiple factoryAdapter's

@ramonsmits

I have made a simple implementation for it. It is not configurable via .config but can be used in code. Just create a MultiFactoryLoggerFactoryAdapter and add multiple LoggerFactoryAdapters to the LoggerFactoryAdapters,

It currently relies on Linq thus not 2.0 compatible. If you think it is worth including then I can extend it with some tests and maybe make it configurable by xml config.

@sbohlen
Member
sbohlen commented Jun 21, 2012

I think this would be a great feature to add but, yes, we'd need both tests and an xml-based config story for it.

@kerryjiang

I aslo support it. We should can get adapter by name and then get logger instance.

@ramonsmits

@kerryjiang as in you already have it working? I have it working in code and have been busy in the configuration section. I did not test that yet but this can probably be used to continue.

@kerryjiang

Not yet. In my project I developed my own common logging interface, but I prefer to use an existing one which is publicly in use.

My basic requirement is we can define multiple logfactoryAdapters and let code to decide to use which one.

@ramonsmits

If you want to configure that by code then you can already do that with my branch.

@MisinformedDNA

@ramonsmits What's the status of your project? It seems like a really good idea and I know I would make use of it.

@ramonsmits

You can use my common.logging repository. I have only published the version which makes it possible to configure multiple factories in code.

I have a version somewhere that supports configuring multiple factory adapters via the xml configuration but I should really search for it.

@amrlafi
amrlafi commented Mar 15, 2014

+1 for Multiple Adapter support. would be really useful.

@nZeus
nZeus commented Apr 2, 2014

+1 for Multiple Adapters feature.

@drmcclelland

+1 - I would like to be able to log both to a file (debug level) and log to a database (error level)

@MisinformedDNA

@drmcclelland What you are asking for is already possible using a single factory.

@drmcclelland

@MisinformedDNA - I wasn't able to find a good example of this, do you know
of one? I would like to be able to set this up in a config file similar to
the way NLog allows multiple targets.
On Sep 25, 2014 4:06 PM, "Dan Friedman" notifications@github.com wrote:

@drmcclelland https://github.com/drmcclelland What you are asking for
is already possible using a single factory.


Reply to this email directly or view it on GitHub
#8 (comment)
.

@sbohlen
Member
sbohlen commented Oct 27, 2014

@ramonsmits were you eventually able to get some tests around this feature? If so, I'd like to review a PR since it sounds as if this feature would be of interest to many (at least on this thread)

@ramonsmits

Yeah I think it is valuable but its is lagging a lot of commits. I'll put some effort in it coming weekend to make it up to date.

@sbohlen
Member
sbohlen commented Mar 27, 2015

@ramonsmits

Just circling back on this to see if anything ever came of it (?) Since we never received a PR on it, I'm guessing not...do you have any interest in (eventually) pursuing this?

@sbohlen sbohlen added the new feature label Mar 27, 2015
@ramonsmits

I had difficulties in porting my changes to the current master head. I would like to contribute to this project but maybe its interesting in discussing my provious solution first. See if it requires some additional thought.

The primary solution was having a MultiLog class that implemented ILog. All log methods would iterate through an IEnumerable<ILog> firing these methods.

This worked and I started in calling each imlementation in a try..catch so that other loggers would be called even when there are errors but the problem here is that logging rules get very complex from then. This is the whole reason why logging frameworks exist.

So then I just removed the try..catch to make the logger responsible for that behavior.

If this implementation would suffice then I'll implement it like this on the current master branch.

@sbohlen
Member
sbohlen commented Mar 27, 2015

Thanks for letting us know the status.

My immediate thought is that while invoking each logger in the IEnumerable<ILog> collection inside its own try...catch block would be 'safer', its not clear to me that this would really be necessary per se.

As exists today in each of the various Common.Logging.XYZ adapters each individual call to e.g., logger.LogError(...) isn't protected (internally) in its own try...catch block, so its not clear to me that its a requirement that each individual invocation of the same within your MultiLog class would need to be either.

In other words, since the naked ILog methods are 'free to throw' unhandled exceptions today, I'm not certain that a collection of them would need to be protected in a try...catch block in your proposed scenario.

That said, I'm also interested in better understanding your comment re: how employing a try...catch block around this necessarily leads to significant complexity in your implementation. I'll admit that I've not actually gone as far as to prototype this myself, but why wouldn't this be as simple as something like the following:

public void LogWarn(...)
{
    foreach (var logger in _loggers)  //where '_loggers' is IEnumerable<ILog>
    {
        try
        {
            logger.LogWarn(...); 
        }
        catch ()
        {
            //swallow all exceptions so we safely move on to the next logger
        }
    }
}

Its probably the case that I am so over-simplifying this that I'm overlooking some hidden complexity you encountered when you attempted to implement this, but can you help me understand where my thinking is too naive here --?

@sbohlen
Member
sbohlen commented Apr 10, 2015

@ramonsmits:

FWIW, I took a look @ the work you've done in your own fork (https://github.com/ramonsmits/common-logging) and its more or less what I thought -- and is generally how I would probably have approached it as well.

Rather than my starting from scratch, if possible I'd like to try to incorporate your work into MASTER here as a starting point for this feature. Do you have any ETA you can offer in re: when you think you might be able to again try to sync your changes into a refresh of the present state of MASTER?

Alternately, if you don't think you're likely to be able to return to this any time soon, could you just issue a PR based on the present state of your FORK so that I can then merge your efforts into MASTER by 'manually' porting them over from your PR?

A quick glance of your changes seems to indicate that there would (likely) be little change to your code itself necessary, although the location of some of that code might need to change (i.e., into other assemblies now that there is Common.Logging.Core and Common.Logging.Portable to contend with). My initial guess is that this is (mostly) a refactoring exercise, although of course its also entirely possible it will prove more complicated than that if/when either of us attempt it :)

@abrasat
abrasat commented Apr 5, 2016

What is the actual status of this issue, does Common.Logging meanwhile support multiple factory adapters ?

@sbohlen
Member
sbohlen commented Apr 21, 2016

Sorry for the delay in this; in its current (released) state, multiple adapters are not presently supported.

I actually have a rudimentary version of this working locally, but ran into some challenges in the related changes to the way XML config parsing would need to work to support this. As mentioned elsewhere, I had some personal issues to address in the first half of 2016 that prevented me from focusing on this, but I'm returning now to focus on this project (and this issue) again.

Hopefully we'll have something in a release shortly -- at a min. I'm hoping to commit this support to MASTER in the next week or so at most.

@sbohlen
Member
sbohlen commented Apr 23, 2016 edited

OK, I've just merged my initial implementation of this feature and pushed it to MASTER in b130c7e.

I'd really appreciate it if anyone interested in testing it would pull the code, build, and experiment with it to validate not only that it works properly (I've of course already done some rudimentary testing of that) but also to let me know whether the implementation of the feature seems user-friendly from an adoption PoV.

For an example of how to configure this, see the App.Config that drives the initial rough unit test.

The salient points are thus:

  1. You need to register the MultiLoggerFactoryAdapter as the 'main' adapter for Common.Logging as seen here.
  2. Each of the 'actual' logger adapters you want to configure needs to be registered under a new <logging.multipleLoggers> section as demonstrated here. As you can see, the syntax for the registration of each individual 'actual' logger adapter is the same as you should already be familiar with when registering them as single loggers.
  3. In order to properly parse/handle the new <logging.multipleLoggers> section, you need to register an additional ConfigurationSectionHandler as shown here.

Let me know any feedback on this feature (pro or con) and I'll look to address it before I include it in the next release of Common.Logging.

Thanks in advance for any input!

@abrasat
abrasat commented May 19, 2016 edited

Definitely a very useful feature. I dont see any cons, it is a nice additional feature which does not affect any of the existing functionality.
For instance this is a possible usage:

`

        ConsoleOutLoggerFactoryAdapter consoleOutLogger = new ConsoleOutLoggerFactoryAdapter();
        consoleOutLogger.Level = LogLevel.All;
        consoleOutLogger.ShowLevel = true;
        consoleOutLogger.DateTimeFormat = "o";
        consoleOutLogger.ShowDateTime = true;
        consoleOutLogger.ShowLogName = true;

        var log4NetProperties = new Common.Logging.Configuration.NameValueCollection();
        log4NetProperties.Add("configType", "FILE");
        log4NetProperties.Add("configFile", "~/log4net.config");
        var log4NetCommonLoggingFactoryAdapter = new Log4NetLoggerFactoryAdapter(log4NetProperties);

        var loggerFactoryAdapter = new MultiLoggerFactoryAdapter(new List<ILoggerFactoryAdapter> {consoleOutLogger, log4NetCommonLoggingFactoryAdapter });
        Common.Logging.LogManager.Adapter = loggerFactoryAdapter;

`

Could you please upload the actual version containing the MultiLoggerFactoryAdapter as beta release on nuget.org ? When is planned the next official release ?

@sbohlen
Member
sbohlen commented May 23, 2016

I'd been hoping to delay the next release until after .NET Core went GA, but its now looking as if that's far enough out that delaying until formal/final support of .NET Core can be included is probably impractical. As such, I'll post a pre-release (probably a beta) of the present state shortly (probably tomorrow) including this new adapter, with an eye towards the next GA release of Common.Logging by the end of the month (May).

@sbohlen
Member
sbohlen commented May 23, 2016

This has now been pushed to NuGet as 3.3.2-Alpha1 (pre-release) to facilitate further testing/evaluation. Please let us know if any issues turn up in testing/review of this feature. Assuming no show-stoppers are identified, we'd expect to have this feature formally supported when 3.3.2 goes GA in the next few weeks.

Thanks!

@abrasat
abrasat commented Jul 14, 2016

Can you please check the links with the description of the configuration of multipleLoggers, as they seem invalid right now.

@sbohlen
Member
sbohlen commented Jul 14, 2016

@abrasat thanks for the heads-up; it should be all fixed now. A subsequent commit actually adjusted the path to the TEST project for the MultipleLogger, breaking the prior URLs. I'm pretty sure I've corrected each of the URLs in the comment now; let me know if you find any that I've missed.

Cheers!

@sbohlen
Member
sbohlen commented Oct 3, 2016

Closing as RESOLVED.

@sbohlen sbohlen closed this Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment