search

Xabaril / AspNetCore.Diagnostics.HealthChecks

Enterprise HealthChecks for ASP.NET Core Diagnostics Package
Apache License 2.0
4.01k stars 782 forks source link

Support more Azure EventHub client types #2258

Open eerhardt opened 3 weeks ago

eerhardt commented 3 weeks ago

What would you like to be added:

Today the AzureEventHubHealthCheck only supports one type of client: EventHubProducerClient.

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/42c2c732a5369849d59f9d79cdd4be497da98786/src/HealthChecks.Azure.Messaging.EventHubs/AzureEventHubHealthCheck.cs#L8-L13

We should add support for the other kinds of EventHub clients:

There is another client EventProcessorClient, however this client doesn't have a GetXPropertiesAsync() API that we use in the AzureEventHubHealthCheck. So it may not be possible to implement a Health Check based on that client.

Why is this needed:

.NET Aspire supports registering the above 4 client types. In trying to enable health checks in .NET Aspire, only the EventHubProducerClient can be used directly. See https://github.com/dotnet/aspire/pull/4139.

cc @adamsitnik @jsquire @oising

jsquire commented 3 weeks ago

The processor supports runtime properties as events are streamed from the service by setting EventProcessorClientOptions.TrackLastEnqueuedEventProperties. This can be read via the ReadLastEnqueuedEventProperties method on the partition context associated with the processing args or using the ReadLastEnqueuedEventProperties protected method on the class.

Neither is really appropriate for a pull-based approach in an external package. One option would be to do it via callback that apps could invoke as part of their event processing. Otherwise, the recommendation would be to use the producer client as the health check sentinel.

oising commented 3 weeks ago

I'm not sure I understand the reason to allow more than one type of client to validate event hub health. Is this just to be able to reuse the underlying aspire client instance rather than have to spin up a sister client 75% of the time?

eerhardt commented 3 weeks ago

Is this just to be able to reuse the underlying aspire client instance rather than have to spin up a sister client 75% of the time?

Correct. See https://github.com/dotnet/aspire/tree/main/src/Components#health-checks. Specifically:

Consider whether the Health Check should reuse the same client object registered in DI by the component or not. Reusing the same client object has the advantages of getting the same configuration, logging, etc during the health check.

oising commented 3 weeks ago

Is this just to be able to reuse the underlying aspire client instance rather than have to spin up a sister client 75% of the time?

Correct. See https://github.com/dotnet/aspire/tree/main/src/Components#health-checks. Specifically:

Consider whether the Health Check should reuse the same client object registered in DI by the component or not. Reusing the same client object has the advantages of getting the same configuration, logging, etc during the health check.

Okay. Would this issue be better placed on the Azure SDK repo?

eerhardt commented 3 weeks ago

Would this issue be better placed on the Azure SDK repo?

No. The EventHub health checks library in this repo only supports EventHubProducerClient. It can add support for EventHubConsumerClient and PartitionReceiver without a change in the Azure SDK. It would just need new constructors that take those clients, and then calling the corresponding "GetXPropertiesAsync()" methods on the respective clients.

For EventProcessorClient Aspire can continue using the producer client as the health check sentinel.

jsquire commented 3 weeks ago

Okay. Would this issue be better placed on the Azure SDK repo?

This opens up a very interesting discussion about the definition of "health check" that the Azure SDK developers have not finished arguing over. 😄