Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Commons.Logging as a logging framework #205

Closed
roji opened this issue May 30, 2013 · 9 comments
Closed

Introduce Commons.Logging as a logging framework #205

roji opened this issue May 30, 2013 · 9 comments

Comments

@roji
Copy link

roji commented May 30, 2013

Continued discussion from #197.

@roji
Copy link
Author

roji commented May 30, 2013

@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:

  • We lose the distinction between priorities; logging interface extension for debug logging #170 adds "debug", but logging frameworks in general allow for more levels (trace, debug, info, warn, error).
  • It's hard to log exceptions; typically the exception message is passed to QuickFix's ILog, but the other details are lost (stack trace, inner exceptions...)
  • We lose the idea of loggers, which allow filtering of logging message by category.

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
Copy link
Author

roji commented May 30, 2013

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
Copy link
Member

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
Copy link
Author

roji commented May 30, 2013

  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
Copy link
Author

roji commented May 30, 2013

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
Copy link
Member

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
Copy link
Author

roji commented Jun 4, 2013

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

@ligu
Copy link
Contributor

ligu commented Jun 7, 2013

@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
Copy link

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

@gbirchmeier gbirchmeier closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants