Marfusios / websocket-client

🔧 .NET/C# websocket client library
MIT License
682 stars 126 forks source link

Remove the logging magic #118

Open BasieP opened 2 years ago

BasieP commented 2 years ago

hi,

request here.. I cannot disable logging right now.. and I really don't want my application to log all the stuff the websocket client is logging. Libraries are not accually suppost to log stuff..

There are accually two reasons for that.

  1. the application that uses the library loses control over the content of the logfile
  2. you might want to do stuff (other then logging) when a specific thing occurs, and right now there is no way to do that.

Why not remove the magic called 'LibLog' and replace it by simple delegates? I can imagine two different approaches.

  1. create a couple of delegates (trace, debug, info, warn, error) and emit the 'debug' stuff trough the 'debug' delegate, etc.
  2. create functional delegates that log specific events. ('connected' for example).

Ofcourse is the first option the quickest, but most ugly. It solves problem 1 (flooding of logfile) but not problem 2, since you can't distinct between different types of errors.

So I would like to see functional delegates (or events) on which my application can subscribe (please not with reactive), and where my application can define actions, based upon the functional meaning.

And ofcourse, a lot of logging can simply be removed, because it looks a lot like it's only there for debug purposes.. like this:

    if (IsStarted)
            {
                Logger.Debug(L("Client already started, ignoring.."));
                return;
            }
zeroed-tech commented 1 year ago

I agree with the above. If anyone is using Serilog and needs a workaround for this in the mean time, this will filter out all logging events from WebsocketClient: Log.Logger = new LoggerConfiguration().Filter.ByExcluding(Matching.FromSource<WebsocketClient>())

Marfusios commented 1 year ago

Hey @BasieP ,

definitely a valid point. It is already planned and initially handled by #50, but it is a breaking change, so I haven't yet managed to finish it.

I think we don't need to reimplement the wheel, abstraction Microsoft.Extensions.Logging from MS seems very sufficient and already supported by most of the OS logging libraries.

You can disable logging, usually by lowering the log level, Serilog example in appsettings.json:

{
  "Serilog": {
    "MinimumLevel": {
      "Default": "Debug",
      "Override": {
        "Websocket.Client": "Error"
      }
    }
  }
}
SrBrahma commented 1 year ago

@Marfusios I think no logging is better than migrating the logging.

jbriggs22 commented 1 year ago

@Marfusios are there any plans to get that PR to use Microsoft.Extensions.Logging in new version? or is there a more active fork of this project?

I would also like an option with no logging or MS logging instead, would try it out if there is a preview version available on NuGet.