rabbitmq / rabbitmq-dotnet-client

RabbitMQ .NET client for .NET Standard 2.0+ and .NET 4.6.2+
https://www.rabbitmq.com/dotnet.html
Other
2.07k stars 576 forks source link

Investigate existing commonly used logging libraries for logging #357

Closed michaelklishin closed 4 years ago

michaelklishin commented 6 years ago

Not sure how common is it to use Microsoft.Extensions.Logging outside of ASP.NET projects but we should at least investigate it as an option for logging.

houseofcat commented 6 years ago

I may be able to assist with this - if my timeline opens up.

michaelklishin commented 6 years ago

@thyams thank you!

pardahlman commented 6 years ago

Hello @michaelklishin - thanks for opening the discussion on logging 👍

Are you suggesting adding Microsoft.Extensions.Logging as a dependency for RabbitMQ.Client? For what it's worth, my observation is the same as yours: that abstraction is mainly used in ASP.NET projects. Using Microsoft's logger framework will force users to use and configure a LoggerFactory, possibly add another dependency to a framework adapter (like Serilog.Extensions.Logging) etc. That is quite big impact made by a library. Also, the latest version of Microsoft.Extensions.Logging only target .NET Standard 2.0 which maps to .NET Framework 4.6.1. You could have some conditional includes in the csproj file to make it work for earlier versions, but that is some extra work.

Personally, I think it is preferable with something like LibLog (tag line: LibLog is a single file for you to either copy/paste or install via nuget, into your library/framework/application to provide a logging abstraction.). I have some experience with LibLog and can help out in some capacity if desired!

Lastly, if for some reason the Microsoft logging lib will be used, the client should only depend on the abstraction (Microsoft.Extensions.Logging.Abstractions).

Interested in hearing your thoughts!

adamralph commented 6 years ago

👍 for using LibLog, or some other internal solution, rather than taking a package dependency (welcome to DLL hell).

michaelklishin commented 6 years ago

@pardahlman not necessarily Microsoft.Extensoins.Logging but something widely used. It's pretty surprising that there is no SLF4J alternative (in terms of how widely it is adopted) in the .NET world, so it's not obvious what interface should be targeted by this client.

adamralph commented 6 years ago

I'm 👎 to taking a runtime dependency on any package for logging. It won't be long before some other package takes a dependency on an incompatible version and the dependency tree for an app will blow up.

pardahlman commented 6 years ago

@michaelklishin there has been attempts on unified logging abstraction in .NET, Common Logging was used a couple of years ago etc. I, however, believe that any such abstraction will be the least common denominator across all logging frameworks. That, in turn, may have a negative effect on innovation in logger frameworks. Aaaanyhow, that's just my opinion.

There is a great post about logging in .NET libraries written by @nblumhardt Good citizenship - logging from .NET libraries that I think is worth a read. LibLog is a good fit, especially for number 2 and 3 from the post.

adamralph commented 6 years ago

For more perspective, my opposition to taking a dependency on a logging abstraction is based on my experience as a Common.Logging veteran. Thinking back to the mess I got myself into by taking a dependency on that package makes me shudder. It was DLL-hell in all it's glory, and I encountered the problem in various projects. It never worked out well. Again, I strongly recommend not taking a dependency on a logging abstraction (Common.Logging or anything other package) if it can be avoided.

lukebakken commented 6 years ago

I agree with the points raised in the article provided by @pardahlman (thanks!) and @adamralph 's suggestion to not use a logging abstraction. I would prefer that return values and exceptions provide all the necessary information for a user of this library to decide what to log or not.

Internally, if we'd like to have more visibility into what is happening, we can use the Debug and Trace classes.

houseofcat commented 6 years ago

I have to agree with everyone's finer points about using DI.

kjnilsson commented 6 years ago

There is already a logging abstraction in this client using EventSource from System.Diagnostics for example, here.

kjnilsson commented 6 years ago

Custom EventListeners can be written to receive the internal log events: https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/91f368259cbbeec1b924c0edf1a5482893e84150/projects/client/RabbitMQ.Client/src/logging/RabbitMqConsoleEventListener.cs

paulomorgado commented 6 years ago

I'm a bit late for this, but I'd like to say that I'm using Microsoft Logging Extensions in everything I'm doing and nothing is ASP.NET Core. I'm using it because I consider it .NET infrastructure like System.Diagnostics tracing used to be (is) for the .NET Framework.

I'm using Serilog as a provider (others are using NLog or Log4Net) and it's a good way to share components inside the company.

I wouldn't take a dependency on any other library other than a Microsoft provided one.

rwkarg commented 5 years ago

I'm in the same boat as @paulomorgado We use M.E.L for our logging abstractions in all of our shared libraries. Hosted programs then add logging implementations they desire (Serilog, Syslog, etc.)

TrymBeast commented 5 years ago

My suggestion (as already mentioned by @adamralph and @pardahlman) is liblog. Because it doesn't force anyone to use a specific logging library, it has built-in automatic integration with logging libraries like serilog, log4net and others and you don't take it as a dependency, instead it injects code into your project. I'm starting to use this in my company, and I am happy with this solution. Take a read to the following article: https://nblumhardt.com/2016/04/which-logging-abstraction-should-i-use/

paulomorgado commented 5 years ago

@TrymBeast, that article was written before the release of Microsoft.Extensions.Logging 1.0.0.

But maybe the best option would be event sourcing with a companion library for logging,

TrymBeast commented 5 years ago

@paulomorgado True, but Microsoft. Extensions.Logging is just another logging abstraction library, I don't see anything new there. I believe liblog is a better option for a library because it doesn't tie the client application with anything and there is no versioning hell as well. That's just my opinion.

paulomorgado commented 5 years ago

@TrymBeast Microsoft.Extensions.Logging.Abstractions is a set of abstractions tied to no particular implementation. Microsoft.Extensions.Logging is Microsoft's implementation with several providers and supported by the most used logging frameworks,

But, again, at this point, for something like RabbitMQ, I prefer event sourcing. It's already in the library (https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/master/projects/client/RabbitMQ.Client/src/logging/RabbitMqClientEventSource.cs), but just barely used.

TrymBeast commented 5 years ago

@paulomorgado I know that, what I meant is that Microsoft.Extensions.Logging.Abstractions doesn't sove the problem differently than any other loggin abstraction library like Common.Logging for example.

What I don't like about event sourcing is that it is not cross platform. From what I have seen, the Mono implementation is just empty, doesn't log anything anywhere (https://github.com/mono/mono/blob/master/mcs/class/corlib/System.Diagnostics.Tracing/EventSource.cs).

Matthew-Davey commented 4 years ago

@TrymBeast the current event sourcing approach does work just fine on Linux running under dotnet core. At least, the following code produces the expected output...

new RabbitMQ.Client.Logging.RabbitMqConsoleEventListener()
    .EnableEvents(RabbitMQ.Client.Logging.RabbitMqClientEventSource.Log, System.Diagnostics.Tracing.EventLevel.Verbose);

RabbitMQ.Client.Logging.RabbitMqClientEventSource.Log.Warn("Test message");

The bigger problem imo is the vanishingly small number of log messages that are actually written using this method. A quick search on 'ESLog' reveals that, in any case, you're really only ever going to get three or four useful log messages. Two when the connection is auto recovering/ auto-recovered, and one when the connection shuts down due to an error or missed heartbeats. At all other times, radio silence.

Discussing which logging abstraction to use, if any, seems rather moot until the number and quality of log messages is increased.

Would it be worthwhile opening another issue for this purpose? I'd be prepared to send a few pull requests...

lukebakken commented 4 years ago

@Matthew-Davey please feel free to submit a pull request to improve the quality of the logging in this library. I'm working on shipping version 6.0 "soon" so contributions are very welcome and will receive my attention.

I am closing this issue with the understanding that the current approach of event sourcing (code) is what we will continue to use.

nblumhardt commented 2 months ago

v7 RC of this library is already on NuGet and instrumented using System.Diagnostics.Activity, which is a great (modern) fit for what's expected from a component like this one. Pretty confident that the rabbitmq-dotnet-client folks have this under control.