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

Register internal ResourceMonitor with ResourceUtilizationHealthCheck #5413

Conversation

evgenyfedorov2
Copy link
Contributor

@evgenyfedorov2 evgenyfedorov2 commented Sep 10, 2024

Fixes #4560

What is changing

ResourceUtilizationHealthCheck depends on IResourceMonitor and requires an implementation of it to be registered first, which is also mentioned in #4560. Typically, users would achieve this by calling the AddResourceMonitoring() method which registers the internal ResourceMonitorService for this purpose:

var serviceCollection = new ServiceCollection();
serviceCollection.AddResourceMonitoring();
serviceCollection.AddResourceUtilizationHealthCheck();

After this change, users don't have to call AddResourceMonitoring() and can just call AddResourceUtilizationHealthCheck() immediately:

var serviceCollection = new ServiceCollection();
serviceCollection.AddResourceUtilizationHealthCheck();

Impact

  • If you use AddResourceMonitoring() method call, you may omit it.

  • If you use a custom implementation of IResourceMonitor and register it using the AddSingleton<IResourceMonitor, TImplementation>() method, then no action is required.

  • If you use a custom implementation of IResourceMonitor and register it using the TryAddSingleton<IResourceMonitor, TImplementation>() method which is placed before AddResourceUtilizationHealthCheck(), then no action is required. For example, assuming the custom implementation TImplementation class name is MyResourceMonitorService, the following scenarios will continue to work as is:

    ✅ works:

     // 1.
     var serviceCollection = new ServiceCollection();
     serviceCollection.AddSingleton<IResourceMonitor, MyResourceMonitorService>();
     serviceCollection.AddResourceUtilizationHealthCheck();
    

    ✅ works:

    // 2.
    var serviceCollection = new ServiceCollection();
    serviceCollection.AddResourceUtilizationHealthCheck();
    serviceCollection.AddSingleton<IResourceMonitor, MyResourceMonitorService>();
    

    ✅ works:

    // 3.
    var serviceCollection = new ServiceCollection();
    serviceCollection.TryAddSingleton<IResourceMonitor, MyResourceMonitorService>();
    serviceCollection.AddResourceUtilizationHealthCheck();
    
  • If you use TryAddSingleton<IResourceMonitor, TImplementation>() which is placed after the AddResourceUtilizationHealthCheck() method call, then the following will not work, because the internal ResourceMonitorService will be registered and used instead of the MyResourceMonitorService:

    ❌ does not work:

     // 4.
     var serviceCollection = new ServiceCollection();
     serviceCollection.AddResourceUtilizationHealthCheck();
     serviceCollection.TryAddSingleton<IResourceMonitor, MyResourceMonitorService>();
    

    To fix that, please use either AddSingleton<IResourceMonitor, TImplementation>() extension methods, or place the registration before the AddResourceUtilizationHealthCheck() method call:

    ✅ works:

     // 5.
     var serviceCollection = new ServiceCollection();
     serviceCollection.TryAddSingleton<IResourceMonitor, MyResourceMonitorService>();
     serviceCollection.AddResourceUtilizationHealthCheck();
    
Microsoft Reviewers: Open in CodeFlow

@RussKie
Copy link
Member

RussKie commented Sep 11, 2024

  • This change breaks customers who use TryAddSingleton<IResourceMonitor>() only when it is placed after the AddResourceUtilizationHealthCheck() method call. I don't think there is a way to avoid this.

How can we help customers to detect and avoid this?
What will be the experience if customers do have the "incorrect" code? Will they be getting meaningful error messages?
What kind of documentation and primers do we need for this?

@RussKie RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 11, 2024
@evgenyfedorov2
Copy link
Contributor Author

  • This change breaks customers who use TryAddSingleton<IResourceMonitor>() only when it is placed after the AddResourceUtilizationHealthCheck() method call. I don't think there is a way to avoid this.

How can we help customers to detect and avoid this?

We will update our documentation at https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostic-health-checks

What will be the experience if customers do have the "incorrect" code? Will they be getting meaningful error messages?

The library will silently use the built-in ResourceMonitoringService instead of the custom one provided by customers. Also, I don't see any way to avoid this because we cannot hook into the IServiceProvider right before the build started and check if a custom implementation was registered or not.

What kind of documentation and primers do we need for this?

Update the docs as mentioned above. Add a note about the order of registration.

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 11, 2024
@RussKie RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 12, 2024
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Sep 13, 2024
@RussKie
Copy link
Member

RussKie commented Sep 16, 2024

Please link the pull-request updating the docs here as well.

@evgenyfedorov2 evgenyfedorov2 merged commit 2018ae6 into dotnet:main Sep 16, 2024
6 checks passed
@evgenyfedorov2
Copy link
Contributor Author

Please link the pull-request updating the docs here as well.

dotnet/docs#42626

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The health checks resource utilization library assumes IResourceMonitor is registered.
2 participants