SSH.NET
SSH.NET copied to clipboard
Replace DiagnosticAbstration with Microsoft.Extensions.Logging.Abstractions
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 .Logging
and Console
packages 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).