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 log4net with Microsoft.Extensions.Logging.Abstractions #103

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

ltrzesniewski
Copy link
Member

@ltrzesniewski ltrzesniewski commented Jul 13, 2021

This replaces log4net (which is dormant) with the Microsoft.Extensions.Logging.Abstractions package.

You now have to set the ZebusLogManager.LoggerFactory static property in order to get logs from Zebus. Further changes to this property will be picked up (which is how you can notify BusMessageLogger that the underlying logging configuration changed).

Additional notes:

  • I could have implemented the log4net interface on top of ILogger using extension methods, but using the real API seemed cleaner.
  • I didn't use the string interpolation feature of the package, but used interpolated strings ($"...") instead in order to avoid additional string parsing and impacting the formatter cache.
  • A simple log factory on top of log4net is provided in the demo directory runner.
  • Maybe we should add a separate ZebusLogManager.NotifyConfigurationChanged() method in order to notify BusMessageLogger to reload its cache?

Copy link
Member

@ocoanet ocoanet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR. I like the fact that you replaced string formats by interpolated strings.

nitpick: The Log4NetConfigurator types were helpful to understand complex unit tests and they will be missed. Do you really think they should be removed?


namespace Abc.Zebus.Directory.Runner
{
public sealed class Log4NetFactory : ILoggerFactory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You had to implement this? There is no built-in Log4NetLoggerFactory?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log4net doesn't reference Microsoft.Extensions.Logging.Abstractions, so there's nothing built-in

src/Abc.Zebus/Core/BusMessageLogger.cs Show resolved Hide resolved
{
private static ILoggerFactory _loggerFactory = NullLoggerFactory.Instance;

public static ILoggerFactory LoggerFactory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Why did you choose to expose ILoggerFactory and not ILoggerProvider?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ILoggerFactory seemed to be the standard approach, and it's conceptually more high-level.

src/Abc.Zebus/ZebusLogManager.cs Outdated Show resolved Hide resolved
src/Abc.Zebus/ZebusLogManager.cs Outdated Show resolved Hide resolved
@alexandrekow
Copy link
Member

@ltrzesniewski
I think it would be a nice addition to use Log message template instead of interpolated string.
It will make the way for structured logging.

@ltrzesniewski
Copy link
Member Author

I mentioned this in the issue:

I didn't use the string interpolation feature of the package, but used interpolated strings ($"...") instead in order to avoid additional string parsing and impacting the formatter cache.

The implementation in the package adds some overhead since it parses the string in order to convert it to an argument to String.Format, and caches the resulting formatter. If the cache size reaches a max value, then the string gets parsed every time... I wanted to avoid that.
Also, the structured logging approach we started using in our codebase is different. We may revisit this in the future if we want to extend it.

# Conflicts:
#	src/Abc.Zebus.Persistence.Cassandra/Cql/CqlStorage.cs
#	src/Abc.Zebus.Persistence.Cassandra/Cql/PeerStateRepository.cs
#	src/Abc.Zebus.Persistence.Runner/Abc.Zebus.Persistence.Runner.csproj
#	src/Abc.Zebus.Persistence/Handlers/MessageHandledHandler.cs
#	src/Abc.Zebus.Persistence/Handlers/PersistMessageCommandHandler.cs
#	src/Abc.Zebus/Directory/PeerDirectoryClient.cs
@ltrzesniewski ltrzesniewski merged commit 3c9e69a into master Apr 12, 2022
@ltrzesniewski ltrzesniewski deleted the logs branch April 12, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants