search

cyanfish / grpc-dotnet-namedpipes

Named pipe transport for gRPC in C#/.NET
Apache License 2.0
190 stars 48 forks source link

Handling of open logging todos #8

Closed taori closed 1 year ago

taori commented 4 years ago

There are a couple of occurences in the code which have todo comments regarding logging handling.

Perhaps it would be best if the structure is changed a bit so that there is a client/server settings class which can pass on properties like bool ThrowsException and Action<Exception> Log

However upon initial investigation i noticed that there are some classes in the Internal folder which are not actually internal. Perhaps it would be best to have a proper architectural cleanup and fix issues like this together with it?

cyanfish commented 4 years ago

I think those classes can just be made internal with no issue.

As far as logging, my idea was to have an ILogger property on the options classes.

taori commented 4 years ago

@cyanfish While i like that logging interface it would add a dependency on this library which isn't actually needed for it to run. Hence the Action<Exception>. Many other libraries take similar approaches to reduce dependencies

cyanfish commented 4 years ago

I'd prefer to have easier compatibility. Microsoft.Extensions.Logging is commonly used (e.g. in grpc-dotnet) and has adapters for pretty much every logging framework. Plus it's easy to use extra functionality (e.g. different log levels) if that's ever needed.

taori commented 4 years ago

Yes, i understand what you are trying to do. But a library for communication forcing a logging framework down your throat is a big no no in library development. Check nuget for big libraries - you're not going to see any of them adding a dependency for logging.

cyanfish commented 4 years ago

Well, as far as big libraries, there's Grpc.Net.Client :). Specifically the package isn't a logging framework but an abstraction that can be used with any framework.

It's certainly a fair position to not want to depend on it; but for this project, if logging is added, I would like to stay in line with existing gRPC projects.

taori commented 4 years ago

It's certainly a fair position to not want to depend on it; but for this project, if logging is added, I would like to stay in line with existing gRPC projects.

That makes perfect sense - one way or another better than the current state would already be throwing the exception and adding documentation tags to indicate that a class/method might throw an exception.

cyanfish commented 4 years ago

Yeah, actually some kind of Error event on the server/channel objects would probably do the trick.

cyanfish commented 1 year ago

I added such an event in 2.0.0.