dotnet / extensions

This repository contains a suite of libraries that provide facilities commonly needed when creating production-ready applications.
MIT License
2.58k stars 740 forks source link

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

Open IEvangelist opened 11 months ago

IEvangelist commented 11 months ago

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

geeknoid commented 11 months ago

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

IEvangelist commented 11 months ago

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).

davidfowl commented 11 months ago

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

geeknoid commented 11 months ago

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.

IEvangelist commented 11 months ago

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.

ReviveDigitalTeam commented 1 month ago

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

Otherwise adding AddResourceUtilizationHealthCheck just breaks the simple /health call.