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

Introduce async factories to Redis health check. #2305

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Introduce async factories to Redis health check.
  • Loading branch information
mitchdenny committed Oct 14, 2024
commit f5fa818aea5916d160673396d09789e55713016d
2 changes: 1 addition & 1 deletion Directory.Build.props
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@
<Deterministic>true</Deterministic>
<DebugType>embedded</DebugType>
<EmbedUntrackedSources>true</EmbedUntrackedSources>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<TreatWarningsAsErrors>false</TreatWarningsAsErrors>
mitchdenny marked this conversation as resolved.
Show resolved Hide resolved
<Nullable>enable</Nullable>
<NoWarn>$(NoWarn);1591;IDISP013;AD0001;</NoWarn> <!--TODO: temporary solution, remove AD0001 after https://github.com/dotnet/aspnetcore/issues/50836 fixed-->
<SuppressTfmSupportBuildWarnings>true</SuppressTfmSupportBuildWarnings>
Original file line number Diff line number Diff line change
@@ -66,6 +66,37 @@ public static IHealthChecksBuilder AddRedis(
timeout));
}

/// <summary>
/// Add a health check for Redis services.
/// </summary>
/// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
/// <param name="connectionStringFactory">A factory to build the Redis connection string to use.</param>
/// <param name="name">The health check name. Optional. If <c>null</c> the type name 'redis' will be used for the name.</param>
/// <param name="failureStatus">
/// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
/// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
/// </param>
/// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
/// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
/// <returns>The specified <paramref name="builder"/>.</returns>
public static IHealthChecksBuilder AddRedis(
this IHealthChecksBuilder builder,
Func<IServiceProvider, CancellationToken, Task<string?>> connectionStringFactory,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overload feels weird to me. It is too specific to the Aspire use case.

If you need to do something async, I think you should create the IConnectionMultiplexer asynchronously. Getting the connection string asynchronously isn't very common.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if delayed acquisition of connection strings is Aspire specific. It certainly impacts Aspire, but there are plenty of times that the connection string is not immediately available (particularly in multi-tenant scenarios).

string? name = default,
HealthStatus? failureStatus = default,
IEnumerable<string>? tags = default,
TimeSpan? timeout = default)
{
Guard.ThrowIfNull(connectionStringFactory);

return builder.Add(new HealthCheckRegistration(
name ?? NAME,
sp => new RedisHealthCheck((ct) => connectionStringFactory(sp, ct)),
failureStatus,
tags,
timeout));
}

/// <summary>
/// Add a health check for Redis services.
/// </summary>
@@ -120,4 +151,35 @@ public static IHealthChecksBuilder AddRedis(
tags,
timeout));
}

/// <summary>
/// Add a health check for Redis services.
/// </summary>
/// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
/// <param name="connectionMultiplexerFactory">A factory to build the Redis connection to use.</param>
/// <param name="name">The health check name. Optional. If <c>null</c> the type name 'redis' will be used for the name.</param>
/// <param name="failureStatus">
/// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
/// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
/// </param>
/// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
/// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
/// <returns>The specified <paramref name="builder"/>.</returns>
public static IHealthChecksBuilder AddRedis(
this IHealthChecksBuilder builder,
Func<IServiceProvider, CancellationToken, Task<IConnectionMultiplexer>> connectionMultiplexerFactory,
string? name = default,
HealthStatus? failureStatus = default,
IEnumerable<string>? tags = default,
TimeSpan? timeout = default)
{
Guard.ThrowIfNull(connectionMultiplexerFactory);

return builder.Add(new HealthCheckRegistration(
name ?? NAME,
sp => new RedisHealthCheck((ct) => connectionMultiplexerFactory(sp, ct)),
failureStatus,
tags,
timeout));
}
}
39 changes: 28 additions & 11 deletions src/HealthChecks.Redis/RedisHealthCheck.cs
Original file line number Diff line number Diff line change
@@ -9,14 +9,19 @@ namespace HealthChecks.Redis;
/// </summary>
public class RedisHealthCheck : IHealthCheck
{
private static readonly ConcurrentDictionary<string, IConnectionMultiplexer> _connections = new();
private readonly string? _redisConnectionString;
private static readonly ConcurrentDictionary<Func<CancellationToken, Task<string?>>, IConnectionMultiplexer> _connections = new();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this caching now broken if I create multiple RedisHealthCheck instances with the same connection string value? They each will get a different delegate instance now, where before it was keyed off the actual string value.

private readonly Func<CancellationToken, Task<string?>>? _redisConnectionStringFactory;
private readonly IConnectionMultiplexer? _connectionMultiplexer;
private readonly Func<IConnectionMultiplexer>? _connectionMultiplexerFactory;
private readonly Func<CancellationToken, Task<IConnectionMultiplexer>>? _connectionMultiplexerFactory;

public RedisHealthCheck(string redisConnectionString)
{
_redisConnectionString = Guard.ThrowIfNull(redisConnectionString);
_redisConnectionStringFactory = (_) => Task.FromResult<string?>(Guard.ThrowIfNull(redisConnectionString));
}

public RedisHealthCheck(Func<CancellationToken, Task<string?>> redisConnectionStringFactory)
{
_redisConnectionStringFactory = Guard.ThrowIfNull(redisConnectionStringFactory);
}

public RedisHealthCheck(IConnectionMultiplexer connectionMultiplexer)
@@ -35,34 +40,46 @@ public RedisHealthCheck(IConnectionMultiplexer connectionMultiplexer)
/// <seealso cref="IConnectionMultiplexer"/> for the first time, so exceptions can be handled gracefully.
/// </remarks>
internal RedisHealthCheck(Func<IConnectionMultiplexer> connectionMultiplexerFactory)
{
_connectionMultiplexerFactory = (ct) => Task.FromResult(connectionMultiplexerFactory());
}

internal RedisHealthCheck(Func<CancellationToken, Task<IConnectionMultiplexer>> connectionMultiplexerFactory)
{
_connectionMultiplexerFactory = connectionMultiplexerFactory;
}


mitchdenny marked this conversation as resolved.
Show resolved Hide resolved
/// <inheritdoc />
public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default)
{
try
{
IConnectionMultiplexer? connection = _connectionMultiplexer ?? _connectionMultiplexerFactory?.Invoke();
var connection = (_connectionMultiplexer, _connectionMultiplexerFactory) switch
{
(not null, _) => _connectionMultiplexer,
(null, { } factory) => await factory(cancellationToken).ConfigureAwait(false),
_ => null
};

if (_redisConnectionString is not null && !_connections.TryGetValue(_redisConnectionString, out connection))
if (_redisConnectionStringFactory is not null && !_connections.TryGetValue(_redisConnectionStringFactory, out connection))
{
try
{
var connectionMultiplexerTask = ConnectionMultiplexer.ConnectAsync(_redisConnectionString!);
var redisConnectionString = await _redisConnectionStringFactory(cancellationToken).ConfigureAwait(false);
var connectionMultiplexerTask = ConnectionMultiplexer.ConnectAsync(redisConnectionString!);
connection = await TimeoutAsync(connectionMultiplexerTask, cancellationToken).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
return new HealthCheckResult(context.Registration.FailureStatus, description: "Healthcheck timed out");
}

if (!_connections.TryAdd(_redisConnectionString, connection))
if (!_connections.TryAdd(_redisConnectionStringFactory, connection))
{
// Dispose new connection which we just created, because we don't need it.
connection.Dispose();
connection = _connections[_redisConnectionString];
connection = _connections[_redisConnectionStringFactory];
}
}

@@ -99,9 +116,9 @@ public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context
}
catch (Exception ex)
{
if (_redisConnectionString is not null)
if (_redisConnectionStringFactory is not null)
{
_connections.TryRemove(_redisConnectionString, out var connection);
_connections.TryRemove(_redisConnectionStringFactory, out var connection);
#pragma warning disable IDISP007 // Don't dispose injected [false positive here]
connection?.Dispose();
#pragma warning restore IDISP007 // Don't dispose injected
Original file line number Diff line number Diff line change
@@ -62,6 +62,29 @@ public void add_health_check_with_connection_string_factory_when_properly_config
factoryCalled.ShouldBeTrue();
}

[Fact]
public void add_health_check_with_async_connection_string_factory_when_properly_configured()
{
var services = new ServiceCollection();
var factoryCalled = false;
services.AddHealthChecks()
.AddRedis((sp, ct) =>
{
factoryCalled = true;
return Task.FromResult<string?>("connectionstring");
});

using var serviceProvider = services.BuildServiceProvider();
var options = serviceProvider.GetRequiredService<IOptions<HealthCheckServiceOptions>>();

var registration = options.Value.Registrations.First();
var check = registration.Factory(serviceProvider);

registration.Name.ShouldBe("redis");
check.ShouldBeOfType<RedisHealthCheck>();
factoryCalled.ShouldBeFalse();
}

[Fact]
public void add_named_health_check_with_connection_multiplexer_when_properly_configured()
{
@@ -109,4 +132,33 @@ public void add_health_check_with_connection_multiplexer_when_properly_configure
// the factory is called when it's used for the first time, as it can throw
factoryCalled.ShouldBeFalse();
}

[Fact]
public void add_health_check_with_async_connection_multiplexer_when_properly_configured()
{
var connectionMultiplexer = Substitute.For<IConnectionMultiplexer>();
var services = new ServiceCollection();

services.AddSingleton(connectionMultiplexer);
var factoryCalled = false;

services.AddHealthChecks()
.AddRedis((sp, _) =>
{
factoryCalled = true;
var multiplexer = sp.GetRequiredService<IConnectionMultiplexer>();
return Task.FromResult(multiplexer);
});

using var serviceProvider = services.BuildServiceProvider();
var options = serviceProvider.GetRequiredService<IOptions<HealthCheckServiceOptions>>();

var registration = options.Value.Registrations.First();
var check = registration.Factory(serviceProvider);

registration.Name.ShouldBe("redis");
check.ShouldBeOfType<RedisHealthCheck>();
// the factory is called when it's used for the first time, as it can throw
factoryCalled.ShouldBeFalse();
}
}
3 changes: 3 additions & 0 deletions test/HealthChecks.Redis.Tests/HealthChecks.Redis.approved.txt
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ namespace HealthChecks.Redis
public class RedisHealthCheck : Microsoft.Extensions.Diagnostics.HealthChecks.IHealthCheck
{
public RedisHealthCheck(StackExchange.Redis.IConnectionMultiplexer connectionMultiplexer) { }
public RedisHealthCheck(System.Func<System.Threading.CancellationToken, System.Threading.Tasks.Task<string?>> redisConnectionStringFactory) { }
public RedisHealthCheck(string redisConnectionString) { }
public System.Threading.Tasks.Task<Microsoft.Extensions.Diagnostics.HealthChecks.HealthCheckResult> CheckHealthAsync(Microsoft.Extensions.Diagnostics.HealthChecks.HealthCheckContext context, System.Threading.CancellationToken cancellationToken = default) { }
}
@@ -14,6 +15,8 @@ namespace Microsoft.Extensions.DependencyInjection
public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddRedis(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, StackExchange.Redis.IConnectionMultiplexer connectionMultiplexer, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }
public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddRedis(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func<System.IServiceProvider, StackExchange.Redis.IConnectionMultiplexer> connectionMultiplexerFactory, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }
public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddRedis(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func<System.IServiceProvider, string> connectionStringFactory, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }
public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddRedis(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func<System.IServiceProvider, System.Threading.CancellationToken, System.Threading.Tasks.Task<StackExchange.Redis.IConnectionMultiplexer>> connectionMultiplexerFactory, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }
public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddRedis(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func<System.IServiceProvider, System.Threading.CancellationToken, System.Threading.Tasks.Task<string?>> connectionStringFactory, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }
public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddRedis(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, string redisConnectionString, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }
}
}
Loading