Health check does not respect `AZURE_APP_CONFIGURATION_PROVIDER_DISABLED` and returns `Unhealthy`

[julealgon]

I just added the newly introduced health check from version 8.3.0 in one of our projects and immediately went to test the results, only to get an Unhealthy response locally in a project that explicitly configures AZURE_APP_CONFIGURATION_PROVIDER_DISABLED for local development.

On the console, this is logged:

fail: Microsoft.Extensions.Diagnostics.HealthChecks.DefaultHealthCheckService[103]
      Health check Microsoft.Extensions.Configuration.AzureAppConfiguration with status Unhealthy completed after 1.0959ms with message 'No configuration provider is found.'

I think this behavior is not good. Clearly, the health check implementation is ignoring the fact that the project has explicitly opted to turn off the provider, which should be a standard scenario for local development when using the AZURE_APP_CONFIGURATION_PROVIDER_DISABLED flag.

I think the team should reconsider the behavior here and return Healthy instead if the provider is not present but it has been explicitly turned off.

[zhiyuanliang-ms]

Hey, @julealgon

Thanks for calling this behavior out. I didn't consider this scenario when I implemented health check. But instead of return Healthy when provider is not present, I think it makes more sense to not add health check service when AZURE_APP_CONFIGURATION_PROVIDER_DISABLED flag is set.

This behavior will be consistent with the fact that when the flag is set, we will not add the configuration provider.

What do you think?

[julealgon]

That seems totally reasonable @zhiyuanliang-ms . I'd definitely be ok with it.

[zhiyuanliang-ms]

Hey, @julealgon

After discuss with @jimmyca15, we agreed on that it is not good to add nothing when calling AddSomething().

We decided to follow the existing pattern of adding EmptyConfigurationRefresherProvider when app config provider is disabled. code

We will add a no-op health check service which will always return healthy (just like what you proposed initially).

The thinking behind this is as follows: under normal circumstances (provider is not disabled), people can enumerate all health check registrations, and we have a dedicated name for the App Config health check registration. If we don’t add the health check when provider is disabled, in theory, this could break people's current code.

This is somewhat different from the case of adding a configuration source, because even if we don’t add the App Config source, the IConfiguration will still be there. It will not break people.

You can check my PR for the details: #700

[julealgon]

Thank you for looking into this @zhiyuanliang-ms . The reasoning for adding a null-pattern provider is interesting and makes sense. I had not thought about it from that perspective (we don't enumerate the checks ourselves but I can see that happening for sure).

[zhiyuanliang-ms]

Done #700