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

Replace DiagnosticAbstration with Microsoft.Extensions.Logging.Abstractions #1509

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

mus65
Copy link
Contributor

@mus65 mus65 commented Sep 28, 2024

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).

@Rob-Hague
Copy link
Collaborator

I think it's the right way to go. The static init seems reasonable given the prior art in other libraries. I don't think the source generator is necessary (definitely tedious). Wrt the new dependency, we can always merge after the next release (hopefully soon) so there is not a big wave of dependencies in one release

@mus65
Copy link
Contributor Author

mus65 commented Oct 12, 2024

I just realized that this PR actually introduces new ToHex() allocations because in release mode this was never called before because of the [Conditional("DEBUG")] attribute on DiagnosticAbstraction.Log.

So I optimized this to only call ToHex() once instead of every log call. This could be even more improved by using a Lazy and checking if (logger.IsEnabled()) before every message, but I'm not sure this is worth it.

@mus65 mus65 marked this pull request as ready for review November 13, 2024 17:09
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 this pull request may close these issues.

2 participants