fix: Refactor probes and add more health checks #1159
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the health check capabilities of the application. It adds several package references related to health checks and dependency injection. New health check classes for well-known endpoints and Redis are implemented, while the infrastructure is updated to integrate health checks for both Redis and PostgreSQL. Additionally, the application's Changes
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
Documentation and Community
|
oskogstad
changed the title
arealmaas
changed the title
arealmaas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (4)
- src/Digdir.Domain.Dialogporten.Infrastructure/Digdir.Domain.Dialogporten.Infrastructure.csproj (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs (3 hunks)
Additional comments not posted (9)
src/Digdir.Domain.Dialogporten.Infrastructure/Digdir.Domain.Dialogporten.Infrastructure.csproj (4)
4-4: LGTM!The
AspNetCore.HealthChecks.NpgSqlpackage reference is a good addition for implementing health checks with PostgreSQL database. The version8.0.2is a stable release.
5-5: LGTM!The
AspNetCore.HealthChecks.Redispackage reference is a good addition for implementing health checks with Redis cache. The version8.0.1is a stable release.
7-7: LGTM!The
AspNetCore.HealthChecks.UI.Clientpackage reference is a good addition for integrating with the health checks UI. The version8.0.1is a stable release.
13-13: LGTM!The
Microsoft.Extensions.DependencyInjectionpackage reference is a good addition for dependency injection, which is typically used for registering health checks. The version8.0.0is a stable release.src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (3)
187-188: LGTM!Adding a dedicated HTTP client for health checks is a good practice to isolate health check traffic from the application's main HTTP traffic. The handler lifetime of 10 seconds is reasonable for managing resources efficiently.
190-205: Comprehensive health checks added.The addition of health checks for Redis, PostgreSQL, and well-known endpoints enhances the monitoring capabilities of the application. The configuration of each health check with a name, failure status, and tags is well-structured and allows for granular monitoring.
Using the
Unhealthyfailure status for all health checks is appropriate to indicate a critical issue when a health check fails. The tags "dependencies" and "auth" enable filtering and categorization of health check results.
33-37: Required using statements added.The addition of the using statements for health check-related namespaces is necessary for the implementation of health checks. They import the required types and namespaces for configuring health checks in the
InfrastructureExtensionsclass.src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
135-136: Correctly adding health checks service and self-check.The health checks service is properly configured, and the self-check with the tag
"self"is appropriately added.
211-215: Appropriate configuration of liveness endpoint.The
/livenessendpoint is correctly mapped to the"self"health check, which is suitable for liveness probes.
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
…lKnownHealthChecks.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…lKnownHealthChecks.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (3)
- src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs (3 hunks)
Additional comments not posted (5)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (3)
135-136: Self health check registration is correct.The 'self' health check is properly registered with the tag
"self", which is used by the/livenessendpoint. This ensures that the liveness probe can correctly assess the application's basic health.
206-210: Dependency health checks are appropriately configured for startup.The
/startupendpoint is configured to report health checks tagged with"dependencies". Assuming that your critical dependencies (e.g., Redis, PostgreSQL) are registered with this tag elsewhere (e.g., inInfrastructureExtensions.cs), this setup will help in verifying that all essential services are running during application startup.
216-220: Verify registration of critical health checks for readiness probe.The
/readinessendpoint reports health checks tagged with"critical". Please ensure that all critical health checks are registered with the"critical"tag so that the readiness probe accurately reflects the application's ability to handle requests.Run the following script to confirm the registration of health checks with the
'critical'tag:src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (2)
194-209: Health checks are well-configured and enhance monitoring capabilities.The addition of health checks for Redis, PostgreSQL, and Well-Known Endpoints enhances the application's monitoring and reliability. The configurations use appropriate failure statuses and tags, which aids in categorizing and responding to issues effectively.
187-192: Verify the usage ofHealthCheckClientin the codebase.Ensure that the
"HealthCheckClient"is being utilized appropriately within your health checks or elsewhere in the application. If it is not used, it might be unnecessary to configure thisHttpClient.Run the following script to search for usages of
"HealthCheckClient":Verification successful
HealthCheckClient is appropriately utilized within the HealthChecks.
- Referenced in
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.csScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of 'HealthCheckClient' in the codebase. # Test: Find all occurrences of 'HealthCheckClient'. # Expect: References where 'HealthCheckClient' is used. rg 'HealthCheckClient'Length of output: 307
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
…lKnownHealthChecks.cs
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs (1 hunks)
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs (4)
22-24: Constructor arguments are properly validatedThe constructor correctly checks for null arguments and assigns the dependencies.
31-34: Filter out null or empty URLs from the endpoints list
42-42: Ensure the HttpClient is configured with appropriate timeouts
57-66: Aggregate exceptions to check all endpoints before determining health status
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Digdir.Domain.Dialogporten.Infrastructure.csproj
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/WellKnownHealthChecks.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range comments (4)
.azure/modules/containerApp/main.bicep (1)
Line range hint
69-69: LGTM: Correct usage of the new probes parameterThe
probesparameter is correctly used in thecontainerAppresource definition, allowing for dynamic configuration of health checks.Consider adding schema validation for probes
While the current implementation is flexible, it might be beneficial to add a schema definition or validation for the
probesarray. This could help prevent potential misconfigurations and ensure that the provided probe configurations are valid.Consider adding a schema definition for the
probesparameter. You can use the@allowed()decorator to define the allowed properties for each probe. Here's an example:@description('The probes for the container app') @allowed([ { type: 'Liveness' httpGet: { path: string port: int } initialDelaySeconds: int periodSeconds: int } { type: 'Readiness' httpGet: { path: string port: int } initialDelaySeconds: int periodSeconds: int } ]) param probes array = []This schema ensures that each probe in the array has the required properties and correct types.
.azure/applications/web-api-eu/main.bicep (1)
Line range hint
86-129: Overall assessment: Excellent enhancement of health check capabilitiesThe changes introduced in this file significantly improve the health monitoring capabilities of the deployed web API application. Key improvements include:
- Centralized port configuration for easier maintenance.
- Comprehensive probe configurations for Liveness, Readiness, and Startup checks.
- Integration of the new probes into the container app deployment.
These modifications align well with the PR objectives and follow best practices in infrastructure-as-code and container orchestration. They should result in more robust and granular health monitoring for the application.
Consider documenting these health check endpoints and their expected behaviors in your application's operational documentation. This will help operations teams understand how to interpret the health check results and respond to potential issues.
docs/schema/V1/swagger.verified.json (2)
Line range hint
2645-2668: Updated schemaOperationOfUpdateDialogDtoThe
OperationOfUpdateDialogDtoschema is well-defined and includes all necessary properties for a JSON Patch operation. The addition of theoperationTypeproperty alongside theopproperty provides flexibility in operation type representation.Suggestion for improvement:
- Consider adding descriptions to each property to clarify their purposes, especially the relationship between
opandoperationType.
Line range hint
5476-5580: Updated PATCH endpoint for dialogsThe PATCH endpoint for dialogs has been correctly updated to use the new
JsonPatchDocumentOfUpdateDialogDtoschema in the request body. This change ensures consistency with the newly added schemas and enables JSON Patch functionality for dialog updates.Suggestion for improvement:
- Consider adding an example request body to the endpoint documentation to illustrate how to use the new JSON Patch document structure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (12)
- .azure/applications/web-api-eu/main.bicep (2 hunks)
- .azure/applications/web-api-so/main.bicep (2 hunks)
- .azure/modules/containerApp/main.bicep (1 hunks)
- docs/schema/V1/swagger.verified.json (4 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Digdir.Domain.Dialogporten.Infrastructure.csproj (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (3 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj (1 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs (4 hunks)
- src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj (1 hunks)
- src/Digdir.Library.Utils.AspNet/HealthCheckExtensions.cs (1 hunks)
- src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs (1 hunks)
🔇 Additional comments (21)
src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj (2)
1-1: LGTM: Correct SDK declaration.The project is correctly set up using the Microsoft.NET.Sdk, which is appropriate for a .NET library project.
1-13: Overall, the project file is well-structured and appropriate for the intended purpose.The
Digdir.Library.Utils.AspNet.csprojfile is correctly set up as a .NET library project with the necessary configurations and dependencies for implementing health checks. The inclusion of XML documentation generation and relevant package references aligns well with the PR objectives.Consider the following minor improvements:
- Add XML comments to all public members instead of suppressing the warnings.
- Align the versions of the package references if possible.
These changes will enhance the maintainability and consistency of the project.
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs (2)
1-7: LGTM: Imports and namespace declaration are appropriate.The necessary namespaces for health checks and Redis are correctly imported, and the class is placed in an appropriate namespace following the project structure.
1-32: Overall, this is a well-implemented Redis health check with room for minor improvements.The
RedisHealthCheckclass is concise, focused, and correctly implements theIHealthCheckinterface. It provides a solid foundation for monitoring Redis health in the application.Summary of suggested improvements:
- Add input validation for the connection string in the constructor.
- Implement more specific exception handling in the
CheckHealthAsyncmethod.- Ensure proper disposal of the
ConnectionMultiplexerto prevent resource leaks.These enhancements will further improve the robustness and reliability of the health check implementation.
src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj (1)
24-24: LGTM! Verify the referenced project exists.The addition of the
Digdir.Library.Utils.AspNetproject reference aligns with the PR objectives of enhancing health checks. This utility library for ASP.NET could provide valuable tools for implementing the new health check mechanisms.To ensure the referenced project exists and is correctly located, run the following script:
✅ Verification successful
Referenced project exists.
The
Digdir.Library.Utils.AspNetproject reference has been verified and exists atsrc/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj. This addition aligns with the PR objectives of enhancing health checks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the Digdir.Library.Utils.AspNet project # Test: Check if the project file exists if fd -p "Digdir.Library.Utils.AspNet.csproj$"; then echo "Project file found." else echo "Project file not found. Please verify the project reference." fiLength of output: 161
.azure/modules/containerApp/main.bicep (1)
31-32: LGTM: Well-defined new parameter for probesThe new
probesparameter is well-defined with a clear description and appropriate type (array). The default empty array makes it optional, which is a good practice for backward compatibility and flexibility..azure/applications/web-api-eu/main.bicep (3)
86-87: LGTM: Port variable additionThe addition of the
portvariable is a good practice. It centralizes the port configuration, making it easier to maintain and update if needed. The value 8080 is consistent with theASPNETCORE_URLSenvironment variable set earlier in the file.
88-116: LGTM: Probes configuration looks goodThe
probesvariable is well-structured and provides comprehensive health check configurations for Liveness, Readiness, and Startup probes. This setup allows for granular health monitoring of the application.A few observations:
- The use of different paths for each probe type is a good practice.
- The configuration is consistent across all probes, which aids in maintainability.
- The
portvariable is used consistently, which is excellent.Please verify that the timing settings (periodSeconds: 5, initialDelaySeconds: 2) are appropriate for your application's startup and response times. These may need adjustment based on the specific behavior of your web API.
129-129: LGTM: Probes integration in containerApp moduleThe addition of the
probesparameter to thecontainerAppmodule is appropriate and necessary. This change ensures that the newly defined health check probes are integrated into the container app configuration during deployment..azure/applications/web-api-so/main.bicep (3)
90-91: LGTM: Port variable additionThe addition of the
portvariable is a good practice. It centralizes the port configuration, making it easier to manage and update if needed. This also aligns well with the existingASPNETCORE_URLSenvironment variable.
133-134: LGTM: Container app module parameter updatesThe addition of the
probesandportparameters to thecontainerAppmodule is appropriate and necessary. This change ensures that the newly defined health check probes and the port configuration are correctly applied during the container app deployment.These updates maintain consistency with the earlier variable definitions and effectively integrate the health check configurations into the deployment process.
Line range hint
90-134: Overall assessment: Excellent enhancements to deployment configurationThe changes introduced in this file significantly improve the deployment configuration:
- The addition of the
portvariable centralizes the port configuration.- The comprehensive
probesconfiguration enhances the application's health monitoring capabilities.- The updates to the
containerAppmodule parameters ensure proper integration of these new configurations.These changes will contribute to improved reliability, observability, and maintainability of the deployed application. The only suggestion for improvement is minor and relates to the structure of the
probesvariable for easier maintenance.Great work on enhancing the deployment script!
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (5)
26-26: LGTM: New using statement added.The addition of
using Digdir.Library.Utils.AspNet;is appropriate and likely related to the new health check configuration.
127-132: LGTM: Improved service configuration structure.The changes enhance readability by separating different types of service configurations. The addition of
.AddAuthorization()is a good security practice.
Line range hint
181-199: LGTM: Improved Swagger configuration with dynamic base URI.The changes to the Swagger configuration ensure that the correct base URL is used for the API documentation. Retrieving the
dialogportenBaseUrifrom the configuration is a good practice, allowing for flexibility across different environments.
Line range hint
1-226: Summary: Overall improvements to health checks and Swagger configuration.The changes in this file primarily focus on enhancing the health check configuration and improving the Swagger documentation setup. These modifications contribute to better maintainability and more accurate API documentation. The new health check configuration provides a more structured approach, while the Swagger changes ensure correct base URL usage across different environments.
A few minor suggestions were made for further improvement:
- Consider extracting the health check configuration path to a constant or configuration value.
- Verify the new health check endpoints to ensure they're working as expected.
Overall, these changes represent a positive step towards a more robust and well-documented API.
208-209: LGTM: Updated health check mapping.The change from a direct
/healthzmapping toapp.MapAspNetHealthChecks()is consistent with the new health check configuration. This likely provides more comprehensive health check endpoints.To ensure the new health check endpoints are working as expected, please run the following verification script:
Please review the results to ensure that all expected health check endpoints are accessible and returning appropriate status codes.
docs/schema/V1/swagger.verified.json (1)
Line range hint
1-6487: Summary of JSON Patch functionality additionsThe changes introduced in this OpenAPI specification successfully implement JSON Patch functionality for dialog updates. Key improvements include:
- Addition of
IContractResolverschema- Introduction of
JsonPatchDocumentOfUpdateDialogDtoschema- Update of
OperationOfUpdateDialogDtoschema- Modification of the PATCH endpoint for dialogs to use the new JSON Patch document schema
These changes enhance the API's capabilities for partial updates to dialogs, providing a more flexible and standardized approach using JSON Patch.
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (3)
36-37: Approved: Added necessary using directivesThe inclusion of
Microsoft.Extensions.Diagnostics.HealthChecksandDigdir.Domain.Dialogporten.Infrastructure.HealthChecksnamespaces is appropriate for implementing health checks.
204-204: Approved: Integrated health checks into the service collectionThe call to
services.AddHealthChecks(infrastructureSettings);correctly integrates the health checks configuration into the service collection.
228-230: Verify health check tags for consistencyThe health checks for Redis and PostgreSQL are registered with tags
["dependencies", "redis"]and["dependencies", "critical"]respectively. Ensure that these tags align with the project's tagging conventions for health checks.
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Outdated
Show resolved
Hide resolved
…isHealthCheck.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs (4 hunks)
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs (1)
13-16: Previous review comment regarding input validation remains applicable.
arealmaas
changed the title
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range comments (1)
docs/schema/V1/swagger.verified.json (1)
Line range hint
1-6460: Overall improvement in API design and functionalityThe changes made to the Dialogporten API specification, particularly the introduction of the
JsonPatchDocumentOfUpdateDialogDtoschema and its integration with the PATCH operation for dialogs, represent a significant enhancement in the API's design and functionality. These updates align the API more closely with RESTful best practices and standards, improving its flexibility, efficiency, and robustness.Key improvements:
- Standardized partial updates using RFC6902 JSON Patch.
- Implementation of optimistic concurrency control.
- More granular and flexible dialog update mechanism.
To maximize the benefits of these changes:
- Ensure comprehensive documentation is provided for the new update mechanism.
- Consider creating migration guides for existing API consumers.
- Update any related client libraries or SDKs to support the new JSON Patch-based updates.
- Monitor API usage after deployment to ensure smooth adoption of the new update mechanism.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (6)
- docs/schema/V1/swagger.verified.json (2 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (4 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs (4 hunks)
- src/Digdir.Library.Utils.AspNet/HealthCheckExtensions.cs (1 hunks)
- src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs (1 hunks)
🔇 Additional comments (6)
src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs (1)
11-76: Well-structured implementation ofEndpointsHealthCheckThe
EndpointsHealthCheckclass is well-implemented, effectively performing health checks on configured endpoints. The use ofIHttpClientFactoryand proper logging enhances scalability and maintainability.src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
137-137:⚠️ Potential issuePossible Typo: 'JwtBearerTokenSchemas' should be 'JwtBearerTokenSchemes'
On line 137, the configuration section path is set to
"WebApi:Authentication:JwtBearerTokenSchemas". The term"Schemas"might be a typo, as"Schemes"is commonly used in authentication configurations. Please verify that this path matches your actual configuration.To confirm which term is used in your configuration files, you can run the following commands:
This will help determine the correct configuration section path to use.
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (4)
36-37: Using directives added appropriatelyThe added using directives for
Microsoft.Extensions.Diagnostics.HealthChecksandDigdir.Domain.Dialogporten.Infrastructure.HealthChecksare necessary for the implementation of the custom health checks.
204-204: Integration of custom health checksThe invocation of
services.AddCustomHealthChecks();successfully integrates the custom health checks into the infrastructure configuration.
228-229: Ensure proper disposal ofRedisHealthCheckIf
RedisHealthCheckimplementsIDisposable, registering it as a singleton without a factory method allows the DI container to manage its lifetime properly. Confirm that this is intentional and that resource management is handled correctly.
221-234: Consider adding retry policies to theHealthCheckClientTo enhance resilience against transient network issues, consider adding a retry policy from the policy registry to the
HealthCheckClient.
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs
Show resolved
Hide resolved
src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Outdated
Show resolved
Hide resolved
…isHealthCheck.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…eck.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Show resolved
Hide resolved
|
Konge 👍 |
src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs
Outdated
Show resolved
Hide resolved
| app.MapHealthChecks("/healthz"); | ||
|
|
||
| // Map Health Checks | ||
| app.MapAspNetHealthChecks(); |
There was a problem hiding this comment.
Denne burde vel være litt lenger opp i middleware stacken. Når jeg ser på dette, burde egentlig både swagger og health checks være over UseFastEndpoints 🤔 Altså:
.MapAspNetHealthChecks
.UseAddSwaggerCorsHeader
.UseSwaggerGen
.UseFastEndpoints
.MapControllers
| return services; | ||
| } | ||
|
|
||
| public static void MapAspNetHealthChecks(this WebApplication app) |
There was a problem hiding this comment.
Kunne vi lagt alle health check endpoints under /health/{healthCheck}? Eks /health/startup slik at vi er eksplisitte i hva dette dreier seg om - nemmelig health.
| { | ||
| app.MapHealthChecks("/startup", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("dependencies"), |
There was a problem hiding this comment.
Ser ut som at det er en 1:1 mellom et spesifikt health endepunkt og en label. I mitt hode hadde det vært fornuftig å kalle taggen noe i samme duren som hvilket endepunkt hvor det skal brukes. Eks startup endepunktet burde lete etter startup taggen.
There was a problem hiding this comment.
Jeg tok feil. "dependencies" er faktisk brukt i både health og startup. Da er det neste spørsmålet, hvorfor har vi både health og startup?
| { | ||
| app.MapHealthChecks("/startup", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("dependencies"), |
There was a problem hiding this comment.
Hadde vært fint om du la til disse taggene som konstanter. 🙏
There was a problem hiding this comment.
Trenger ikke gjøre det med endepunktene
There was a problem hiding this comment.
Yes. Brukes både i infrastructure og webapi, så kanskje skille det ut i et lib? Evt. deklarere konstanter i begge prosjektene da.
There was a problem hiding this comment.
Hmm, det er jo et godt poeng. Dersom vi eksponerer det i en public klasse her er det kun tilgjengelig for WebApi og ikke infrastruktur. Det "riktigste" hadde vært å lage en Digdir.Library.Utils.Abstractions pakke som WebApi hadde brukt implisitt gjennom Digdir.Library.Utils.AspNet og Infrastruktur hadde brukt eksplistt. Men det er kanskje overkill for en fil? 🤔 Et annet alternativ er å gjøre et kompromiss. public static class i Digdir.Library.Utils.AspNet som WebApi bruker, og magic strings i Infrastruktur. Jeg lar det være opp til deg 😬
| public string WellKnown { get; set; } = string.Empty; | ||
| } | ||
|
|
||
| public static IServiceCollection AddAspNetHealthChecks(this IServiceCollection services, IConfiguration configuration, Action<AspNetHealthChecksOptions> configure) |
There was a problem hiding this comment.
Her må konfigurasjon registreringen og ansvarsfordelingen masseres litt mer. Vi tar inn IConfiguration og en configure action for en spesifikk konfigurajson. Mye på rett spor, men disse pleier å ha en XOR relasjon i slike kontekster. Personlig liker jeg at pakker har en nullable configure action fremfor å ta inn hele IConfiguration. Det er fordi pakker fort får litt for mye "power" med hele IConfiguration og skjuler de faktiske avhengighetene for konsumenten av pakken. Hvilken del av konfigurasjonen konsumeres av pakken? Hvilken del er påkrevd? Hvilken del er optional? Mye implisitt. Det er selvfølgelig unntak til denne "regelen". Men det er mer forbeholdt store bibliotek med god config dokumentasjon Eks serilog (EDIT: IMAO).
En nullable configure action gjør at konsumenter kan gi eksplisitt konfigurasjon til pakker, enten via configure action eller via interfacene IConfigureOptions, IPostConfigureOptions, eller deres "named" varianter.
Se Options pattern guidance for .NET library authors hvis it tickles your fancy.
Det vi faktisk er interessert i av konfigurasjon her er kun well known url endepunktene. Men av det jeg forstår av koden er det verken påkrevd, eller spesifikk til well known endepunkt - kun http get endepunkt. Dette kan vi eksponere til konsumenten via en settings objekt med en liste av string eller urler:
public class AspNetHealthChecksSettings
{
public List<string>? HttpGetEndpointsToCheck { get; set; }
}Men har ikke altinn autorisasjon sin egen health check som vi kunne sjekke i stede for å sjekke spesifikke get endepunkt?
There was a problem hiding this comment.
Hører med ulike teams her om de har et eget health-check-endpoint
There was a problem hiding this comment.
En nullable configure action gjør at konsumenter kan gi eksplisitt konfigurasjon til pakker, enten via configure action eller via interfacene IConfigureOptions, IPostConfigureOptions, eller deres "named" varianter.
Agree!
|
|
||
| public static class HealthCheckExtensions | ||
| { | ||
| public class AspNetHealthChecksOptions |
There was a problem hiding this comment.
Denne trenger ikke være nested, men kan være sealed. De fleste settings/options klasser i DP postfikses med "Settings" fremfor "Options".
| public string WellKnownEndpointsConfigurationSectionPath { get; set; } = string.Empty; | ||
| } | ||
|
|
||
| public class JwtBearerTokenSchema |
There was a problem hiding this comment.
Dette trenger ikke pakken å vite om.
| if (_endpoints.Count == 0) | ||
| { | ||
| _logger.LogWarning("No endpoints provided."); | ||
| return HealthCheckResult.Unhealthy("No endpoints are configured."); |
There was a problem hiding this comment.
Skal vi tvinge konsumenten av pakka å ha downstream endepunkt å sjekke mot?
There was a problem hiding this comment.
hm, usikker. Om konsumenten bruker pakken, men ikke har spesifisert noen URLer så tyder det kanskje på noe galt? 🤔 Igjen så kan jo det variere på miljø da. Ingen store innvendinger her
There was a problem hiding this comment.
men ikke har spesifisert noen URLer så tyder det kanskje på noe galt?
Er ikke nødvendigvis uenig, men det er ikke noe utils kan ta stilling til. Det er presentasjonslaget som vet hvilke deler av applikasjonen og dermed hvilke downstream sources den er avhengig av.
| } | ||
| } | ||
|
|
||
| if (unhealthyEndpoints.Count > 0) |
There was a problem hiding this comment.
Veldig bra at du batcher opp alle! 🙌
…isHealthCheck.cs Co-authored-by: Ole Jørgen Skogstad <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range comments (3)
docs/schema/V1/swagger.verified.json (3)
Line range hint
3326-3355: Consider removing "Invalid" from OperationType enumThe
OperationTypeenum in theOperationschema includes an "Invalid" value, which is not part of the standard RFC6902 JSON Patch operations. This might lead to confusion in API usage and implementation.Consider removing the "Invalid" value from the enum to strictly adhere to the RFC6902 standard, which defines only Add, Remove, Replace, Move, Copy, and Test operations.
"OperationType": { "description": "", "enum": [ "Add", "Remove", "Replace", "Move", "Copy", "Test", - "Invalid" ], "type": "string", "x-enumNames": [ "Add", "Remove", "Replace", "Move", "Copy", "Test", - "Invalid" ] },
Line range hint
5663-5777: Add examples for PATCH operation request bodyTo improve the clarity of the PATCH operation for dialogs, consider adding examples of valid JSON Patch documents in the operation description or as part of the schema definition. This will help API consumers understand how to correctly format their requests.
You could add an example like this to the request body schema:
"example": [ { "op": "replace", "path": "/status", "value": "Completed" }, { "op": "add", "path": "/content/summary", "value": { "MediaType": "text/plain", "Value": [{ "LanguageCode": "en", "Value": "Updated summary" }] } } ]
Line range hint
1-6463: Include rate limiting informationThe API specification would benefit from including information about rate limiting. This is important for API consumers to understand usage limits and plan their integrations accordingly.
Consider adding a global description or including rate limit headers in the responses of appropriate endpoints. You could use standard headers like
X-RateLimit-Limit,X-RateLimit-Remaining, andX-RateLimit-Reset.Add a global description about rate limiting in the API info section:
"info": { "title": "Dialogporten", "version": "v1", "description": "API for Dialogporten. This API is rate limited. Please see the 'Rate Limiting' section for details." },And include rate limit headers in the response objects of appropriate endpoints:
"responses": { "200": { "headers": { "X-RateLimit-Limit": { "schema": { "type": "integer" }, "description": "The number of allowed requests in the current period" }, "X-RateLimit-Remaining": { "schema": { "type": "integer" }, "description": "The number of remaining requests in the current period" }, "X-RateLimit-Reset": { "schema": { "type": "integer" }, "description": "The number of seconds left in the current period" } }, // ... existing response definition } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- docs/schema/V1/swagger.verified.json (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/HealthChecks/RedisHealthCheck.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (4 hunks)
- src/Digdir.Library.Utils.AspNet/HealthCheckExtensions.cs (1 hunks)
- src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs (1 hunks)
🔇 Additional comments (8)
docs/schema/V1/swagger.verified.json (1)
Line range hint
5663-5777: PATCH operation improvements approvedThe PATCH operation for dialogs has been successfully enhanced to use the RFC6902 JSON Patch standard. The implementation now includes:
- An array of
Operationobjects in the request body, aligning with the JSON Patch standard.- Optimistic concurrency control using the If-Match header.
- Clear documentation mentioning the use of RFC6902.
These changes address the concerns raised in the previous review and significantly improve the robustness and reliability of dialog updates.
src/Digdir.Library.Utils.AspNet/HealthChecks/EndpointsHealthCheck.cs (2)
11-66: Well-implementedEndpointsHealthCheckclassThe
EndpointsHealthCheckclass effectively implements theIHealthCheckinterface and correctly checks the health of specified endpoints. The use ofIHttpClientFactoryand proper logging enhances the robustness of the health check.
68-71:EndpointsHealthCheckOptionsclass is properly definedThe
EndpointsHealthCheckOptionsclass correctly encapsulates the endpoints configuration with appropriate default initialization.src/Digdir.Library.Utils.AspNet/HealthCheckExtensions.cs (3)
13-22: Consider moving nested classes to separate filesThe classes
AspNetHealthChecksOptionsandJwtBearerTokenSchemaare nested within theHealthCheckExtensionsstatic class. For better code organization and maintainability, consider moving these classes to separate files. This adheres to the standard practice of having one public class per file, improving readability, and making future modifications easier.
24-27: Add null checks for method parameters to enhance robustnessThe method
AddAspNetHealthChecksshould validate its input parameters to prevent potentialArgumentNullExceptionifconfigurationorconfigureis null. Implementing null checks improves code safety and provides clearer error messages.Consider applying the following changes:
public static IServiceCollection AddAspNetHealthChecks(this IServiceCollection services, IConfiguration configuration, Action<AspNetHealthChecksOptions> configure) { + if (configuration == null) throw new ArgumentNullException(nameof(configuration)); + if (configure == null) throw new ArgumentNullException(nameof(configure)); var options = new AspNetHealthChecksOptions(); configure(options);
69-71: Health check predicate uses undefined tag "critical"The
/readinessendpoint is configured to select health checks with the tag"critical", but no health checks are registered with this tag. This means the/readinessendpoint will not return any health check results.To resolve this, you can either:
- Add the
"critical"tag to the necessary health checks. For example, if the"Endpoints"health check is critical for readiness, update its registration:healthChecks.AddCheck<EndpointsHealthCheck>( "Endpoints", failureStatus: HealthStatus.Unhealthy, - tags: [DependenciesTag]); + tags: [DependenciesTag, CriticalTag]);
- Change the predicate to use an existing tag. If the
"dependencies"tag is appropriate for readiness checks, modify the predicate:app.MapHealthChecks("/readiness", new HealthCheckOptions { - Predicate = check => check.Tags.Contains("critical"), + Predicate = check => check.Tags.Contains(DependenciesTag), ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse });Ensure that the tags used in your predicates correspond to the tags assigned during health check registration.
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (2)
36-37: LGTMThe added using directives for
Microsoft.Extensions.Diagnostics.HealthChecksandDigdir.Domain.Dialogporten.Infrastructure.HealthChecksare appropriate and necessary for implementing health checks.
204-204: LGTMAdding
services.AddCustomHealthChecks();correctly integrates the custom health checks into the service collection.
| private readonly InfrastructureSettings _settings; | ||
|
|
||
| public RedisHealthCheck(IOptions<InfrastructureSettings> options) | ||
| { | ||
| _settings = options?.Value ?? throw new ArgumentNullException(nameof(options)); | ||
| } |
There was a problem hiding this comment.
Reuse ConnectionMultiplexer instance to improve performance
Creating a new ConnectionMultiplexer on each call to CheckHealthAsync is resource-intensive and can degrade performance. ConnectionMultiplexer is designed to be shared and reused throughout the application's lifetime. Consider injecting a singleton instance of IConnectionMultiplexer via dependency injection.
Apply this diff to modify the constructor and use the injected IConnectionMultiplexer:
- private readonly InfrastructureSettings _settings;
+ private readonly IConnectionMultiplexer _redis;
- public RedisHealthCheck(IOptions<InfrastructureSettings> options)
+ public RedisHealthCheck(IConnectionMultiplexer redis)
{
- _settings = options?.Value ?? throw new ArgumentNullException(nameof(options));
+ _redis = redis ?? throw new ArgumentNullException(nameof(redis));
}Update the CheckHealthAsync method:
- using var redis = await ConnectionMultiplexer.ConnectAsync(_settings.Redis.ConnectionString);
- var db = redis.GetDatabase();
+ var db = _redis.GetDatabase();
await db.PingAsync();This change promotes efficient resource usage by reusing the ConnectionMultiplexer instance. Ensure that the IConnectionMultiplexer is registered as a singleton in your dependency injection container:
services.AddSingleton<IConnectionMultiplexer>(ConnectionMultiplexer.Connect(_settings.Redis.ConnectionString));Also applies to: 23-25
| if (_endpoints.Count == 0) | ||
| { | ||
| _logger.LogWarning("No endpoints provided."); | ||
| return HealthCheckResult.Unhealthy("No endpoints are configured."); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider revising the health status when no endpoints are configured
Currently, when no endpoints are provided (_endpoints.Count == 0), the health check returns an unhealthy status. If having no endpoints to check is acceptable in certain configurations, consider returning a healthy status instead, or adjusting the logic to reflect the intended behavior.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Exception occurred while checking endpoint: {Url}", url); | ||
| unhealthyEndpoints.Add($"{url} (Exception: {ex.GetType().Name})"); | ||
| } |
There was a problem hiding this comment.
Handle OperationCanceledException separately to respect cancellation requests
Catching the general Exception includes OperationCanceledException, which is thrown when the operation is canceled via the CancellationToken. This can prevent the proper propagation of the cancellation request. Consider catching OperationCanceledException separately and rethrowing it to ensure correct task cancellation behavior.
Apply this diff to handle cancellation appropriately:
}
+ catch (OperationCanceledException)
+ {
+ // Properly propagate the cancellation request
+ throw;
+ }
catch (Exception ex)
{
_logger.LogError(ex, "Exception occurred while checking endpoint: {Url}", url);
unhealthyEndpoints.Add($"{url} (Exception: {ex.GetType().Name})");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Exception occurred while checking endpoint: {Url}", url); | |
| unhealthyEndpoints.Add($"{url} (Exception: {ex.GetType().Name})"); | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| // Properly propagate the cancellation request | |
| throw; | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Exception occurred while checking endpoint: {Url}", url); | |
| unhealthyEndpoints.Add($"{url} (Exception: {ex.GetType().Name})"); | |
| } |
| healthChecks.AddCheck("self", () => HealthCheckResult.Healthy(), tags: ["self"]); | ||
|
|
||
| var wellKnownEndpoints = wellKnownSchemas?.Select(schema => schema.WellKnown).ToList() ?? new List<string>(); | ||
|
|
||
| if (wellKnownEndpoints.Count > 0) | ||
| { | ||
| services.Configure<EndpointsHealthCheckOptions>(opts => | ||
| { | ||
| opts.Endpoints = wellKnownEndpoints; | ||
| }); | ||
|
|
||
| healthChecks.AddCheck<EndpointsHealthCheck>( | ||
| "Endpoints", | ||
| failureStatus: HealthStatus.Unhealthy, | ||
| tags: ["dependencies"]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define health check tag strings as constants
Currently, the tag strings "self", "dependencies", and "critical" are hard-coded in multiple places. For better maintainability and to avoid typos, consider defining these tag strings as constants.
Add constants to your class:
private const string SelfTag = "self";
private const string DependenciesTag = "dependencies";
private const string CriticalTag = "critical";Update the code to use these constants:
healthChecks.AddCheck("self", () => HealthCheckResult.Healthy(), tags: [SelfTag]);
healthChecks.AddCheck<EndpointsHealthCheck>(
"Endpoints",
failureStatus: HealthStatus.Unhealthy,
tags: [DependenciesTag]);And update the predicates in MapAspNetHealthChecks:
app.MapHealthChecks("/startup", new HealthCheckOptions
{
- Predicate = check => check.Tags.Contains("dependencies"),
+ Predicate = check => check.Tags.Contains(DependenciesTag),
ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse
});| var wellKnownEndpoints = wellKnownSchemas?.Select(schema => schema.WellKnown).ToList() ?? new List<string>(); | ||
|
|
||
| if (wellKnownEndpoints.Count > 0) | ||
| { |
There was a problem hiding this comment.
Handle empty or missing configuration for well-known schemas
When retrieving well-known schemas from configuration, wellKnownSchemas might be null if the configuration section is missing or empty. This results in wellKnownEndpoints being an empty list, which may not be the intended behavior. Consider adding a check to handle this scenario or log a warning if no endpoints are configured.
| public static void MapAspNetHealthChecks(this WebApplication app) | ||
| { | ||
| app.MapHealthChecks("/startup", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("dependencies"), | ||
| ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse | ||
| }); | ||
| app.MapHealthChecks("/liveness", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("self"), | ||
| ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse | ||
| }); | ||
| app.MapHealthChecks("/readiness", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("critical"), | ||
| ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse | ||
| }); | ||
| app.MapHealthChecks("/health", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("dependencies"), | ||
| ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Consider unifying health check endpoints under a common path
To improve the API structure and make it clear that these endpoints are related to application health, consider grouping all health check endpoints under a common base path like /health. This would result in endpoints like /health/startup, /health/liveness, /health/readiness, and /health.
Update your endpoint mappings as follows:
app.MapHealthChecks("/startup", new HealthCheckOptions
{
Predicate = check => check.Tags.Contains(DependenciesTag),
ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse
});
app.MapHealthChecks("/liveness", new HealthCheckOptions
{
Predicate = check => check.Tags.Contains(SelfTag),
ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse
});
app.MapHealthChecks("/readiness", new HealthCheckOptions
{
Predicate = check => check.Tags.Contains(CriticalTag),
ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse
});
app.MapHealthChecks("/health", new HealthCheckOptions
{
Predicate = check => check.Tags.Contains(DependenciesTag),
ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse
});Committable suggestion was skipped due to low confidence.
| public static IServiceCollection AddAspNetHealthChecks(this IServiceCollection services, IConfiguration configuration, Action<AspNetHealthChecksOptions> configure) | ||
| { | ||
| var options = new AspNetHealthChecksOptions(); | ||
| configure(options); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review the configuration pattern for clarity and best practices
The AddAspNetHealthChecks method currently accepts both IConfiguration and an Action<AspNetHealthChecksOptions> configure. While this approach provides flexibility, it might be clearer to use the options pattern fully.
Consider refactoring to accept only the Action<AspNetHealthChecksOptions> and bind the configuration within the method:
public static IServiceCollection AddAspNetHealthChecks(this IServiceCollection services, Action<AspNetHealthChecksOptions> configure)
{
var options = new AspNetHealthChecksOptions();
configure(options);
+ services.Configure<AspNetHealthChecksOptions>(configure);
var wellKnownSchemas = options.Configuration
.GetSection(options.WellKnownEndpointsConfigurationSectionPath)
.Get<List<JwtBearerTokenSchema>>();Alternatively, if the configuration is essential, make it explicit which configuration section is required by the method.
Committable suggestion was skipped due to low confidence.
| .AddCheck<RedisHealthCheck>("redis", tags: ["dependencies", "redis"]) | ||
| .AddDbContextCheck<DialogDbContext>("postgres", tags: ["dependencies", "critical"]); | ||
|
|
||
| services.AddSingleton<RedisHealthCheck>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing redundant service registration
Since RedisHealthCheck is registered in the health checks via AddCheck<RedisHealthCheck>(), explicitly registering it again with services.AddSingleton<RedisHealthCheck>(); may be unnecessary. The dependency injection container will automatically handle the instantiation of the health check.
Apply this diff to remove the redundant registration:
- services.AddSingleton<RedisHealthCheck>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| services.AddSingleton<RedisHealthCheck>(); |
| { | ||
| client.Timeout = TimeSpan.FromSeconds(5); | ||
| }) | ||
| .AddPolicyHandlerFromRegistry(PollyPolicy.DefaultHttpRetryPolicy); ; |
There was a problem hiding this comment.
Remove redundant semicolon
There's an extra semicolon at the end of the line. Removing the redundant semicolon improves code readability.
Apply this diff to fix the issue:
.AddPolicyHandlerFromRegistry(PollyPolicy.DefaultHttpRetryPolicy);;
-
+ Committable suggestion was skipped due to low confidence.
| private static IServiceCollection AddCustomHealthChecks(this IServiceCollection services) | ||
| { | ||
| services.AddHttpClient("HealthCheckClient") | ||
| .ConfigureHttpClient(client => | ||
| { | ||
| client.Timeout = TimeSpan.FromSeconds(5); | ||
| }) | ||
| .AddPolicyHandlerFromRegistry(PollyPolicy.DefaultHttpRetryPolicy); ; | ||
|
|
||
| services.AddHealthChecks() | ||
| .AddCheck<RedisHealthCheck>("redis", tags: ["dependencies", "redis"]) | ||
| .AddDbContextCheck<DialogDbContext>("postgres", tags: ["dependencies", "critical"]); | ||
|
|
||
| services.AddSingleton<RedisHealthCheck>(); | ||
|
|
||
| return services; | ||
| } |
There was a problem hiding this comment.
Correct the syntax for specifying tags in health checks
The syntax for specifying the tags parameter in the AddCheck methods should use array initializers with curly braces {}. Replace the square brackets [] with new[] {} to correctly initialize the string arrays.
Apply this diff to fix the issue:
services.AddHealthChecks()
- .AddCheck<RedisHealthCheck>("redis", tags: ["dependencies", "redis"])
- .AddDbContextCheck<DialogDbContext>("postgres", tags: ["dependencies", "critical"]);
+ .AddCheck<RedisHealthCheck>("redis", tags: new[] { "dependencies", "redis" })
+ .AddDbContextCheck<DialogDbContext>("postgres", tags: new[] { "dependencies", "critical" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static IServiceCollection AddCustomHealthChecks(this IServiceCollection services) | |
| { | |
| services.AddHttpClient("HealthCheckClient") | |
| .ConfigureHttpClient(client => | |
| { | |
| client.Timeout = TimeSpan.FromSeconds(5); | |
| }) | |
| .AddPolicyHandlerFromRegistry(PollyPolicy.DefaultHttpRetryPolicy); ; | |
| services.AddHealthChecks() | |
| .AddCheck<RedisHealthCheck>("redis", tags: ["dependencies", "redis"]) | |
| .AddDbContextCheck<DialogDbContext>("postgres", tags: ["dependencies", "critical"]); | |
| services.AddSingleton<RedisHealthCheck>(); | |
| return services; | |
| } | |
| private static IServiceCollection AddCustomHealthChecks(this IServiceCollection services) | |
| { | |
| services.AddHttpClient("HealthCheckClient") | |
| .ConfigureHttpClient(client => | |
| { | |
| client.Timeout = TimeSpan.FromSeconds(5); | |
| }) | |
| .AddPolicyHandlerFromRegistry(PollyPolicy.DefaultHttpRetryPolicy); ; | |
| services.AddHealthChecks() | |
| .AddCheck<RedisHealthCheck>("redis", tags: new[] { "dependencies", "redis" }) | |
| .AddDbContextCheck<DialogDbContext>("postgres", tags: new[] { "dependencies", "critical" }); | |
| services.AddSingleton<RedisHealthCheck>(); | |
| return services; | |
| } |
Related to #292
Summary by CodeRabbit
Summary by CodeRabbit
New Features
/startup,/liveness,/readiness, and/health.Enhancements
Bug Fixes