LNK-2883: Added Kafka health check to .NET services #494
Conversation
WalkthroughThe pull request introduces modifications across multiple Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dvargaslantana
changed the title
dvargaslantana
requested review from
edward-miller-lcg,
amphillipsLGC,
arianamihailescu and
MontaltoNick
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
DotNet/Shared/Shared.csproj (1)
21-21: LGTM! Consider sorting package references alphabetically.The addition of the AspNetCore.HealthChecks.Kafka package (version 8.0.1) aligns perfectly with the PR objective of adding Kafka health checks to .NET services. The package version is compatible with the project's target framework (.NET 8.0), and there are no apparent conflicts with other package references.
As a minor suggestion for improved readability and consistency, consider sorting all package references alphabetically.
DotNet/Submission/Program.cs (2)
153-160: Health check configuration looks good, consider adding error handling.The Kafka health check configuration is well-implemented:
- It correctly uses the existing Kafka connection settings.
- The health check is properly added to the service collection.
However, consider adding error handling for the case where the Kafka connection configuration might be missing:
var kafkaConnection = builder.Configuration.GetRequiredSection(KafkaConstants.SectionName).Get<KafkaConnection>(); if (kafkaConnection == null) { throw new InvalidOperationException("Kafka connection configuration is missing."); }This will provide a more informative error message if the configuration is missing.
Line range hint
1-234: Consider adding unit tests for health check functionality.While the changes look good, it's important to ensure that the newly added health check functionality is properly tested. Consider adding unit tests to verify:
- The health check configuration is correctly set up.
- The health check endpoint responds as expected.
- The Kafka health check accurately reflects the state of the Kafka connection.
This will help ensure the reliability of the health check feature and catch any potential issues early.
Would you like assistance in generating sample unit tests for the health check functionality?
DotNet/Tenant/Program.cs (1)
176-180: LGTM: Kafka health check added correctly.The Kafka health check configuration and registration are implemented correctly. Good job on preserving the existing database health check while adding the new Kafka health check.
For improved readability, consider extracting the health checks configuration into a separate method.
Here's a suggested refactor for improved readability:
private static void ConfigureHealthChecks(IServiceCollection services, KafkaConnection kafkaConnection) { var kafkaHealthOptions = new KafkaHealthCheckConfiguration(kafkaConnection, TenantConstants.ServiceName).GetHealthCheckOptions(); services.AddHealthChecks() .AddCheck<DatabaseHealthCheck>("Database") .AddKafka(kafkaHealthOptions); }Then, in the
RegisterServicesmethod, you can call:ConfigureHealthChecks(builder.Services, kafkaConnection);DotNet/Report/Program.cs (1)
172-173: LGTM: Kafka health check options added.The addition of
kafkaHealthOptionsusingKafkaHealthCheckConfigurationis consistent with the PR objective. It correctly uses the existingkafkaConnectionandReportConstants.ServiceNameto configure the health check options.Consider adding a blank line after this declaration for improved readability:
var kafkaHealthOptions = new KafkaHealthCheckConfiguration(kafkaConnection, ReportConstants.ServiceName).GetHealthCheckOptions(); +DotNet/Shared/Application/Health/KafkaHealthConfiguration.cs (2)
18-40: Add XML documentation comments to public method.Adding XML documentation comments to the
GetHealthCheckOptionsmethod will enhance maintainability and provide better IntelliSense support for consumers of this class.Example:
/// <summary> /// Constructs and returns the Kafka health check options. /// </summary> /// <returns>A configured <see cref="KafkaHealthCheckOptions"/> instance.</returns> public KafkaHealthCheckOptions GetHealthCheckOptions() { // Method implementation... }
7-46: Add XML documentation comments to the class.Including XML documentation for the
KafkaHealthCheckConfigurationclass will improve code readability and assist other developers in understanding the purpose and usage of the class.Example:
/// <summary> /// Provides configuration settings for Kafka health checks. /// </summary> public class KafkaHealthCheckConfiguration { // Class implementation... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- DotNet/Account/Program.cs (2 hunks)
- DotNet/Audit/Program.cs (2 hunks)
- DotNet/Census/Program.cs (2 hunks)
- DotNet/DataAcquisition/Program.cs (2 hunks)
- DotNet/Normalization/Program.cs (2 hunks)
- DotNet/Notification/Program.cs (2 hunks)
- DotNet/QueryDispatch/Program.cs (2 hunks)
- DotNet/Report/Program.cs (2 hunks)
- DotNet/Shared/Application/Health/KafkaHealthConfiguration.cs (1 hunks)
- DotNet/Shared/Shared.csproj (1 hunks)
- DotNet/Submission/Program.cs (3 hunks)
- DotNet/Tenant/Program.cs (2 hunks)
- docker-compose.yml (1 hunks)
🔇 Additional comments (40)
DotNet/Submission/Program.cs (2)
8-8: LGTM: Health check namespace added.The new using statement for
LantanaGroup.Link.Shared.Application.Healthis correctly added and necessary for the health check functionality being introduced.
230-234: Health check middleware setup looks good, verify CORS policy.The health check middleware is well-configured:
- The
/healthendpoint is correctly mapped.- Using
UIResponseWriter.WriteHealthCheckUIResponseensures consistent health check responses.- Requiring a CORS policy is good for security.
However, please verify that the "HealthCheckPolicy" is defined elsewhere in the application. If not, you should define it or use an existing policy. For example:
app.UseCors(policy => policy .WithOrigins("http://example.com") .AllowAnyMethod() .AllowAnyHeader() .AllowCredentials()); app.MapHealthChecks("/health", new HealthCheckOptions { ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse }).RequireCors(policy);Replace
"http://example.com"with the appropriate origin(s) for your application.DotNet/Tenant/Program.cs (2)
33-33: LGTM: New using directive added for health checks.The addition of
using LantanaGroup.Link.Shared.Application.Health;is appropriate for implementing Kafka health checks.
Line range hint
1-280: Summary: Kafka health check successfully implemented.The changes in this PR successfully add Kafka health checks to the .NET Tenant service, aligning with the PR objective (LNK-2883). The implementation is correct and well-integrated with the existing health check system.
Key points:
- Appropriate using directive added for health checks.
- Kafka health check configuration and registration implemented correctly.
- Existing health checks (e.g., database) preserved.
The code changes are minimal and focused, which is good for maintainability. The suggested refactoring for improved readability is optional but could enhance the code structure.
Overall, this PR achieves its goal effectively and can be approved after addressing the minor suggestion if desired.
DotNet/Audit/Program.cs (4)
45-45: LGTM: New using directive added for Kafka health checks.The addition of
using LantanaGroup.Link.Shared.Application.Health;is appropriate for implementing Kafka health checks.
185-186: LGTM: Kafka health check configuration added.The addition of Kafka health check configuration using
KafkaHealthCheckConfigurationis appropriate. It correctly uses the existingkafkaConnectionandAuditConstants.ServiceNameto create the configuration.
188-189: LGTM: Kafka health check added to health checks registration.The addition of the Kafka health check using
AddKafka(kafkaHealthOptions)alongside the existing database health check is appropriate. This enhances the application's health monitoring capabilities.
45-45: Summary: Kafka health check implementation is well-integrated and follows best practices.The changes to add Kafka health checks to the application are well-implemented:
- The necessary using directive is added.
- Kafka health check configuration is properly set up.
- The health checks registration is updated to include the Kafka health check.
These changes enhance the application's health monitoring capabilities without disrupting the existing functionality.
Also applies to: 185-189
DotNet/Account/Program.cs (4)
24-24: LGTM: New using directive for health checks.The addition of
using LantanaGroup.Link.Shared.Application.Health;is appropriate for implementing Kafka health checks.
192-193: LGTM: Kafka health check options configured.The
kafkaHealthOptionsvariable is correctly instantiated usingKafkaHealthCheckConfigurationwith appropriate parameters. This setup will enable Kafka-specific health checks.
195-196: LGTM: Kafka health check added to configuration.The health checks configuration has been correctly updated to include both the existing database check and the new Kafka health check. This ensures comprehensive monitoring of critical system components.
192-196: Verify the impact of Kafka health checks on system monitoring.The implementation of Kafka health checks looks good. To ensure it functions as expected:
- Confirm that the Kafka health check doesn't negatively impact the existing health check performance.
- Verify that the health check endpoint (
/health) now includes Kafka status.- Test the behavior when Kafka is unavailable to ensure proper error handling.
To assist with verification, you can run the following script:
This script will help you verify the health check implementation and ensure that Kafka health status is properly included in the response.
DotNet/Normalization/Program.cs (5)
42-42: LGTM: New using directive for health checks.The addition of
using LantanaGroup.Link.Shared.Application.Health;is appropriate for implementing Kafka health checks.
215-215: LGTM: Kafka connection retrieval.The Kafka connection is correctly retrieved from the configuration using
GetRequiredSection, which ensures the section exists. This is good practice for fail-fast behavior.
216-216: LGTM: Kafka health check options creation.The Kafka health check options are correctly created using the
KafkaHealthCheckConfigurationclass, which centralizes the configuration and makes it easier to manage.
218-221: LGTM: Kafka health check added.The Kafka health check is correctly added to the existing health checks using the
AddKafkamethod with the previously createdkafkaHealthOptions. This addition enhances the service's health monitoring capabilities.
42-42: Summary: Kafka health checks successfully implemented.The changes effectively implement Kafka health checks for the Normalization service:
- Necessary using directive added.
- Kafka connection retrieved from configuration.
- Kafka health check options created with appropriate parameters.
- Kafka health check added to the existing health checks.
These changes enhance the service's health monitoring capabilities by including Kafka alongside the existing database health check. The implementation is clean, follows good practices, and aligns well with the PR objective.
Also applies to: 215-221
DotNet/Census/Program.cs (4)
47-47: LGTM: Import statement for Kafka health check added.The new import statement for
LantanaGroup.Link.Shared.Application.Healthis correctly placed and necessary for the Kafka health check functionality.
187-188: LGTM: Kafka health check configuration added.The Kafka health check configuration is correctly set up using the existing
kafkaConnectionandCensusConstants.ServiceName. The variable namekafkaHealthOptionsis clear and descriptive.
190-191: LGTM: Kafka health check added to health checks builder.The Kafka health check is correctly added to the existing health checks configuration using the
kafkaHealthOptionscreated earlier. It's appropriately chained after the database health check.
47-47: Summary: Kafka health check successfully implemented.The changes in this file successfully implement the Kafka health check as per the PR objective. The implementation includes:
- Adding the necessary import statement.
- Creating a Kafka health check configuration.
- Integrating the Kafka health check into the existing health checks builder.
These changes enhance the application's health monitoring capabilities by including Kafka connectivity in the health checks. The implementation is clean, well-integrated, and follows good practices.
Also applies to: 187-191
DotNet/Notification/Program.cs (4)
41-41: LGTM: Import statement for Kafka health check added.The import of
LantanaGroup.Link.Shared.Application.Healthis correctly added and necessary for implementing the Kafka health check.
202-203: LGTM: Kafka health check configuration added.The Kafka health check configuration is correctly implemented using the existing
kafkaConnectionand the service name constant. This approach ensures consistency and allows for centralized configuration.
205-206: LGTM: Kafka health check added to the service configuration.The Kafka health check is correctly added to the existing health checks configuration. The database health check is preserved, and the new Kafka health check is properly integrated using the
AddKafkamethod with the previously createdkafkaHealthOptions.
41-41: Summary: Kafka health check successfully implemented.The changes in this file successfully implement the Kafka health check as per the PR objective. The implementation includes:
- Adding the necessary import statement.
- Creating a Kafka health check configuration.
- Integrating the Kafka health check into the existing health checks service.
The changes are well-structured, follow good practices, and maintain consistency with the existing codebase.
Also applies to: 202-206
DotNet/Report/Program.cs (3)
20-20: LGTM: New using statement for health checks.The addition of
using LantanaGroup.Link.Shared.Application.Health;is consistent with the PR objective of adding Kafka health checks to the .NET services.
175-176: LGTM: Kafka health check added to the configuration.The addition of
.AddKafka(kafkaHealthOptions)to the health checks configuration is correct and consistent with the PR objective. It's properly chained to the existing health checks and uses thekafkaHealthOptionscreated earlier.
20-20: Summary: Kafka health checks successfully implemented.The changes in this PR successfully implement Kafka health checks for the .NET services as intended. The implementation includes:
- Adding the necessary using statement for health-related functionality.
- Creating Kafka health check options using the existing Kafka connection and service name.
- Adding the Kafka health check to the existing health checks configuration.
These changes enhance the service's health monitoring capabilities without affecting the existing functionality. The implementation is clean, focused, and follows good practices.
Also applies to: 172-176
DotNet/DataAcquisition/Program.cs (4)
54-54: LGTM: New using directive for Kafka health checks.The addition of
using LantanaGroup.Link.Shared.Application.Health;is appropriate for implementing Kafka health checks.
342-344: LGTM: Kafka health check configuration setup.The Kafka health check configuration is properly set up using the correct connection settings and service name.
346-347: LGTM: Health checks configuration updated with Kafka.The health checks configuration has been correctly updated to include both the existing DbContextCheck and the new Kafka health check.
54-54: Summary: Kafka health checks successfully implemented.The changes effectively implement Kafka health checks in the Data Acquisition service:
- Added the necessary using directive for Kafka health checks.
- Set up the Kafka health check configuration using the correct connection settings.
- Updated the health checks configuration to include both existing and new Kafka checks.
These changes enhance the service's health monitoring capabilities by including Kafka connectivity in the health checks.
Also applies to: 342-347
docker-compose.yml (4)
267-267: LGTM! Consider clarifying the retention period.The addition of the "Service-Healthcheck" topic aligns with the PR objective. However, the retention period of 1ms is extremely short. While this might be intentional for health check purposes, it's worth confirming if this is the desired behavior.
Could you please clarify the reasoning behind the 1ms retention period for the "Service-Healthcheck" topic?
Line range hint
232-237: LGTM! Healthcheck addition improves service reliability.The addition of the healthcheck to the
kafka_bservice is a good improvement. It ensures that the Kafka broker is functioning correctly by verifying that it can list topics. The frequency, timeout, and retry settings are reasonable for a healthcheck.🧰 Tools
🪛 yamllint
[error] 269-269: trailing spaces
(trailing-spaces)
267-267: Verify usage of the new health check topic in .NET services.The new "Service-Healthcheck" topic has been added, but it's not clear which services will be using it. To ensure full implementation of the Kafka health check for .NET services (as mentioned in the PR objective), it would be beneficial to verify that the relevant .NET services are configured to use this new topic.
Could you please confirm which .NET services are expected to use the new "Service-Healthcheck" topic and ensure they are properly configured to do so?
Line range hint
1-267: Summary: Kafka health check implementation looks good, with a few points to verify.The changes to add Kafka health check capabilities to the .NET services have been implemented in the
docker-compose.ymlfile. The main modifications include:
- Adding a new "Service-Healthcheck" topic in the
kafka_initservice.- Implementing a healthcheck for the
kafka_bservice.These changes align with the PR objective (LNK-2883: Added Kafka health check to .NET services). However, there are a few points that require attention:
- The retention period of 1ms for the "Service-Healthcheck" topic is unusually short. Please confirm if this is intentional.
- Verify which .NET services are expected to use the new "Service-Healthcheck" topic and ensure they are properly configured to do so.
Once these points are addressed, the implementation looks solid and should improve the overall reliability of the Kafka-related services in the system.
🧰 Tools
🪛 yamllint
[error] 269-269: trailing spaces
(trailing-spaces)
DotNet/Shared/Application/Health/KafkaHealthConfiguration.cs (1)
42-45: Ensure_serviceNameis not null or empty inMessageBuilder.The
MessageBuildermethod uses_serviceNameas the message key. If_serviceNameisnullor empty, it could lead to unexpected behavior. Since you've added null checks in the constructor, this should be safe, but it's good to be cautious.DotNet/QueryDispatch/Program.cs (3)
48-49: Correct inclusion of namespaces for Kafka health checks.The added using directives
HealthChecks.KafkaandLantanaGroup.Link.Shared.Application.Healthare necessary for implementing Kafka health checks.
248-249: Proper initialization of Kafka health check options.The
kafkaHealthOptionsare correctly instantiated usingKafkaHealthCheckConfiguration, providing the necessary configuration for Kafka health checks.
251-252: Kafka health checks are successfully integrated into the health check pipeline.Adding
.AddKafka(kafkaHealthOptions)ensures that Kafka's health status is included in the application's health checks.
Also added a missing health check endpoint to the Submission service.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These enhancements improve the application's reliability and monitoring of Kafka services, ensuring better operational oversight.