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

Support more Azure EventHub client types #2258

Open
eerhardt opened this issue Jul 10, 2024 · 6 comments
Open

Support more Azure EventHub client types #2258

eerhardt opened this issue Jul 10, 2024 · 6 comments

Comments

@eerhardt
Copy link
Collaborator

What would you like to be added:

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

private readonly EventHubProducerClient _client;
public AzureEventHubHealthCheck(EventHubProducerClient client)
{
_client = Guard.ThrowIfNull(client);
}

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

  • EventHubConsumerClient
  • PartitionReceiver

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 dotnet/aspire#4139.

cc @adamsitnik @jsquire @oising

@jsquire
Copy link

jsquire commented Jul 10, 2024

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
Copy link

oising commented Jul 11, 2024

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
Copy link
Collaborator Author

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
Copy link

oising commented Jul 11, 2024

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
Copy link
Collaborator Author

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
Copy link

jsquire commented Jul 11, 2024

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

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

No branches or pull requests

3 participants