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

Client factory decorators for logging and metrics #2561

Open
bentoi opened this issue Jan 19, 2023 · 2 comments
Open

Client factory decorators for logging and metrics #2561

bentoi opened this issue Jan 19, 2023 · 2 comments
Labels
proposal Proposal for a new feature or significant update
Milestone

Comments

@bentoi
Copy link
Contributor

bentoi commented Jan 19, 2023

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.

@bentoi bentoi added the proposal Proposal for a new feature or significant update label Jan 19, 2023
@bentoi bentoi added this to the 0.2 milestone Jan 19, 2023
@bernardnormier
Copy link
Member

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
Copy link
Contributor Author

bentoi commented Jan 20, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal for a new feature or significant update
Projects
None yet
Development

No branches or pull requests

2 participants