Replace Nest to Elastic.Clients.Elasticsearch

[Alirexaa]

What this PR does / why we need it:

using Elastic.Clients.Elasticsearch package instead of deprecated Nest package

Which issue(s) this PR fixes:

Please reference the issue this PR will close: #2236

Special notes for your reviewer:

this PR brings the following breaking changes:

• Users that use the Nest package Public API without directly referencing this package.
• Change UseApiKey Public API

- public ElasticsearchOptions UseApiKey(ApiKeyAuthenticationCredentials apiKey);

+ public ElasticsearchOptions UseApiKey(string apiKey);

• Remove ApiKeyAuthenticationCredentials Public API, Because this type is part of Nest package

- public ApiKeyAuthenticationCredentials? ApiKeyAuthenticationCredentials { get; private set; }

+public string? ApiKey { get; private set; }

Does this PR introduce a user-facing change?:
Yes.
Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Unit tests passing
• End-to-end tests passing
• Extended the documentation
• Provided sample for the feature

[adamsitnik]

@Alirexaa big thanks for your contribution! Please fix the failing test.

@Mpdreamz and @stevejgordon PTAL and let us know if this is the direction we should be heading (this update will be shipped along with .NET 9 and it won't affect older Health Check versions)

[adamsitnik]

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];

[Alirexaa]

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

[adamsitnik]

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).

[unaizorrilla]

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

[adamsitnik]

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.

[adamsitnik]

@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 ;)

[Alirexaa]

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

[Mpdreamz]

Yes @adamsitnik this LGTM.

cc @flobernd who's the maintainer for the new .net client.

[Alirexaa]

The new UseElasticCloud method supports health check for elastic cloud.
it should close: #2242

[adamsitnik]

@Alirexaa great progress! PTAL at my comment about the public API design.

[adamsitnik]

I’ve been trying to wrap my head around the shape of public API and got to the following conclusions.
The ElasticsearchOptions option bag is mutable and can be modified by the optional setup action (System.Action<HealthChecks.Elasticsearch.ElasticsearchOptions>? setup = null).
The setup action takes no parameter that would allow the user to read the information from the config (which is how I imagine users provide address, username, key or password).

services.AddHealthChecks().AddElasticsearch(setup =>
{
    // how should the user read these values here?
    setup.UseElasticCloud("cloudId", "cloudApiKey");
});

The CheckHealthAsync method performs a late validation of the option bag and throws if something is missing.

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.Elasticsearch/ElasticsearchHealthCheck.cs

Lines 33 to 36 in 4a49dfa

	if (_options.AuthenticateWithBasicCredentials)
	{
	settings = settings.BasicAuthentication(_options.UserName, _options.Password);
	}

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.Elasticsearch/ElasticsearchOptions.cs

Lines 34 to 37 in 4a49dfa

	public ElasticsearchOptions UseBasicAuthentication(string name, string password)
	{
	UserName = Guard.ThrowIfNull(name);
	Password = Guard.ThrowIfNull(password);

That exception won’t be easy to discover, as it will be reported somewhere up in the ASP.NET stack as a health check error.
On the other hand, the ElasticsearchClientSettingsprovided by Elastic has dedicated ctors that expect everything to be provided up front.

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.Elasticsearch/ElasticsearchHealthCheck.cs

Lines 35 to 37 in 7b374ae

	settings = _options.AuthenticateWithElasticCloud
	? new ElasticsearchClientSettings(_options.CloudId!, new ApiKey(_options.ApiKey!))
	: new ElasticsearchClientSettings(new Uri(_options.Uri!));

I believe that this overload should not expose the setup action and just require the users to configure everything before calling the factory method in the DI container and then just resolve the right instance in the setup

Example based on this doc

builder.Services.AddSingleton(s => new ElasticClient(
    Environment.GetEnvironmentVariable("ELASTIC_CLOUD_ID"),
    new ApiKeyAuthenticationCredentials(Environment.GetEnvironmentVariable("ELASTIC_API_KEY"))));

builder.Services..AddHealthChecks().AddElasticsearch();

Suggested change

public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddElasticsearch(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func<System.IServiceProvider, Elastic.Clients.Elasticsearch.ElasticsearchClient>? clientFactory = null, System.Action<HealthChecks.Elasticsearch.ElasticsearchOptions>? setup = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }

public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddElasticsearch(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func<System.IServiceProvider, Elastic.Clients.Elasticsearch.ElasticsearchClient>? clientFactory = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable<string>? tags = null, System.TimeSpan? timeout = default) { }

[adamsitnik]

Overall it LGTM, thank you for your contribution @Alirexaa !

I have found some nits, I am just going to apply mu suggestions and hit the merge button.

[Alirexaa]

@adamsitnik, we should block this PR until .NET 9 release, Right?

[adamsitnik]

we should block this PR until .NET 9 release, Right?

AFAIK the package won't be released to nuget.org until @unaizorrilla triggers some automation, so there is no need to wait with the merge.