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

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

Closed
IEvangelist opened this issue Oct 12, 2023 · 6 comments · Fixed by #5413
Closed
Assignees
Labels
area-fundamentals enhancement This issue represents an ask for new feature or an enhancement to an existing one work in progress 🚧

Comments

@IEvangelist
Copy link
Member

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:

<ProjectReference Include="..\Microsoft.Extensions.Diagnostics.ResourceMonitoring\Microsoft.Extensions.Diagnostics.ResourceMonitoring.csproj" />

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
Copy link
Member

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
Copy link
Member Author

IEvangelist commented Oct 17, 2023

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
Copy link
Member

davidfowl commented Oct 17, 2023

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

@geeknoid
Copy link
Member

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
Copy link
Member Author

IEvangelist commented Oct 17, 2023

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.

@geeknoid geeknoid added area-fundamentals enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Nov 27, 2023
@geeknoid geeknoid assigned mobratil and unassigned rafal-mz Mar 16, 2024
@ReviveDigitalTeam
Copy link

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

Otherwise adding AddResourceUtilizationHealthCheck just breaks the simple /health call.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-fundamentals enhancement This issue represents an ask for new feature or an enhancement to an existing one work in progress 🚧
Projects
None yet
7 participants