connamara / quickfixn

QuickFIX/n implements the FIX protocol on .NET.
http://quickfixn.org
Other
463 stars 552 forks source link

Introduce Commons.Logging as a logging framework #205

Open roji opened 11 years ago

roji commented 11 years ago

Continued discussion from #197.

roji commented 11 years ago

@gbirchmeier: implementing a CommonLoggerLogFactory is possible and is definitely least-effort, but this prevents us from benefiting from some of the important features of the full-blown logging frameworks, since the QuickFix ILog methods only take a simple string:

So the problem is at the source... If you keep using ILog you're always restricted to a simple string message, and even if you pass that along to a logging framework (as I do now) you're still left with only a string... Whereas if we actually transition to something like Commons.Logging inside QuickFix code many other things become possible.

The downside is of course that ILog would have to die, although we can provide a transitional Commons.Logging to ILog - instead of forwarding ILog to Commons.Logging maybe we can forward Commons.Logging to ILog for users who require the backwards compatibility (needs to be checked).

Let me know what you think...

roji commented 11 years ago

By the way, pull requests such as #132, #180, #170 are obviated by this kind of transition: any logging framework does file rolling as a feature, composite logging is an inherent configuration thing, etc...

gbirchmeier commented 11 years ago

For this change, then, would you need to go through all log statements in the engine and replace with trace/debug/info/warn/error?

Would users still be able to do a NullLogger or ScreenLogger, or perhaps implement their own loggers (like, I dunno, log to db or to a message queue)?

We still absolutely need to allow creation of message logs as they currently are. Ideally, if we switch to Common.Logging, I'd like to provide a default configuration that will give message/event logs as they currently are (and I think 90% of users would just use that).

roji commented 11 years ago
  1. That's correct, this would require modifying all log statements in the engine.
  2. Users would be able to configure any type of logger using their own chosen logging package (e.g. NLog, log4net...). These packages typically provide a very wide array of options, including null and screen. Here's the list of "targets" supported by NLog, which is the one I use: https://github.com/nlog/NLog/wiki/Targets. You can of course also write your own targets.
  3. You're right about backwards compatibility. One option here is to treat the event log and the message log differently; I definitely agree that the message log must be backwards compatible, but do you feel as strongly about the event log? If not, we could move the event log to Commons.Logging and leave the message log as it is.

The other solution is to write a very thin wrapper, which would forward logs to Commons.Logging and to the current QuickFix ILog (if it is configured). This would maintain 100% backwards compatibility, and we could mark the current ILog as obsolete and recommend transitioning to one of the standard logging packages.

roji commented 11 years ago

Note, by the way, that NLog has logging targets for databases and message queues out of the box... Another advantage of moving to a standard logging package...

gbirchmeier commented 11 years ago

I don't feel strongly about the event log. Obviously, I'd like it to be recognizable to people familiar with the old form, but it doesn't have to be exactly the same.

It's good to hear that NLog has support for DBs and queues. I doubt many people will need it, but eventually there will be some guy who really needs it.

I think you know what you're doing, and I think you should give it a shot. Can you branch it off #197? I think you should start it up and we'll figure out the backward-compatibility angle later.

roji commented 11 years ago

Will do. Once I have something working I'll post it here.

ligu commented 11 years ago

@roji @gbirchmeier the plan fo removing ILog and implementing Common.Logging is a good idea, especially with default settings identical to current logging operations...

abbeycode commented 2 years ago

Looks like #679 is the current iteration of this request, if anyone else lands on this abandoned issue.