feat: add detailed component health checks for MySQL, Redis, and MongoDB #114
Conversation
- Enhanced /health endpoint to show per-component status - Added type, host, port, database, version, and response time details - Returns error messages and types when components are down - Added MongoDB dependency and health check support
…to contributor/suraj
📝 WalkthroughWalkthroughRestructured health check implementation to introduce per-component health routines with detailed status reporting, version retrieval, and error handling. Replaced HashMap with LinkedHashMap for insertion order preservation, added dedicated MySQL and Redis checks with host/port/database extraction from JDBC URLs, and enriched global payload with timestamps and structured component details. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Suraj-kumar00
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: 1
🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/admin/service/health/HealthService.java:
- Around line 230-232: The ping check in HealthService uses
pingResult.getDouble("ok") which can throw ClassCastException if "ok" is an
Integer; change the check to safely handle numeric types by retrieving "ok" as a
Number (e.g., pingResult.get("ok", Number.class) or cast to Number) and compare
its doubleValue() to 1.0 (or compare number.intValue() == 1) after null-checking
pingResult; update the condition around mongoTemplate.getDb().runCommand(...) to
use this safe numeric comparison.
🧹 Nitpick comments (3)
src/main/resources/application.properties (1)
78-81: Consider adding MongoDB authentication configuration.The MongoDB configuration lacks authentication credentials (
spring.data.mongodb.username,spring.data.mongodb.password,spring.data.mongodb.authentication-database). While this may be acceptable for thetestprofile (line 10), ensure that environment-specific property files for production include proper authentication and consider TLS settings (spring.data.mongodb.ssl.enabled).src/main/java/com/iemr/admin/service/health/HealthService.java (2)
180-184: Consider adding a timeout for Redis operations.Unlike the MySQL health check which has explicit timeouts, the Redis ping operation doesn't have a timeout configured. If Redis is unresponsive, this could block the health check thread indefinitely. Consider configuring
spring.redis.timeoutin application properties or wrapping the operation with a timeout.
305-349: Consider extracting common URL parsing logic to reduce duplication.
extractHostandextractPortshare identical logic for prefix removal andhostPortextraction (lines 311-316 and 336-341). Consider extracting this to a helper method for maintainability.♻️ Suggested refactor
+ private String extractHostPort(String jdbcUrl) { + if (jdbcUrl == null || "unknown".equals(jdbcUrl)) { + return null; + } + try { + String withoutPrefix = jdbcUrl.replaceFirst("jdbc:mysql://", ""); + int slashIndex = withoutPrefix.indexOf('/'); + return slashIndex > 0 ? withoutPrefix.substring(0, slashIndex) : withoutPrefix; + } catch (Exception e) { + return null; + } + } + private String extractHost(String jdbcUrl) { - if (jdbcUrl == null || "unknown".equals(jdbcUrl)) { - return "unknown"; - } - try { - String withoutPrefix = jdbcUrl.replaceFirst("jdbc:mysql://", ""); - int slashIndex = withoutPrefix.indexOf('/'); - String hostPort = slashIndex > 0 - ? withoutPrefix.substring(0, slashIndex) - : withoutPrefix; + String hostPort = extractHostPort(jdbcUrl); + if (hostPort == null) return "unknown"; + try { int colonIndex = hostPort.indexOf(':'); return colonIndex > 0 ? hostPort.substring(0, colonIndex) : hostPort; } catch (Exception e) { - logger.debug("Could not extract host from URL: {}", e.getMessage()); + return "unknown"; } - return "unknown"; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/commit-lint.ymlpom.xmlsrc/main/java/com/iemr/admin/service/health/HealthService.javasrc/main/resources/application.properties
🔇 Additional comments (6)
.github/workflows/commit-lint.yml (1)
20-22: LGTM! Proper base branch fetching for commitlint.This step correctly fetches the base branch to ensure
commitlint --fromcan access the base SHA. Thefetch-depth: 0in the checkout step combined with this fetch ensures the full commit history is available for comparison.pom.xml (1)
203-206: LGTM!The MongoDB starter dependency is correctly added without an explicit version, allowing Spring Boot's dependency management (via
spring-boot-starter-parent:3.2.2) to resolve a compatible version.src/main/java/com/iemr/admin/service/health/HealthService.java (4)
54-77: LGTM! Good use of optional autowiring and configuration defaults.The
@Autowired(required = false)for bothRedisTemplateandMongoTemplatecorrectly handles scenarios where these datastores are not configured. The@Valueannotations with sensible defaults ensure the service doesn't fail if properties are missing.
78-114: LGTM! Clean health aggregation logic.The method correctly aggregates component health statuses and marks overall status as
DOWNif any component fails. The null checks forredisTemplateandmongoTemplateproperly handle optional datastores.
116-168: LGTM! Robust MySQL health check with proper resource management.Good implementation with:
- Try-with-resources for automatic connection and statement cleanup
- Reasonable timeouts (2s for connection validation, 3s for query)
- Comprehensive error details including exception type
- Connection validation before executing the health check query
263-299: LGTM! Defensive version retrieval with graceful fallback.All three version retrieval methods properly catch exceptions and log at
debuglevel (appropriate since version info is supplementary). Returningnullon failure is correctly handled by the callers with null checks before adding to details.
|
Hi @vanitha1822 and @vishwab1 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/admin/service/health/HealthService.java:
- Around line 171-178: The MongoDB health check may NPE by unboxing a null from
pingResult.getDouble("ok") and uses fragile equality; update the lambda in
performHealthCheck to first verify pingResult is non-null and contains the "ok"
key, retrieve the value as a Number (e.g., Number okVal = (Number)
pingResult.get("ok")) or via getDouble with null-check, ensure okVal != null,
then use Double.compare(okVal.doubleValue(), 1.0) == 0 or a tolerance (e.g., >=
0.999) to decide success; if the key is missing or the value indicates failure
return new HealthCheckResult(false, null, "Ping returned unexpected or missing
ok value") and keep getMongoDBVersion() and HealthCheckResult usage unchanged.
🧹 Nitpick comments (1)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
190-215: Response time not recorded for failed health checks.
responseTimeMsis only added when the check succeeds. Recording response time for failures would help diagnose timeout vs immediate failure scenarios.Proposed improvement
try { HealthCheckResult result = checker.get(); long responseTime = System.currentTimeMillis() - startTime; + details.put("responseTimeMs", responseTime); if (result.isHealthy) { logger.debug("{} health check: UP ({}ms)", componentName, responseTime); status.put("status", "UP"); - details.put("responseTimeMs", responseTime); if (result.version != null) { details.put("version", result.version); } } else { logger.warn("{} health check: {}", componentName, result.error); status.put("status", "DOWN"); details.put("error", result.error); } status.put("details", details); return status; } catch (Exception e) { + long responseTime = System.currentTimeMillis() - startTime; logger.error("{} health check failed: {}", componentName, e.getMessage()); status.put("status", "DOWN"); details.put("error", e.getMessage()); details.put("errorType", e.getClass().getSimpleName()); + details.put("responseTimeMs", responseTime); status.put("details", details); return status; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/health/HealthService.java
🔇 Additional comments (3)
src/main/java/com/iemr/admin/service/health/HealthService.java (3)
316-329: LGTM!Clean internal result holder. If targeting Java 16+, this could be simplified to a record, but the current implementation is perfectly fine.
79-115: Well-structured health aggregation.Good use of
LinkedHashMapfor predictable JSON ordering, conditional component checks based on template availability, and clean aggregation of overall status. The timestamp usingInstant.now().toString()produces ISO-8601 format which is appropriate for APIs.
260-276: No issue: URL parsing is appropriately MySQL-specific.The application consistently uses
com.mysql.cj.jdbc.Driveracross all environment configurations (admin_ci.properties, admin_docker.properties, admin_example.properties). TheextractHost,extractPort, andextractDatabaseNamemethods are correctly hardcoded for thejdbc:mysql://format that matches the actual JDBC driver in use. The specificity is intentional and appropriate for this codebase's design.Likely an incorrect or invalid review comment.
| return performHealthCheck("MongoDB", details, () -> { | ||
| Document pingResult = mongoTemplate.getDb().runCommand(new Document("ping", 1)); | ||
| if (pingResult != null && pingResult.getDouble("ok") == 1.0) { | ||
| String version = getMongoDBVersion(); | ||
| return new HealthCheckResult(true, version, null); | ||
| } | ||
| return new HealthCheckResult(false, null, "Ping returned unexpected response"); | ||
| }); |
There was a problem hiding this comment.
Potential NullPointerException on MongoDB health check.
Document.getDouble("ok") returns a Double wrapper that can be null if the key doesn't exist. Unboxing a null Double for comparison with 1.0 will throw NPE. Additionally, floating-point equality comparisons are fragile.
Proposed fix
return performHealthCheck("MongoDB", details, () -> {
Document pingResult = mongoTemplate.getDb().runCommand(new Document("ping", 1));
- if (pingResult != null && pingResult.getDouble("ok") == 1.0) {
+ if (pingResult != null) {
+ Double okValue = pingResult.getDouble("ok");
+ if (okValue != null && okValue >= 1.0) {
String version = getMongoDBVersion();
return new HealthCheckResult(true, version, null);
+ }
}
return new HealthCheckResult(false, null, "Ping returned unexpected response");
});📝 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.
| return performHealthCheck("MongoDB", details, () -> { | |
| Document pingResult = mongoTemplate.getDb().runCommand(new Document("ping", 1)); | |
| if (pingResult != null && pingResult.getDouble("ok") == 1.0) { | |
| String version = getMongoDBVersion(); | |
| return new HealthCheckResult(true, version, null); | |
| } | |
| return new HealthCheckResult(false, null, "Ping returned unexpected response"); | |
| }); | |
| return performHealthCheck("MongoDB", details, () -> { | |
| Document pingResult = mongoTemplate.getDb().runCommand(new Document("ping", 1)); | |
| if (pingResult != null) { | |
| Double okValue = pingResult.getDouble("ok"); | |
| if (okValue != null && okValue >= 1.0) { | |
| String version = getMongoDBVersion(); | |
| return new HealthCheckResult(true, version, null); | |
| } | |
| } | |
| return new HealthCheckResult(false, null, "Ping returned unexpected response"); | |
| }); |
🤖 Prompt for AI Agents
In @src/main/java/com/iemr/admin/service/health/HealthService.java around lines
171 - 178, The MongoDB health check may NPE by unboxing a null from
pingResult.getDouble("ok") and uses fragile equality; update the lambda in
performHealthCheck to first verify pingResult is non-null and contains the "ok"
key, retrieve the value as a Number (e.g., Number okVal = (Number)
pingResult.get("ok")) or via getDouble with null-check, ensure okVal != null,
then use Double.compare(okVal.doubleValue(), 1.0) == 0 or a tolerance (e.g., >=
0.999) to decide success; if the key is missing or the value indicates failure
return new HealthCheckResult(false, null, "Ping returned unexpected or missing
ok value") and keep getMongoDBVersion() and HealthCheckResult usage unchanged.
|
Admin-API doesn't talk to Mongo directly. This is a dependency for FHIR-API. @Suraj-kumar00 can you also show the final JSON output? |
- Removed MongoDB dependency and configuration (Admin-API doesn't use MongoDB) - Removed MongoDB health check from HealthService - Removed unnecessary base branch fetch step from commit-lint workflow - MongoDB is only used by FHIR-API, not Admin-API
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/iemr/admin/service/health/HealthService.java`:
- Around line 146-177: In performHealthCheck, stop returning raw exception
messages and ensure failure payloads are consistent: replace uses of
e.getMessage() in the catch and in unhealthy branches with a sanitized error
string (e.g., "InternalError" or a curated mapping) while logging the full
exception internally via logger.error with the exception object; always populate
responseTimeMs (compute using startTime) and errorType (use
e.getClass().getSimpleName() or set to the HealthCheckResult error type) into
details for both checker-failure (result.isHealthy == false) and catch paths so
every returned status map includes status, details.responseTimeMs and
details.errorType alongside a safe details.error value; keep HealthCheckResult
handling (version) unchanged but ensure you log full exception for debugging
only and return only sanitized error text to callers.
| private Map<String, Object> performHealthCheck(String componentName, | ||
| Map<String, Object> details, | ||
| Supplier<HealthCheckResult> checker) { | ||
| Map<String, Object> status = new LinkedHashMap<>(); | ||
| long startTime = System.currentTimeMillis(); | ||
|
|
||
| try { | ||
| HealthCheckResult result = checker.get(); | ||
| long responseTime = System.currentTimeMillis() - startTime; | ||
|
|
||
| if (result.isHealthy) { | ||
| logger.debug("{} health check: UP ({}ms)", componentName, responseTime); | ||
| status.put("status", "UP"); | ||
| details.put("responseTimeMs", responseTime); | ||
| if (result.version != null) { | ||
| details.put("version", result.version); | ||
| } | ||
| } else { | ||
| logger.warn("{} health check: {}", componentName, result.error); | ||
| status.put("status", "DOWN"); | ||
| details.put("error", result.error); | ||
| } | ||
| logger.warn("Database health check: Connection not valid"); | ||
| return false; | ||
| status.put("details", details); | ||
| return status; | ||
| } catch (Exception e) { | ||
| logger.error("Database health check failed: {}", e.getMessage()); | ||
| return false; | ||
| logger.error("{} health check failed: {}", componentName, e.getMessage()); | ||
| status.put("status", "DOWN"); | ||
| details.put("error", e.getMessage()); | ||
| details.put("errorType", e.getClass().getSimpleName()); | ||
| status.put("details", details); | ||
| return status; | ||
| } |
There was a problem hiding this comment.
Don’t return raw exception messages; ensure failure payloads are consistent.
e.getMessage() can leak internal host/user details to callers. Also, responseTimeMs and errorType aren’t guaranteed on all failure paths. Consider sanitizing errors and always attaching timing/error metadata.
🐛 Proposed fix
try {
HealthCheckResult result = checker.get();
long responseTime = System.currentTimeMillis() - startTime;
+ details.put("responseTimeMs", responseTime);
if (result.isHealthy) {
logger.debug("{} health check: UP ({}ms)", componentName, responseTime);
status.put("status", "UP");
- details.put("responseTimeMs", responseTime);
if (result.version != null) {
details.put("version", result.version);
}
} else {
logger.warn("{} health check: {}", componentName, result.error);
status.put("status", "DOWN");
- details.put("error", result.error);
+ details.put("error", result.error != null ? result.error : "Health check failed");
+ details.put("errorType", "CheckFailed");
}
status.put("details", details);
return status;
} catch (Exception e) {
- logger.error("{} health check failed: {}", componentName, e.getMessage());
+ long responseTime = System.currentTimeMillis() - startTime;
+ logger.error("{} health check failed", componentName, e);
status.put("status", "DOWN");
- details.put("error", e.getMessage());
+ details.put("responseTimeMs", responseTime);
+ details.put("error", "Health check failed");
details.put("errorType", e.getClass().getSimpleName());
status.put("details", details);
return status;
}🤖 Prompt for AI Agents
In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines
146 - 177, In performHealthCheck, stop returning raw exception messages and
ensure failure payloads are consistent: replace uses of e.getMessage() in the
catch and in unhealthy branches with a sanitized error string (e.g.,
"InternalError" or a curated mapping) while logging the full exception
internally via logger.error with the exception object; always populate
responseTimeMs (compute using startTime) and errorType (use
e.getClass().getSimpleName() or set to the HealthCheckResult error type) into
details for both checker-failure (result.isHealthy == false) and catch paths so
every returned status map includes status, details.responseTimeMs and
details.errorType alongside a safe details.error value; keep HealthCheckResult
handling (version) unchanged but ensure you log full exception for debugging
only and return only sanitized error text to callers.
@drtechie, after applying the fixes you suggested, here is the refined JSON for /health: {
"status": "DOWN",
"timestamp": "2026-01-17T12:11:27.086571048Z",
"components": {
"mysql": {
"status": "DOWN",
"details": {
"type": "MySQL",
"host": "mysql-container",
"port": "3306",
"database": "db_iemr",
"error": "java.sql.SQLException: Access denied for user 'root'@'172.18.0.5' (using password: YES)",
"errorType": "RuntimeException"
}
},
"redis": {
"status": "UP",
"details": {
"type": "Redis",
"host": "redis-container",
"port": 6379,
"responseTimeMs": 446,
"version": "7.2.4"
}
}
}
} |
2 participants
📋 Description
Closes #102
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.