icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
101 stars 13 forks source link

Client factory decorators for logging and metrics #2561

Open bentoi opened 1 year ago

bentoi commented 1 year ago

From a DI perspective, I think it would be much cleaner to replace the ILogger? logger client connection or connection cache arguments with an IClientProtocolConnectionFactory argument. We'd support a logger and a metrics factory decorator and a default factory that provides both logging and metrics.

Our DI extension, could create these decorators if a logger is registered or if services.AddMetrics() is called.

We could consider doing the same for Server if we add IProtocolConnectionListenerFactory.

bernardnormier commented 1 year ago

We're talking about constructor parameters for ClientConnection and ConnectionCache here.

I find ILogger? much more user-friendly and expected than IClientProtocolConnectionFactory.

Removing ILogger? would also prevent us from using the logger for logging some errors. See #2351.

bentoi commented 1 year ago

We can keep an ILogger convenience constructor that sets up a decorated client connection factory but I think we should provide the option to let the application provide an IClientProtocolConnectionFactory.

It's the DI way, it allows the application to implement custom logging/metrics, it allows to disable our metrics/logger decorators.