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

Replace Nest to Elastic.Clients.Elasticsearch #2244

Merged
merged 13 commits into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 32 additions & 16 deletions src/HealthChecks.Elasticsearch/ElasticsearchHealthCheck.cs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
using System.Collections.Concurrent;
using Elasticsearch.Net;
using Elastic.Clients.Elasticsearch;
using Elastic.Transport;
using Microsoft.Extensions.Diagnostics.HealthChecks;
using Nest;

namespace HealthChecks.Elasticsearch;

public class ElasticsearchHealthCheck : IHealthCheck
{
private static readonly ConcurrentDictionary<string, ElasticClient> _connections = new();
private static readonly ConcurrentDictionary<string, ElasticsearchClient> _connections = new();

private readonly ElasticsearchOptions _options;

Expand All @@ -21,9 +21,9 @@ public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context
{
try
{
if (!_connections.TryGetValue(_options.Uri, out var lowLevelClient))
if (!_connections.TryGetValue(_options.Uri, out var elasticsearchClient))
Copy link
Collaborator

Choose a reason for hiding this comment

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

not related to your changes, but something that we should improve in the future:

From https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/recommendations.html:

Client reuse can be achieved by creating a singleton static instance or by registering the type with a singleton lifetime when using dependency injection containers.

So same as #2113 we should try to steer the users towards best practices and provide and recommend overloads that accept ElasticsearchClient and simply re-use it (to there would be one instance for the whole app, not one for health checks and another for the app itself).

Copy link
Collaborator

@unaizorrilla unaizorrilla Jun 24, 2024

Choose a reason for hiding this comment

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

@adamsitnik what about many health checks for different Elastic instances?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what about many health checks for different Elastic instances?

We could use the same approach we have for Azure health checks: by default a singleton instance is resolved from DI, but the users can provide an overload that solves it on their own.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Alirexaa I don't expect you to change it in this PR, this is just something to consider as a next improvement. And since we are going to introduce breaking changes now, we could simply introduce one more breaking change ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamsitnik, is the last change providing all the things we need?
I think we should make all the changes here.

{
var settings = new ConnectionSettings(new Uri(_options.Uri));
var settings = new ElasticsearchClientSettings(new Uri(_options.Uri));

if (_options.RequestTimeout.HasValue)
{
Expand All @@ -32,49 +32,65 @@ public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context

if (_options.AuthenticateWithBasicCredentials)
{
settings = settings.BasicAuthentication(_options.UserName, _options.Password);
if (_options.UserName is null)
{
throw new ArgumentNullException(nameof(_options.UserName));
}
if (_options.Password is null)
{
throw new ArgumentNullException(nameof(_options.Password));
}
settings = settings.Authentication(new BasicAuthentication(_options.UserName, _options.Password));
}
else if (_options.AuthenticateWithCertificate)
{
if (_options.Certificate is null)
{
throw new ArgumentNullException(nameof(_options.Certificate));
}
settings = settings.ClientCertificate(_options.Certificate);
}
else if (_options.AuthenticateWithApiKey)
{
settings = settings.ApiKeyAuthentication(_options.ApiKeyAuthenticationCredentials);
if (_options.ApiKey is null)
{
throw new ArgumentNullException(nameof(_options.ApiKey));
}
settings.Authentication(new ApiKey(_options.ApiKey));
}

if (_options.CertificateValidationCallback != null)
{
settings = settings.ServerCertificateValidationCallback(_options.CertificateValidationCallback);
}

lowLevelClient = new ElasticClient(settings);
elasticsearchClient = new ElasticsearchClient(settings);

if (!_connections.TryAdd(_options.Uri, lowLevelClient))
if (!_connections.TryAdd(_options.Uri, elasticsearchClient))
{
lowLevelClient = _connections[_options.Uri];
elasticsearchClient = _connections[_options.Uri];
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not related to your specific changes, but I wonder: does ElasticsearchClient implement IDisposable? If so, it should be disposed here (we have failed to add it to the cache and decided to reuse the existing instance)

Suggested change
elasticsearchClient = _connections[_options.Uri];
elasticsearchClient.Dispose();
elasticsearchClient = _connections[_options.Uri];

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not implement.
Is there anything else I can do about this?

}
}

if (_options.UseClusterHealthApi)
{
var healthResponse = await lowLevelClient.Cluster.HealthAsync(ct: cancellationToken).ConfigureAwait(false);
var healthResponse = await elasticsearchClient.Cluster.HealthAsync(cancellationToken: cancellationToken).ConfigureAwait(false);

if (healthResponse.ApiCall.HttpStatusCode != 200)
if (healthResponse.ApiCallDetails.HttpStatusCode != 200)
{
return new HealthCheckResult(context.Registration.FailureStatus);
}

return healthResponse.Status switch
{
Health.Green => HealthCheckResult.Healthy(),
Health.Yellow => HealthCheckResult.Degraded(),
Elastic.Clients.Elasticsearch.HealthStatus.Green => HealthCheckResult.Healthy(),
Elastic.Clients.Elasticsearch.HealthStatus.Yellow => HealthCheckResult.Degraded(),
_ => new HealthCheckResult(context.Registration.FailureStatus)
};
}

var pingResult = await lowLevelClient.PingAsync(ct: cancellationToken).ConfigureAwait(false);
bool isSuccess = pingResult.ApiCall.HttpStatusCode == 200;
var pingResult = await elasticsearchClient.PingAsync(cancellationToken: cancellationToken).ConfigureAwait(false);
bool isSuccess = pingResult.ApiCallDetails.HttpStatusCode == 200;

return isSuccess
? HealthCheckResult.Healthy()
Expand Down
9 changes: 4 additions & 5 deletions src/HealthChecks.Elasticsearch/ElasticsearchOptions.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using System.Net.Security;
using System.Security.Cryptography.X509Certificates;
using Elasticsearch.Net;

namespace HealthChecks.Elasticsearch;

Expand All @@ -17,8 +16,6 @@ public class ElasticsearchOptions

public X509Certificate? Certificate { get; private set; }

public ApiKeyAuthenticationCredentials? ApiKeyAuthenticationCredentials { get; private set; }

public bool AuthenticateWithBasicCredentials { get; private set; }

public bool AuthenticateWithCertificate { get; private set; }
Expand All @@ -27,6 +24,8 @@ public class ElasticsearchOptions

public bool UseClusterHealthApi { get; set; }

public string? ApiKey { get; private set; }

public Func<object, X509Certificate, X509Chain, SslPolicyErrors, bool>? CertificateValidationCallback { get; private set; }

public TimeSpan? RequestTimeout { get; set; }
Expand Down Expand Up @@ -55,9 +54,9 @@ public ElasticsearchOptions UseCertificate(X509Certificate certificate)
return this;
}

public ElasticsearchOptions UseApiKey(ApiKeyAuthenticationCredentials apiKey)
public ElasticsearchOptions UseApiKey(string apiKey)
{
ApiKeyAuthenticationCredentials = Guard.ThrowIfNull(apiKey);
ApiKey = Guard.ThrowIfNull(apiKey);

UserName = string.Empty;
Password = string.Empty;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="NEST" Version="7.17.5" />
<PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks" Version="8.0.0" />
<PackageReference Include="Elastic.Clients.Elasticsearch" Version="8.14.3" />
<PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks" Version="8.0.6" />
<!--<PackageReference Include="NEST" Version="7.17.5" />-->
Alirexaa marked this conversation as resolved.
Show resolved Hide resolved

</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System.Net;
using Elasticsearch.Net;
using HealthChecks.Elasticsearch.Tests.Fixtures;

namespace HealthChecks.Elasticsearch.Tests.Functional;
Expand All @@ -25,7 +24,7 @@ public async Task be_healthy_if_elasticsearch_is_using_valid_api_key()
.AddElasticsearch(options =>
{
options.UseServer(connectionString);
options.UseApiKey(new ApiKeyAuthenticationCredentials(_fixture.ApiKey));
options.UseApiKey(_fixture.ApiKey!);
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
options.UseCertificateValidationCallback(delegate
{
return true;
Expand Down
Loading