Skip to content

Add Health Check tags to client integrations #8877

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Alirexaa
Copy link
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #2138

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • No
  • Does the change make any security assumptions or guarantees?
    • No
  • Does the change require an update in our Aspire docs?

@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label Apr 18, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 18, 2025
@Alirexaa
Copy link
Contributor Author

@eerhardt, I want to do similar things for other clients at this PR, but before that, I want to see your feedback about these changes.

@@ -136,7 +136,8 @@ private static void AddRedisClient(
// That is why we don't invoke it here, but capture the state (in a closure)
// and let the health check invoke it and handle the exception (if any).
connectionMultiplexerFactory: sp => serviceKey is null ? sp.GetRequiredService<IConnectionMultiplexer>() : sp.GetRequiredKeyedService<IConnectionMultiplexer>(serviceKey),
healthCheckName));
healthCheckName,
tags: settings.HealthCheckTags));
Copy link
Member

Choose a reason for hiding this comment

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

I almost wonder if we want/need a more general way to affect the HealthCheckRegistration. It has other properties on it that a user may want to set:

https://github.com/dotnet/aspnetcore/blob/7ced4d6df649ecdd9b298d1b30491f837c815165/src/HealthChecks/Abstractions/src/HealthCheckRegistration.cs#L142-L171

  • Timeout
  • Delay
  • Period

Any thoughts @sebastienros on if we need to be able to configure these things? Or would we tell someone to just turn ours off, and add their own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Timeout property has been configured for some clients:

if (!settings.DisableHealthChecks)
{
var healthCheckName = serviceKey is null ? "Qdrant.Client" : $"Qdrant.Client_{connectionName}";
builder.TryAddHealthCheck(new HealthCheckRegistration(
healthCheckName,
sp => new QdrantHealthCheck(serviceKey is null ?
sp.GetRequiredService<QdrantClient>() :
sp.GetRequiredKeyedService<QdrantClient>(serviceKey)),
failureStatus: null,
tags: null,
timeout: settings.HealthCheckTimeout
));
}

if (!settings.DisableHealthChecks)
{
var healthCheckName = serviceKey is null ? "Elastic.Clients.Elasticsearch" : $"Elastic.Clients.Elasticsearch_{connectionName}";
builder.TryAddHealthCheck(new HealthCheckRegistration(
healthCheckName,
sp => new ElasticsearchHealthCheck(serviceKey is null ?
sp.GetRequiredService<ElasticsearchClient>() :
sp.GetRequiredKeyedService<ElasticsearchClient>(serviceKey)),
failureStatus: null,
tags: null,
timeout: settings.HealthCheckTimeout > 0 ? TimeSpan.FromMilliseconds(settings.HealthCheckTimeout.Value) : null
));
}

@davidfowl davidfowl changed the title Add Health Check tags to client components Add Health Check tags to client integrations Apr 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support tags for Health Checks added by Components
2 participants