Eventhubs health check (retargeted for main)#4139
Conversation
- address nit
…ng to align with other components
ghost
added
the
area-codeflow
dotnet-policy-service
Bot
added
the
community-contribution
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @oising!
I merged the latest main into this PR, and made some minor PR feedback. I think the product changes look good.
Is there a way we can write some tests for this functionality? I think that is all that is needed in order to merge this.
radical
added
area-integrations
|
@eerhardt Do we still want this PR? |
Yes, I think so. Just looking for tests to be added. |
I'm just back from vacation. I'll look at this over the weekend. /cc @davidfowl |
|
We have this for the hosting integration but not the client integration? |
Correct - we added it to the hosting integration when we did the emulator work. This PR is adding the health check to the client integration. |
danmoseley
force-pushed
the
eventhubs-healthcheck-main
branch
from
e186474 to
f05024c
Compare
|
Some of the tests still failing and I haven't figured out why. It seems those components are not getting registered correctly. Might need help. |
| [assembly: ConfigurationSchema("Aspire:Azure:Messaging:EventHubs:PartitionReceiver", typeof(AzureMessagingEventHubsPartitionReceiverSettings))] | ||
| [assembly: ConfigurationSchema("Aspire:Azure:Messaging:EventHubs:PartitionReceiver:ClientOptions", typeof(PartitionReceiverOptions))] | ||
|
|
||
| [assembly: ConfigurationSchema("Aspire:Azure:Messaging:EventHubs:EventHubBufferedProducerClient", typeof(AzureMessagingEventHubsBufferedProducerSettings))] |
There was a problem hiding this comment.
@eerhardt I don't think this was ignored on purpose, correct?
There was a problem hiding this comment.
The support for this client was added by someone other than myself, with a small PR. They weren't familiar with the codebase and I would say it's 99% sure they didn't know about this plumbing. I can't remember who it was, but it would probably show up in PRs if you search for that client type.
| var producerClient = !string.IsNullOrEmpty(settings.ConnectionString) ? | ||
| new EventHubProducerClient(settings.ConnectionString, producerClientOptions) : | ||
| new EventHubProducerClient(settings.FullyQualifiedNamespace, settings.EventHubName, settings.Credential ?? new DefaultAzureCredential(), producerClientOptions); |
There was a problem hiding this comment.
This isn't great because it will create a new EventHubProducerClient instance each time the health is checked. EventHubProducerClient establishes an amqp connection, which isn't cheap.
I'm not sure, but I don't believe the client is getting disposed. So we might risk having memory issues here.
There was a problem hiding this comment.
I've opened EventHubs health checks create a new client on each health check (dotnet/aspire#7537) for this issue.
There was a problem hiding this comment.
That's surprising -- I assumed CreateHealthCheck would be called once just to create it, and then subsequent calls happen on the return instance. Odd pattern?
There was a problem hiding this comment.
TBH - I had this intuition at first as well. But unfortunately, that's not how HealthChecks works in ASP.NET Core.
See
- Code that runs the health checks
- Note it calls the
registration.Factory(scope.ServiceProvider);every time, and then calls.CheckHealthAsyncon the returned object. - The
HealthCheckRegistrationis the singleton/cached object that is reused across checks. TheIHealthCheckis the transient object.
- Note it calls the
- Azure health checks: How to steer the users towards best practices (Xabaril/AspNetCore.Diagnostics.HealthChecks#2040)
There was a problem hiding this comment.
TBH - I had this intuition at first as well. But unfortunately, that's not how HealthChecks works in ASP.NET Core.
Urgh. What do you think of using the event hub REST API as a health check instead of instantiating heavy clients?
There was a problem hiding this comment.
In my mind, the ideal health check follows this logic:
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.
From @jsquire's comment here: Xabaril/AspNetCore.Diagnostics.HealthChecks#2258 (comment) The first bullet aligns the most with this thinking:
Is it something client-focused that indicates "yes, the processor is still functioning and attempting to read. If you have no events flowing, don't panic there's just no data in the partitions."
The closer we can get to seeing if the processor is still functioning, the better IMO.
I think we have a couple options:
- Keep going down the route of using the producer client as the health check sentinel. But make it cached, so we aren't creating new heavy-weight clients
- Use the REST API as a health check
- Only have a health check for the producer client until we have a plan for health checks for consumer/receiver clients
For .NET Aspire 9.1, the top option feels like it is the path of least resistence. We already decided to use the producer client, let's just make it better.
There was a problem hiding this comment.
Definitely worth checking the other health checks for similar pitfalls... Lazy<T> all the things?
6 participants
Original: #4050
Closes #3980
Microsoft Reviewers: Open in CodeFlow