SSH.NET icon indicating copy to clipboard operation
SSH.NET copied to clipboard

Replace DiagnosticAbstration with Microsoft.Extensions.Logging.Abstractions

Open mus65 opened this issue 4 months ago • 1 comments

This replaces the DiagnosticAbstraction with Microsoft.Extensions.Logging.Abstractions and allows consuming applications to pass their own ILoggerFactory to SSH.NET.

I don't expect this to be merged as is since there are a few things worth discussing:

Why do this?

Because it is the de-facto standard for modern and structured logging in .NET and allows consuming applications to easily integrate SSH.NET into their own logging. This also means users can more easily provide log messages in issues.

Also the currently used Trace API is pretty much considered obsolete.

Injecting ILoggerFactory vs static instance

The imho best way to implement this would be to add an ILoggerFactory to the public constructors. The problem is that passing this through all the constructors and down to all the layers is quite a lot of work. I actually tried this and finally gave up when I realized that the logging in KeyExchange.cs meant that the ConnectionInfo constructors would also need an ILoggerFactory (because of the KeyExchangeAlgorithms dictionary).

So I opted for a static configuration class, similiar how Npgsql handles this. Imho this is "good enough" since having multiple ILoggerFactory instances in an application sounds like quite a niche use case.

Applications simply have to call SshNetLoggingConfiguration.InitializeLogging(loggerFactory); before using SSH.NET.

Compile-time logging source generation

While this implementation already allocates less than the old one (because it doesn't create the full string unless logging is enabled), things could be further improved with the LoggerMessage attribute. This would mean even better performance, but it also means writing a method for every single log message, which can be quite tedious. I would be fine with doing this, but decided to not bother unless it is decided to use it.

There are also quite a few unneccessary allocation because of the usage of Session.ToHex(). I didn't change this here since it could be optimized separately (even without this PR being merged).

New dependency

This obviously adds a new dependency to Microsoft.Extensions.Logging.Abstractions to SSH.NET (the .Loggingand Consolepackages are only for the tests) . Imho this is not really an issue. This library is as minimal as it gets and most applications already pull this is in anyway, either directly or by another dependency needing it (ASP.NET Core requires it for example).

mus65 avatar Sep 28 '24 15:09 mus65