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

Refactor healthchecks service mapping to support filtering on check #2142

Merged
merged 5 commits into from
Jun 14, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented May 30, 2023

Fixes #2141

  • Previously, all health checks were run when Check was called, and then they were filtered. This PR changes the filter to happen before health checks are run. Now health checks can be selectly run depending on Check's service name argument.
  • Previous health result filtering is now obsolete. The new predicate is based on the information available when a health check is registered (its name and tags).
  • Add some logging to the health check publisher.

API changes:

namespace Grpc.AspNetCore.HealthChecks;

+public sealed class HealthCheckMapContext
+{
+   public HealthCheckMapContext(string name, IEnumerable<string> tags);
+   public string Name { get; }
+   public IEnumerable<string> Tags { get; }
+}

+[Obsolete($"HealthResult is obsolete and will be removed in a future release. Use {nameof(HealthCheckMapContext)} instead.")]
public sealed class HealthResult
{
    public HealthResult(string name, IEnumerable<string> tags, HealthStatus status, string? description, TimeSpan duration, Exception? exception, IReadOnlyDictionary<string, object> data);
    public string Name { get; }
    public IEnumerable<string> Tags { get; }
    public HealthStatus Status { get; }
    public string? Description { get; }
    public IReadOnlyDictionary<string, object> Data { get; }
    public Exception? Exception { get; }
    public TimeSpan Duration { get; }
}

public sealed class ServiceMapping
{
+   [Obsolete("This constructor is obsolete and will be removed in the future. Use ServiceMapping(string name, Func<HealthCheckRegistration, bool> predicate) to map service names to .NET health checks.")]
    public ServiceMapping(string name, Func<HealthResult, bool> predicate);
+   public ServiceMapping(string name, Func<HealthCheckMapContext, bool> predicate);
+   public Func<HealthCheckMapContext, bool>? HealthCheckPredicate { get; }
+   [Obsolete($"This member is obsolete and will be removed in the future. Use {nameof(HealthCheckPredicate)} to map service names to .NET health checks.")]
    public Func<HealthResult, bool>? Predicate { get; }
}

-public sealed class ServiceMappingCollection : IEnumerable<ServiceMapping>
+public sealed class ServiceMappingCollection : ICollection<ServiceMapping>
{
+   [Obsolete("This method is obsolete and will be removed in the future. Use Map(string name, Func<HealthCheckRegistration, bool> predicate) to map service names to .NET health checks.")]
    public void MapService(string name, Func<HealthResult, bool> predicate);
+   public void Map(string name, Func<HealthCheckMapContext, bool> predicate);
}

Usage:

builder.Services.AddGrpcHealthChecks(o =>
{
    o.Services.Map("greet.Greeter", c => c.Tags.Contains("greeter"));
    o.Services.Map("count.Counter", c => c.Tags.Contains("counter"));
});

@JamesNK
Copy link
Member Author

JamesNK commented May 31, 2023

@gfoidl I've simplified the code in the area you commented on. The publisher runs every couple of seconds so I just want to keep it simple.

@gfoidl
Copy link
Contributor

gfoidl commented May 31, 2023

Yep, looks better now 😄
Thanks!

Copy link
Contributor

@mgravell mgravell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; my only comment was something I then deleted

@JamesNK JamesNK merged commit 697f349 into grpc:master Jun 14, 2023
@JamesNK JamesNK deleted the jamesnk/healthchecks-mapping branch June 14, 2023 05:11
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.

gRPC health checks in kubernetes scenario - unable to execute on demand only subset of healthchecks
3 participants