Add ability to re-use single RabbitMQ Connection #307
Conversation
6dde9c7 to
4617e02
Compare
* Addresses Issue-304 * Add many configuration options that support ability to re-use a single connection with multiple multiplexed channels. * Add documentation around this health check to help others follow RabbitMQ best practices
| /// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param> | ||
| /// <param name="timeout">An optional System.TimeSpan representing the timeout of the check.</param> | ||
| /// <returns>The <see cref="IHealthChecksBuilder"/>.</returns></param> | ||
| public static IHealthChecksBuilder AddRabbitMQ(this IHealthChecksBuilder builder, string name = default, |
There was a problem hiding this comment.
One problem here is that there is ambiguity between the existing method's signature and this one if you are only specifying a single string. I can either rename this method or consolidate into a single method where it has different behavior depending on what fields are provided to prevent this ambiguity.
There was a problem hiding this comment.
We could resolve the ambiguity by re-ordering the parameters here, but that feels a little cheaty:
public static IHealthChecksBuilder AddRabbitMQ(this IHealthChecksBuilder builder, HealthStatus? failureStatus = default, string name = default, I think with that ordering a call like AddRabbitMQ("amqps://broker") would resolve to the overload on L27, and if we wanted to call this overload we'd need something like AddRabbitMQ(name: "broker")
There was a problem hiding this comment.
The thought I had today was to add a bool indicating whether to give priority to IConnectionFactory instead of IConnection. If I make it optional. I introduce a possibly useful feature and I maintain backwards compatibility.
There was a problem hiding this comment.
that'd do it; what would that parameter be called? preferIConnectionFactoryFromServices ?
the code might get a little messy to make this work as a priority; how about making it a little narrower, something like: bool useIConnectionFactoryFromServices=false. If it's true, we only look for an IConnectionFactory, if it's false, we only look for an IConnection?
| { | ||
| return new RabbitMQHealthCheck(connection); | ||
| } | ||
| else if(connectionFactory != null) |
| } | ||
| ``` | ||
|
|
||
| ### Without Conveniece Extension Method |
| AutomaticRecoveryEnabled = true | ||
| }; | ||
|
|
||
| var connection = factory.CreateConnection(); |
There was a problem hiding this comment.
I know this is only some sample code, but people frequently copy/paste these things and assume the code is fit for production. In this case, I don't think we want to be creating a real connection in ConfigureServices. If our amqpuri doesn't work, we want our health check to fail, and as written I think the app will fail to start.
I think we can keep this connection lazy without too much hassle using another AddSingleton overload:
.AddSingleton<IConnection>(_ => factory.CreateConnection())|
Hi @jnovick I will try to review this PR today afternoon! |
|
Hi @jnovick After CI build I will create a new tag and when the release build finish a new package will be created with version (3.0.1) P.S: I change also version for us 3.0 is the .NET Core Version, and next is our changes, is not semver! |
|
Release build finished, 3.0.1 package for Rabbitmq is on Nuget |
connection with multiple multiplexed channels.
RabbitMQ best practices