extensions icon indicating copy to clipboard operation
extensions copied to clipboard

The health checks resource utilization library assumes `IResourceMonitor` is registered.

Open IEvangelist opened this issue 2 years ago • 5 comments

Description

The Microsoft.Extensions.Diagnostics.HealthChecks.ResourceUtilization assumes that the IResourceMonitor service has been registered, it's my belief that if this package relies on the resource monitoring services:

https://github.com/dotnet/extensions/blob/6fda2c8b8f8bacf74eaafff243a10ea20f849aaa/src/Libraries/Microsoft.Extensions.Diagnostics.HealthChecks.ResourceUtilization/Microsoft.Extensions.Diagnostics.HealthChecks.ResourceUtilization.csproj#L20

It should be responsible for ensuring that the IResourceMonitor is added to the IServiceCollection.

Reproduction Steps

Create a simple console app:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <LangVersion>preview</LangVersion>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks.ResourceUtilization" Version="8.0.0-rc.2.23510.2" />
    <PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0-rc.2.23479.6" />
  </ItemGroup>

</Project>

Update the Program.cs with the following:

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Diagnostics.HealthChecks;
using Microsoft.Extensions.Hosting;

var builder = Host.CreateApplicationBuilder(args);

var healthChecksBuilder = builder.Services.AddHealthChecks()
    .AddResourceUtilizationHealthCheck();

var app = builder.Build();

var healthCheckService = app.Services.GetRequiredService<HealthCheckService>();

var result = await healthCheckService.CheckHealthAsync();

_ = result;

app.Run();

This results in the following:

image

Expected behavior

I'd expect this to run without exception, as calling AddResourceUtilizationHealthCheck would add the required services.

Actual behavior

A runtime exception is thrown.

Regression?

No response

Known Workarounds

Manually call AddResourceMonitoring.

Configuration

No response

Other information

No response

IEvangelist avatar Oct 12 '23 20:10 IEvangelist

This would create a coupling between the implementation of the health check and the implementation of the IResourceMonitoring API, which I think is incorrect.

geeknoid avatar Oct 16 '23 18:10 geeknoid

The coupling is already there, I'm forced to add a NuGet package reference to something I know nothing about, nor do I know what needs to be implemented to satisfy this error—that's going to be a bad end user experience. If you want more developers adopting this package, you'll want to make this optional, to where the consumer wouldn't be required to do anything extra to consume your library (the happy path).

IEvangelist avatar Oct 17 '23 03:10 IEvangelist

Agree with @IEvangelist. What would you expect the user to do in this situation @geeknoid? Are we missing another abstraction or default implementation?

davidfowl avatar Oct 17 '23 03:10 davidfowl

The issue is that you have two abstractions and associated implementations:

IFoo and Foo as an implementation IFooProvider and FooProvider as an implementation

If I add <IFoo, Foo> to DI should I also force <IFooProvider, FooProvider> into DI? This breaks the abstraction in IMO since it's implicitly assuming that because the user chose to implement IFoo with Foo, that also means they implicitly chose to implement IFooProvider with FooProvider. What if they instead want MyCustomFooProvider instead?

This is pretty pervasive in how we put together the R9 libs at this point. The customer generally needs to attach explicitly the providers and the available implementations.

geeknoid avatar Oct 17 '23 04:10 geeknoid

I'm saying you provide a DefaultFooProvider to use when I don't give you one, and give me the choice to omit it—I believe this should be optional. That way I don't care that you need rely on an IFooProvider, I just care about IFoo. Also, assuming that you register a default, that shouldn't prevent me from providing a MyCustomFooProvider.

IEvangelist avatar Oct 17 '23 04:10 IEvangelist

I had to find this thread and add AddResourceMonitoring to my code.

Otherwise adding AddResourceUtilizationHealthCheck just breaks the simple /health call.

ReviveDigitalTeam avatar Aug 08 '24 23:08 ReviveDigitalTeam