Move to Microsoft.Extensions.Logging #899
Conversation
|
While I personally would love to do this, I prefer to use an extension library implementation, |
|
If taking on an external dependency is a hard no, then that leaves two options, keep the current logging as-is, or implement an extension package QF.extensions.logging as you suggest. The downside with making a "QF.Extensions.Logging" package that eventually hooks into ms.extensions is that it would require improving the current QF logging abstraction to produce all the data that a user of ms.extensions.logging might require, at which point you're rewriting ms.extensions.logging, without getting 100% there in terms of performance and ergonomics. The problem with maintaining the status quo is that users are asking for better logging, and the current system isn't meeting their needs. I'm partial to just going with ms.extensions.logging, and the .net world has coalesced around it, so I'd be comfortable with that choice. But even an extension package would be an improvement over what we have now. |
Yes, either way, it's better than the status quo. |
|
I'm starting to digest this PR this week. I have to admit that I am not familiar with Microsoft.Extensions.Logging. (Can I call it MEL going forward?) My work with .NET is very FIX-centric, so somehow I'm able to stay oblivious to certain aspects of the greater .NET ecosystem. I need to download this branch and it and see how it works. My first impulse is "aaughhh! So much change!" Which is of course not a legitimate criticism, and also betrays my ignorance of what this extension can offer. Let me start with one dumb question, however: Why not just write an QF ILog implementation that defers all its functionality to MEL? |
I'll try my best to help. The happy path of MEL is that library authors, write logs using an ILogger from MEL. The concrete implementation of ILogger will be provided by the library consumer/application developer. The benefit of this is that on the consumer side, there are purpose-built logging libraries the plug straight into MEL to do all you'd ever want to do with logs, like log to a Db, log to a file, log to a rolling file etc. The thinking is a quickfix developer wants to spend their time building quickfix, not become an expert in logging.
A QF ILog that writes into MEL is easy. In fact, in this PR, there's an adapter to pipe logs from existing ILog and ILogFactory into MEL so as not to break folks. But that's just piping plain text logs, while MEL can do so much more (e.g. structured logging/scopes/high performance logging). A QF ILog that can do the extras that MEL can do is a bit more work (I'm not an expert so i can't say exactly how much) and ends up looking a whole lot like MEL. |
QuickFIXn/Session.cs
Outdated
| /// <returns></returns> | ||
| public bool Send(string message) | ||
| public bool Send(string message, string messageType) |
There was a problem hiding this comment.
Wasn't supposed to be - it's been reverted. What do you think of what it is now together with #909?
|
Please verify that the levels at which stuff is logged makes sense. I went with Information for the inbound/outbound messages, and error/debug/trace for the rest |
|
I am seeing quite a lot of test failures locally which I don't seem to get on master, e.g.: |
|
I can't reproduce this using |
|
@jkulubya Are you running on windows? This looks like an error that would not occur on Linux/Mac, but would occur on Windows. (I ran into this myself with some recent PRs) |
Should fix System.IO.IOExceptions related to locked files on windows
|
@gbirchmeier I'm on linux. I presume you fixed the issues in #904? It was a helpful pointer. @Rob-Hague do you mind trying again? I was missing a few |
@jkulubya I had help, but yes, that was it |
|
It is improved in the last commits, I am just getting 3 failures instead of ~300. Example: I am on Windows 11. Could be worth adding a Windows test run to Github Actions? |
|
If the 3 remaining failing tests are "DifferentPortForAcceptorTest", "DynamicAcceptor" and "TestRecreation", then they should be fixed now. And I agree with the windows runner suggestion. |
There was a problem hiding this comment.
Yep runs green for me now. Was not aware about the different disposal behaviour with the constructors.
I think this concludes my feedback for now. I will just point out that e.g. ScreenLogFactory and ScreenLogProvider could probably be merged in a similar way to ScreenLog and ScreenLogger earlier on
|
FYI, I'm planning to put out 1.13 sometime in January, but not with this feature in it yet. After the holiday I'll get more involved with this PR, with the plan to put it in 1.14 for release probably in the summer. |
|
Ok I said I was done but I have had a thought about a change to the design. Instead of adapting the existing log types to fit as MEL providers (and obsoleting them), the idea would be to allow them to exist separately: // A bridge between MEL and QF logging.
// This is what would be passed around the library instead of MEL ILoggerFactory.
// It would avoid the potentially awkward session lookup in the current Provider&LogFactoryAdaptor impls.
public interface IQFLoggerFactory
{
MEL.ILogger CreateLogger(SessionID sessionId);
MEL.ILogger CreateNonSessionLog();
}
internal class LoggerFactoryAdaptor(ILoggerFactory factory) : IQFLoggerFactory
{
MEL.ILogger CreateLogger(SessionID sessionId)
=> factory.CreateLogger(FileLog.Prefix(sessionId)); // or whatever string derived from sessionId
MEL.ILogger CreateNonSessionLog()
=> factory.CreateLogger("Non.session.log"); // or whatever string
}
internal class LogFactoryAdaptor(ILogFactory factory) : IQFLoggerFactory
{
MEL.ILogger CreateLogger(SessionID sessionId)
=> new LogAdaptor(logFactory.Create(sessionId)); // LogAdaptor as in this PR
MEL.ILogger CreateNonSessionLog()
=> new LogAdaptor(logFactory.CreateNonSessionLog());
}then the Provider types added in this PR would be removed and we can ignore anything to do with providers; the existing Log types would not need to be changed and could be un-obsoleted. One downside of this is that it would be nice to decorate the non-session logs with the class name as is common with MEL e.g. using public interface IQFLoggerFactory
{
MEL.ILogger CreateLogger(SessionID sessionId);
MEL.ILogger CreateLogger(Type type);
}
internal class LoggerFactoryAdaptor(ILoggerFactory factory) : IQFLoggerFactory
{
MEL.ILogger CreateLogger(SessionID sessionId)
=> factory.CreateLogger(FileLog.Prefix(sessionId)); // or whatever string derived from sessionId
MEL.ILogger CreateLogger(Type type)
=> factory.CreateLogger(type);
}
internal class LogFactoryAdaptor(ILogFactory factory) : IQFLoggerFactory
{
private LogAdaptor? _nonSessionLog; // just uses one instance
MEL.ILogger CreateLogger(SessionID sessionId)
=> new LogAdaptor(logFactory.Create(sessionId)); // LogAdaptor as in this PR
MEL.ILogger CreateLogger(Type type)
=> _nonSessionLog ??= new LogAdaptor(logFactory.CreateNonSessionLog());
}Hope it makes some sense, we can discuss it at a later date. Happy holidays! |
I like this but I think regardless of where we end up with the rest, we should still obsolete the logging functionality that currently exists. We can leave that to serilog and the rest. The next question to answer is what should the logs be categorised as. My suggestion is session logs are labelled "QuickFix.SessionLogs." and non-session logs labelled "QuickFix.NonSessionLogs" (or similar), regardless of source, consumers can direct filter by session ID, or whether session log/non session log. I'm not attached to the name but we need to decide and go for it. |
If it were to use |
Don't use string parsing to decide whether to return session/non session log Remove iloggerproviders that were added before
|
In the latest update, session logs will be tagged QuickFix.SessionLogs., and non session logs will take the name of the class that generated them, e.g QuickFix.ThreadedSocketAcceptor. The two main entry points (ThreadedSocketAcceptor & SocketInitiator) accept either a QuickFix.Logger.ILogFactory (obsolete) or an MEL.ILoggerFactory. |
|
I've updated the sample serilog project to log to console and rolling files (by the minute) and I get: Console logsand Log files |
Hi,
We'd like to 1. pipe the QF logs through our existing log pipeline based on MS.Extensions.Logging, and 2. eventually add more context to the structured logs that will come out of QF when using MS Logging. It doesn't seem like these two issues (#205, #679) were resolved so here's a PR to get that moving along.
There's a new dependency on Microsoft.extensions.logging.abstractions, and all the logging is happening via an ILogger. When messages are logged, they're logged with the message type in the scope (where possible), to allow heartbeats and such to be filtered out downstream.
Instead of passing in an ILogFactory, end-users should now pass an ILoggerFactory. Exisiting ILogFactory's and ILog's should work, via an adapter, but all that has been marked obsolete with a note telling the user to migrate to MS Logging.
There are a few questions to resolve, like what levels things should be logged at, what event IDs should be used for logs, what category names should be used.