AspNetCore.Diagnostics.HealthChecks icon indicating copy to clipboard operation
AspNetCore.Diagnostics.HealthChecks copied to clipboard

Add an overload to Kafka extensions for registering multiple instances

Open Alirexaa opened this issue 1 year ago • 5 comments

What this PR does / why we need it: This pull request introduces a new overload for the AddKafka method in the KafkaHealthCheckBuilderExtensions class, which allows configuring Kafka health checks using an optionFactory. Additionally, the corresponding method signature is added to the approved API file.

New Overload for Kafka Health Check Configuration:

  • src/HealthChecks.Kafka/DependencyInjection/KafkaHealthCheckBuilderExtensions.cs: Added a new overload of the AddKafka method that accepts an optionFactory to configure Kafka health checks. This method includes parameters for the health check name, failure status, tags, and timeout.

Update to Approved API:

  • test/HealthChecks.Kafka.Tests/HealthChecks.Kafka.approved.txt: Added the new overload of the AddKafka method to the approved API file to reflect the changes in the method signature. Which issue(s) this PR fixes:

Please reference the issue this PR will close: #2298

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

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

Alirexaa avatar Dec 05 '24 21:12 Alirexaa

@adamsitnik, I prefer that overload too but this mean we should remove all other overloads and introduce breaking changes. Should we do this?

Alirexaa avatar Dec 06 '24 14:12 Alirexaa

@adamsitnik, I prefer that overload too but this mean we should remove all other overloads and introduce breaking changes. Should we do this?

This time I am not 100% convinced as the new approach would require the users to always specify TKey, TValue and the message builder. So it would not be super easy to move to the new version.

I was hoping to get feedback from @mitchdenny but it seems that he just started his winter holiday leave.

@eerhardt what are your thoughts on this? Should we introduce such a breaking change or perhaps just extend the existing ways with a new overload?

adamsitnik avatar Dec 09 '24 10:12 adamsitnik

@eerhardt what are your thoughts on this? Should we introduce such a breaking change or perhaps just extend the existing ways with a new overload?

My default opinion is to not introduce a breaking change if you can help it. If anything, we could create a new IHealthCheck class, if necessary.

eerhardt avatar Dec 09 '24 20:12 eerhardt

@eerhardt what are your thoughts on this? Should we introduce such a breaking change or perhaps just extend the existing ways with a new overload?

My default opinion is to not introduce a breaking change if you can help it. If anything, we could create a new IHealthCheck class, if necessary.

I thought about new class and it seems creating a new class is better approach. I think this new factory method could create memory leak. Also with new class we can resolve singlton client from factory method and mark older class and methods as obsolated.

Alirexaa avatar Dec 10 '24 20:12 Alirexaa

@adamsitnik Should we add a new HealthCheck class?

Alirexaa avatar Dec 13 '24 18:12 Alirexaa