Add Redis Health Check

[hishamco]

Part of #13588.

[hishamco]

@jtkech unable to resolve service for type OrchardCore.Redis.IRedisService

[jtkech]

Maybe it failed to configure RedisOptions, in that case IRedisService is not registered and you may have a logged error message.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Redis/Startup.cs

Lines 42 to 55 in cc52c8d

	services.Configure<RedisOptions>(options =>
	{
	options.Configuration = configuration;
	options.ConfigurationOptions = configurationOptions;
	options.InstancePrefix = instancePrefix;
	});
	}
	catch (Exception e)
	{
	_logger.LogError(e, "'Redis' features are not active on tenant '{TenantName}' as the 'Configuration' string is missing or invalid.", _tenant);
	return;
	}
	services.AddSingleton<IRedisService, RedisService>();

The other Redis features check if IRedisService is registered, they can do that because they depend on the main OC.Redis feature (manifest dependencies) so their related startups execute after.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Redis/Startup.cs

Lines 66 to 71 in cc52c8d

	if (services.Any(d => d.ServiceType == typeof(IRedisService)))
	{
	services.AddSingleton<IDistributedCache, RedisCacheWrapper>();
	services.AddTransient<IConfigureOptions<RedisCacheOptions>, RedisCacheOptionsSetup>();
	services.AddScoped<ITagCache, RedisTagCache>();
	}

In the HealthChecks startup we could override the Order property to a quite high value and then check if IRedisService is registered as done in the other startups.

Hmm, but I think it's better to update RedisHealthCheck to not inject IRedisService but ServiceProvider, then resolve lazily IRedisService by not using GetRequiredService() but by using .GetService() which doesn't throw, then if it is null it would mean that it is Unhealthy.

Does PingAsync() can throw an exception? If so you may need to use a try/catch block.

[hishamco]

Hmm, but I think it's better to update RedisHealthCheck to not inject IRedisService but ServiceProvider

This is may be a better solution but I'm not sure why it can't be retrieved from DI directly, let me check and come back to you

[jtkech]

but I'm not sure why it can't be retrieved from DI directly

See my previous comment and look at the Redis Startup to see how it is done.

Maybe it failed to configure RedisOptions, in that case IRedisService is not registered and you may have a logged error message.

[hishamco]

Everything works as expected, the RedisOptions wasn't set previously, I didn't think it will break IRedisService

[hishamco]

We might need to add abortConnect=false to the redis connection

[hishamco]

@jtkech could you please have a quick review once you have time

[Piedone]

Add a comment on why this is necessary, and what's the reason for 100 in particular.

[hishamco]

I might need to check this one, I remember this might be necessary to run the health check early

[Piedone]

So, also document what's the reason for 100 in particular. This is a magic number; can it be 90? Can it be 110? Why not 1000?

[hishamco]

Could be, no particular reason for using 10 but usually we're using 100 or more to avoid start ordering issue

[Piedone]

OK, so please document it.

[hishamco]

BTW there are many places where we use 1000 for instance without docs :)

[Piedone]

And those should have been documented as well :). Having less-than-ideal pieces of code currently doesn't mean that we should keep writing the same.

[Piedone]

So, also document what's the reason for 100 in particular. This is a magic number; can it be 90? Can it be 110? Why not 1000?

[hishamco]

Let's finalize the health checks PRs, so I can focus to finishing the others

[Piedone]

This is not addressed: https://github.com/OrchardCMS/OrchardCore/pull/13589/files#r1534675914

[hishamco]

I already added a comment, as I said before 100 does not matter, the important thing is to choose an order greater than 0

[Piedone]

Great, all clear, now add this to the comment :).

[hishamco]

Seems the added comment is not enough, time to modify :)