feat(health,version): add health and version endpoints#155
feat(health,version): add health and version endpoints#155drtechie merged 13 commits intoPSMRI:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds /health and /version REST endpoints with a concurrent HealthService (MySQL + optional Redis); exposes those endpoints publicly in security and JWT filter; updates pom.xml (removes actuator, changes finalName, removes checkstyle encoding, adds git-commit-id-maven-plugin). Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant HC as HealthController
participant HS as HealthService
participant DS as DataSource
participant RT as RedisTemplate
Client->>HC: GET /health
HC->>HS: checkHealth()
activate HS
par concurrent checks
HS->>DS: lightweight SQL (3s timeout)
HS->>RT: PING (if configured, 3s timeout)
end
DS-->>HS: SQL result / error
RT-->>HS: PONG / timeout / error
HS->>HS: aggregate statuses, timings, severity
deactivate HS
HS-->>HC: health map (status, timestamp, components)
HC-->>Client: ResponseEntity (200 or 503 + health JSON)
sequenceDiagram
actor Client
participant VC as VersionController
participant Loader as GitPropertiesLoader
participant Classpath as ClasspathResource
Client->>VC: GET /version
VC->>Loader: load git.properties from classpath
Loader->>Classpath: read git.properties
alt git.properties found
Classpath-->>Loader: properties (git.build.time, git.build.version, git.branch, git.commit.id.abbrev)
Loader-->>VC: populated map
else not found / error
Classpath-->>Loader: missing / error
Loader-->>VC: defaults ("unknown")
end
VC-->>Client: ResponseEntity (200 + version JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)
67-93: Health checks run sequentially despite having a thread pool.
checkHealth()callsperformHealthCheckfor MySQL then Redis in sequence (both synchronously viachecker.get()). TheCompletableFutureinsidecheckRedisHealthadds async wrapping but the caller blocks onfuture.get()anyway, so total latency is MySQL-timeout + Redis-timeout in the worst case (6s).If concurrent checking was intended, consider submitting both checks via
CompletableFutureand joining. If sequential was intentional, theCompletableFutureincheckRedisHealthonly adds the timeout capability — consider documenting this or using a simplerFutureapproach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 67 - 93, checkHealth currently calls performHealthCheck sequentially for "MySQL" and "Redis" (using checkMySQLHealth and checkRedisHealth) which blocks on each checker.get() so checks run serially; to run concurrently, submit both health checks as CompletableFutures (e.g., use CompletableFuture.supplyAsync or submit the existing check lambdas to your thread pool inside checkHealth), store futures keyed by component name, wait for all with CompletableFuture.allOf(...).join(), then populate the components map from each future result and compute overall status with isHealthy and STATUS_KEY/STATUS_UP/STATUS_DOWN; alternatively, if sequential behavior is intended, remove the unnecessary CompletableFuture wrapping from checkRedisHealth or document that it only provides timeout behaviour.src/main/java/com/iemr/mmu/controller/health/HealthController.java (1)
60-70: ConsiderDEBUGlevel for routine health-check access logging (Line 61).Load balancers and monitoring systems typically poll
/healthevery few seconds. Logging atINFOon every invocation will generate significant log noise.DEBUGis more appropriate for the entry log; theWARN/ERRORpaths inHealthServicealready capture failures at the right level.Proposed fix
- logger.info("Health check endpoint called"); + logger.debug("Health check endpoint called");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java` around lines 60 - 70, The entry log in HealthController.checkHealth currently uses logger.info and should be lowered to logger.debug to avoid noisy INFO-level logs from frequent health checks; update the log call at the start of the checkHealth method (the statement that currently logs "Health check endpoint called") to use debug level while leaving the existing debug/warn/error logs in HealthService and the method unchanged otherwise.src/main/java/com/iemr/mmu/controller/version/VersionController.java (1)
49-67:git.propertiesis re-read from classpath on every request — consider loading once at startup.The git properties are static for the lifetime of the application. Loading them in the constructor (or a
@PostConstructmethod) and caching thePropertiesobject avoids repeated classpath I/O.Proposed fix
`@RestController` public class VersionController { private final Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); private static final String UNKNOWN_VALUE = "unknown"; + private final Map<String, String> versionInfo; + + public VersionController() { + this.versionInfo = loadVersionInfo(); + } + + private Map<String, String> loadVersionInfo() { + Map<String, String> info = new LinkedHashMap<>(); + try { + Properties gitProperties = loadGitProperties(); + info.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); + info.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); + info.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); + info.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); + } catch (Exception e) { + logger.error("Failed to load version information", e); + info.put("buildTimestamp", UNKNOWN_VALUE); + info.put("version", UNKNOWN_VALUE); + info.put("branch", UNKNOWN_VALUE); + info.put("commitHash", UNKNOWN_VALUE); + } + return info; + } `@Operation`(summary = "Get version information") `@GetMapping`(value = "/version", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity<Map<String, String>> versionInformation() { - Map<String, String> response = new LinkedHashMap<>(); - try { - logger.info("version Controller Start"); - Properties gitProperties = loadGitProperties(); - response.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); - ... - } catch (Exception e) { ... } - logger.info("version Controller End"); - return ResponseEntity.ok(response); + return ResponseEntity.ok(versionInfo); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/controller/version/VersionController.java` around lines 49 - 67, The versionInformation() method currently calls loadGitProperties() on every request; move loading of git.properties into a one-time initialization and cache it on the controller (e.g., load in the VersionController constructor or a `@PostConstruct` method into a private final Properties field like cachedGitProperties), update versionInformation() to read from cachedGitProperties and fall back to UNKNOWN_VALUE if null, and remove repeated I/O by deleting the per-request call to loadGitProperties(); keep existing keys (git.build.time, git.build.version, git.branch, git.commit.id.abbrev) and existing error fallback logic referencing UNKNOWN_VALUE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 109-111: In HealthService (checkMySQLHealth and checkRedisHealth)
avoid returning raw exception messages in HealthCheckResult.result/error and the
JSON status map; instead log the full exception with logger.warn or logger.error
including e (e.g., logger.warn("MySQL health check failed", e)) and set the
HealthCheckResult(error) to a sanitized generic string like "MySQL connection
failed" or "Redis connection failed"; also change where status.put("error",
safeError) is populated to use that sanitized message while keeping the full
exception only in logs (update usages of HealthCheckResult, result.error and the
status.put call to use the generic message).
- Around line 56-65: HealthService creates a fixed ExecutorService
(executorService) but never shuts it down; implement a lifecycle shutdown by
either having HealthService implement DisposableBean and overriding destroy() to
call executorService.shutdown() (and optionally await termination) or add a
`@PreDestroy-annotated` method (e.g., shutdownExecutor) that invokes
executorService.shutdown() and handles InterruptedException/timeout; update the
class constructor usage accordingly and ensure the executorService field is used
in that shutdown method to avoid thread leakage on application stop.
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java`:
- Around line 60-70: The entry log in HealthController.checkHealth currently
uses logger.info and should be lowered to logger.debug to avoid noisy INFO-level
logs from frequent health checks; update the log call at the start of the
checkHealth method (the statement that currently logs "Health check endpoint
called") to use debug level while leaving the existing debug/warn/error logs in
HealthService and the method unchanged otherwise.
In `@src/main/java/com/iemr/mmu/controller/version/VersionController.java`:
- Around line 49-67: The versionInformation() method currently calls
loadGitProperties() on every request; move loading of git.properties into a
one-time initialization and cache it on the controller (e.g., load in the
VersionController constructor or a `@PostConstruct` method into a private final
Properties field like cachedGitProperties), update versionInformation() to read
from cachedGitProperties and fall back to UNKNOWN_VALUE if null, and remove
repeated I/O by deleting the per-request call to loadGitProperties(); keep
existing keys (git.build.time, git.build.version, git.branch,
git.commit.id.abbrev) and existing error fallback logic referencing
UNKNOWN_VALUE.
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 67-93: checkHealth currently calls performHealthCheck sequentially
for "MySQL" and "Redis" (using checkMySQLHealth and checkRedisHealth) which
blocks on each checker.get() so checks run serially; to run concurrently, submit
both health checks as CompletableFutures (e.g., use
CompletableFuture.supplyAsync or submit the existing check lambdas to your
thread pool inside checkHealth), store futures keyed by component name, wait for
all with CompletableFuture.allOf(...).join(), then populate the components map
from each future result and compute overall status with isHealthy and
STATUS_KEY/STATUS_UP/STATUS_DOWN; alternatively, if sequential behavior is
intended, remove the unnecessary CompletableFuture wrapping from
checkRedisHealth or document that it only provides timeout behaviour.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/iemr/mmu/service/health/HealthService.java (2)
150-152:performHealthCheckboth mutatesstatusand returns it — redundant return value.All call sites in
checkHealth()already hold a reference to the same map; the return value is never used. Changing the signature tovoideliminates the implicit "this mutates in place AND returns" contract, which is easy to misread.♻️ Proposed refactor
-private Map<String, Object> performHealthCheck(String componentName, - Map<String, Object> status, - Supplier<HealthCheckResult> checker) { +private void performHealthCheck(String componentName, + Map<String, Object> status, + Supplier<HealthCheckResult> checker) { ... - return status; ... - return status; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 150 - 152, performHealthCheck currently mutates the passed-in Map and also returns it; change its signature from returning Map<String,Object> to void and remove any return statements inside performHealthCheck, then update all call sites in checkHealth() to stop using its return value (just call performHealthCheck(componentName, status, checker)); ensure the method name performHealthCheck and callers in checkHealth() are updated consistently and compilation errors are fixed by removing any assignments expecting a return.
121-127: Swallowed exception in Redis PING lambda — actual failure cause is never logged.The
catch (Exception e)block on Lines 124-126 discards the exception entirely, returningnull. Whenpong == null,checkRedisHealthreports "Redis PING failed" with no diagnostic detail in the logs — making it impossible to distinguish a connection error, an auth failure, or any other root cause.🔍 Proposed fix — log before swallowing
CompletableFuture<String> future = CompletableFuture.supplyAsync(() -> { try { return redisTemplate.execute((org.springframework.data.redis.core.RedisCallback<String>) connection -> connection.ping()); } catch (Exception e) { + logger.warn("Redis PING execution failed: {}", e.getMessage()); return null; } }, executorService);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 121 - 127, The Redis PING lambda in CompletableFuture.supplyAsync swallows exceptions, losing diagnostics; update the catch block inside the supplyAsync lambda (the redisTemplate.execute lambda submitted to executorService in checkRedisHealth) to log the caught Exception before returning null — e.g. use the class logger (logger or log) to call error with a descriptive message and the exception (include exception as parameter) so the root cause (connection/auth/etc.) is visible in logs, then return null as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 137-139: In the TimeoutException catch block inside
HealthService's Redis health check, call future.cancel(true) before returning to
ensure the background CompletableFuture task is interrupted and frees the Redis
connection held by the executorService thread; update the catch for
TimeoutException to invoke future.cancel(true) (using the existing future
variable) and then log and return the HealthCheckResult as before.
- Around line 95-113: The MySQL health check in checkMySQLHealth currently calls
dataSource.getConnection() directly (and uses stmt.setQueryTimeout for query
execution), which can block the request thread while waiting for a connection;
wrap the whole check (connection acquisition, prepare/execute) in an
asynchronous task with a bounded deadline (e.g., CompletableFuture.supplyAsync
or runAsync plus .get(timeout, unit) or .orTimeout) similar to the Redis check
so the call fails fast; ensure you still set stmt.setQueryTimeout((int)
MYSQL_TIMEOUT_SECONDS) for query execution, catch and translate
timeout/ExecutionException/InterruptedException into a HealthCheckResult(false,
"...") and cancel the underlying task if the deadline is exceeded so resources
(including pending getConnection) are not left hanging.
---
Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 144-146: The catch block in HealthService is returning the raw
Redis exception message to the /health response; instead return a generic,
non-sensitive message (e.g., "Redis health check failed") in the
HealthCheckResult and stop exposing e.getMessage() there, while preserving
detailed diagnostics in logs by logging the exception object (e.g.,
logger.warn("Redis health check failed", e)) so the stack trace is available for
debugging; update the catch that constructs new HealthCheckResult(false, ...)
and the logger.warn call accordingly.
- Around line 56-65: The HealthService constructs a fixed-thread ExecutorService
(executorService) in the constructor but never shuts it down; add a lifecycle
shutdown to avoid thread leaks by implementing a destroy hook (e.g., add a
`@PreDestroy` method or implement DisposableBean#destroy) that calls shutdown()
and awaitTermination with a short timeout on the executorService, and force
shutdownNow() if it doesn’t terminate; reference the executorService field and
HealthService class/constructor when locating where to add the cleanup.
- Around line 109-111: The catch block in HealthService that currently returns
e.getMessage() exposes internal details on the public /health endpoint; instead
log the full exception (logger.warn("MySQL health check failed", e)) to capture
stacktrace, but return a non-sensitive generic message in the HealthCheckResult
such as "Database unavailable" or a sanitized error code; update the catch in
the health check method accordingly to stop returning e.getMessage() while
preserving internal logs for debugging.
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 150-152: performHealthCheck currently mutates the passed-in Map
and also returns it; change its signature from returning Map<String,Object> to
void and remove any return statements inside performHealthCheck, then update all
call sites in checkHealth() to stop using its return value (just call
performHealthCheck(componentName, status, checker)); ensure the method name
performHealthCheck and callers in checkHealth() are updated consistently and
compilation errors are fixed by removing any assignments expecting a return.
- Around line 121-127: The Redis PING lambda in CompletableFuture.supplyAsync
swallows exceptions, losing diagnostics; update the catch block inside the
supplyAsync lambda (the redisTemplate.execute lambda submitted to
executorService in checkRedisHealth) to log the caught Exception before
returning null — e.g. use the class logger (logger or log) to call error with a
descriptive message and the exception (include exception as parameter) so the
root cause (connection/auth/etc.) is visible in logs, then return null as
before.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/com/iemr/mmu/controller/health/HealthController.java (1)
68-68: Hardcoded"DOWN"literal duplicates the privateSTATUS_DOWNconstant inHealthService.If
HealthService.STATUS_DOWNis ever renamed, this comparison silently breaks. Expose the constant (or extract a sharedHealthConstantsclass) and reference it here.♻️ Proposed refactor
// In HealthService.java - private static final String STATUS_DOWN = "DOWN"; + static final String STATUS_DOWN = "DOWN"; // In HealthController.java - HttpStatus httpStatus = "DOWN".equals(overallStatus) ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK; + HttpStatus httpStatus = HealthService.STATUS_DOWN.equals(overallStatus) ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java` at line 68, The controller currently compares overallStatus to a hardcoded "DOWN" string; make this comparison use the central constant instead to avoid drift: either change the private STATUS_DOWN in HealthService to a public static final (e.g., HealthService.STATUS_DOWN) and update the comparison in HealthController (replace "DOWN" with HealthService.STATUS_DOWN), or extract the constant into a shared HealthConstants class (e.g., HealthConstants.STATUS_DOWN), import that in HealthController and use HealthConstants.STATUS_DOWN in the HttpStatus selection logic where overallStatus is compared; ensure you update references and imports accordingly.src/main/java/com/iemr/mmu/service/health/HealthService.java (2)
96-96:ADVANCED_HEALTH_CHECKS_ENABLEDis dead code — cannot be toggled without recompiling.The
static final boolean = trueconstant makes the guard at Line 320 unreachable at compile time. Consider externalising it as a Spring property:♻️ Proposed refactor
- private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true; + `@Value`("${health.advanced-checks.enabled:true}") + private boolean advancedHealthChecksEnabled;- if (!ADVANCED_HEALTH_CHECKS_ENABLED) { + if (!advancedHealthChecksEnabled) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` at line 96, The ADVANCED_HEALTH_CHECKS_ENABLED static final flag is dead code; replace it with a configurable Spring property to allow toggling at runtime. Remove the static final ADVANCED_HEALTH_CHECKS_ENABLED constant in HealthService, add an instance boolean field injected from configuration (e.g. `@Value`("${app.health.advanced:false}") private boolean advancedHealthChecksEnabled or constructor-inject `@Value`) and update the guard(s) that reference ADVANCED_HEALTH_CHECKS_ENABLED (e.g., the check currently guarding advanced checks in HealthService) to use the new instance field so the behavior can be toggled via application properties.
200-218: Redis "not configured" is reported asUP— consider aSKIPPEDstatus for clarity.When
redisTemplateisnull, the component appears in the response as"status": "UP"with a message. Consumers (monitoring dashboards, load balancers) may incorrectly interpret this as a live Redis connection. A dedicated"SKIPPED"status (or omitting the component entirely) is less ambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 200 - 218, The current checkRedisHealthSync method returns an "UP" HealthCheckResult when redisTemplate is null, which is misleading; update checkRedisHealthSync (and if needed the HealthCheckResult class) so that when redisTemplate == null it returns a result that represents a SKIPPED status (e.g., a dedicated SKIPPED enum/state or a skipped flag) instead of reporting UP — change the redisTemplate null branch in checkRedisHealthSync to construct/return a HealthCheckResult that conveys SKIPPED with the message "Redis not configured — skipped" (and adjust HealthCheckResult constructors/serialization if required) so monitoring consumers see SKIPPED for the redis component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java`:
- Around line 60-62: In HealthController.checkHealth replace the noisy
logger.info("Health check endpoint called") with logger.debug(...) so
health-check probes no longer emit INFO logs; locate the method checkHealth in
class HealthController and change the logging call to logger.debug while leaving
the surrounding method behavior intact (no other code changes).
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 410-439: The hasDeadlocks(Connection) method currently treats any
historical "LATEST DETECTED DEADLOCK" as a current failure and uses fragile
message matching for permission errors; update it to either remove the check
entirely or make it time-bounded and robust: replace the message-based
permission detection with numeric SQL error code checks (e.getErrorCode() ==
1142 || e.getErrorCode() == 1227) in the SQLException handler, and if keeping
the deadlock check parse the InnoDB status blob from hasDeadlocks() to extract
the deadlock timestamp (or switch to querying performance_schema/innodb_metrics)
and only return true when the detected deadlock timestamp is within a recent
threshold (e.g., last N minutes); also keep the deadlockCheckDisabled boolean
behavior but set it only on numeric error codes. Ensure these changes are made
inside the hasDeadlocks method and reference the deadlockCheckDisabled flag and
the SHOW ENGINE INNODB STATUS handling code.
- Around line 122-168: The timeout path has two issues: shared LinkedHashMap
writes from background tasks (mysqlStatus/redisStatus) create a data-race, and
CompletableFuture.cancel(true) won’t interrupt worker threads so executor
threads can be starved; fix by making the health tasks return results and using
Future-based cancellation: change checkHealth to submit callables via
executorService.submit(...) that call performHealthCheck/checkMySQLHealthSync
and checkRedisHealthSync and return their status maps (use ConcurrentHashMap for
returned maps or construct new LinkedHashMap only on the caller thread), keep
the Future references, call future.get(timeout, TimeUnit.SECONDS) and on
TimeoutException call future.cancel(true), then merge each Future’s returned map
into mysqlStatus/redisStatus before calling ensurePopulated and
computeOverallStatus (refer to methods checkHealth, performHealthCheck,
checkMySQLHealthSync, checkRedisHealthSync, ensurePopulated,
computeOverallStatus, and executorService).
---
Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 194-196: The current catch block in HealthService correctly logs
the full exception via logger.warn("MySQL health check failed: {}",
e.getMessage(), e) while returning a sanitized, generic HealthCheckResult(false,
"MySQL connection failed", false); no code change is required—keep the existing
logger.warn usage and the generic HealthCheckResult return as-is to preserve
detailed logs and prevent information disclosure.
- Around line 105-120: The HealthService.shutdown method annotated with
`@PreDestroy` properly addresses the thread-leak by checking executorService,
calling executorService.shutdown(), awaiting termination for 5 seconds, falling
back to executorService.shutdownNow(), and handling InterruptedException by
re-interrupting the thread; no further code changes are required for this issue
(ensure the logic remains in the shutdown() method and references to
executorService, awaitTermination, shutdownNow, and the `@PreDestroy` annotation
are preserved).
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java`:
- Line 68: The controller currently compares overallStatus to a hardcoded "DOWN"
string; make this comparison use the central constant instead to avoid drift:
either change the private STATUS_DOWN in HealthService to a public static final
(e.g., HealthService.STATUS_DOWN) and update the comparison in HealthController
(replace "DOWN" with HealthService.STATUS_DOWN), or extract the constant into a
shared HealthConstants class (e.g., HealthConstants.STATUS_DOWN), import that in
HealthController and use HealthConstants.STATUS_DOWN in the HttpStatus selection
logic where overallStatus is compared; ensure you update references and imports
accordingly.
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Line 96: The ADVANCED_HEALTH_CHECKS_ENABLED static final flag is dead code;
replace it with a configurable Spring property to allow toggling at runtime.
Remove the static final ADVANCED_HEALTH_CHECKS_ENABLED constant in
HealthService, add an instance boolean field injected from configuration (e.g.
`@Value`("${app.health.advanced:false}") private boolean
advancedHealthChecksEnabled or constructor-inject `@Value`) and update the
guard(s) that reference ADVANCED_HEALTH_CHECKS_ENABLED (e.g., the check
currently guarding advanced checks in HealthService) to use the new instance
field so the behavior can be toggled via application properties.
- Around line 200-218: The current checkRedisHealthSync method returns an "UP"
HealthCheckResult when redisTemplate is null, which is misleading; update
checkRedisHealthSync (and if needed the HealthCheckResult class) so that when
redisTemplate == null it returns a result that represents a SKIPPED status
(e.g., a dedicated SKIPPED enum/state or a skipped flag) instead of reporting UP
— change the redisTemplate null branch in checkRedisHealthSync to
construct/return a HealthCheckResult that conveys SKIPPED with the message
"Redis not configured — skipped" (and adjust HealthCheckResult
constructors/serialization if required) so monitoring consumers see SKIPPED for
the redis component.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)
96-96: Consider externalisingADVANCED_HEALTH_CHECKS_ENABLEDas a configurable propertyThe flag is hardcoded to
trueand cannot be disabled at deploy time without recompilation. If the diagnostic queries (INFORMATION_SCHEMA.PROCESSLIST,SHOW ENGINE INNODB STATUS) cause unexpected load on a production host, there's no runtime escape hatch.♻️ Proposed refactor
- private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true; + `@org.springframework.beans.factory.annotation.Value`("${health.advanced-checks.enabled:true}") + private boolean advancedHealthChecksEnabled;Then reference
advancedHealthChecksEnabledinperformAdvancedMySQLChecksWithThrottle()instead of the constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` at line 96, Replace the hardcoded static constant ADVANCED_HEALTH_CHECKS_ENABLED with a configurable instance property (e.g., advancedHealthChecksEnabled) that is loaded from configuration at startup (application.properties/@Value, Environment, or System.getenv/System.getProperty) with a sensible default (true). Remove or rename the static constant and initialize advancedHealthChecksEnabled in the HealthService constructor or via DI so it can be changed at deploy time, then update performAdvancedMySQLChecksWithThrottle() to reference advancedHealthChecksEnabled instead of ADVANCED_HEALTH_CHECKS_ENABLED; ensure the new field is final/immutable after initialization and has a fallback default if the config key is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 138-153: The two health-check futures (mysqlFuture and
redisFuture) currently each call get(maxTimeout, TimeUnit.SECONDS) which can
double the total wait; change to a shared deadline approach in HealthService:
record a deadline (e.g., using System.nanoTime() +
TimeUnit.SECONDS.toNanos(maxTimeout)), then when waiting for each future compute
the remaining time as deadline - System.nanoTime(), convert to the appropriate
TimeUnit and pass that remaining budget into future.get(...); if remaining <= 0,
treat as a timeout and cancel both futures (mysqlFuture.cancel(true),
redisFuture.cancel(true)) and handle the TimeoutException/cleanup as before so
total wall-clock wait never exceeds maxTimeout.
---
Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 412-439: hasDeadlocks currently flags any historical deadlock
because it looks for the literal "LATEST DETECTED DEADLOCK" in the output of
SHOW ENGINE INNODB STATUS; update hasDeadlocks to either (A) parse the timestamp
inside the "LATEST DETECTED DEADLOCK" block from the innodbStatus string and
only return true when that timestamp is within a recent configurable window
(e.g., last X minutes), or (B) replace the check entirely by querying
performance_schema.events_errors_summary_global_by_error (or another appropriate
performance_schema table) to detect recent deadlock events; implement one
approach, add a small helper (e.g., parseInnodbDeadlockTimestamp or
queryPerformanceSchemaForDeadlocks) and honor the existing deadlockCheckDisabled
flag and permission handling in hasDeadlocks.
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Line 96: Replace the hardcoded static constant ADVANCED_HEALTH_CHECKS_ENABLED
with a configurable instance property (e.g., advancedHealthChecksEnabled) that
is loaded from configuration at startup (application.properties/@Value,
Environment, or System.getenv/System.getProperty) with a sensible default
(true). Remove or rename the static constant and initialize
advancedHealthChecksEnabled in the HealthService constructor or via DI so it can
be changed at deploy time, then update performAdvancedMySQLChecksWithThrottle()
to reference advancedHealthChecksEnabled instead of
ADVANCED_HEALTH_CHECKS_ENABLED; ensure the new field is final/immutable after
initialization and has a fallback default if the config key is absent.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)
350-351:writeLock().lock()ignores thread interrupts fromcancel(true).Under concurrent health check requests, two MySQL tasks from different callers can race on the write lock. If a
cancel(true)fires while Thread-2 is blocked inwriteLock().lock(), the interrupt flag is set but the thread stays blocked until Thread-1 releases the lock (potentially after running up to 4 × 2 s = 8 s of advanced queries). UselockInterruptibly()so the outer deadline actually propagates into this critical section.♻️ Proposed fix
- advancedCheckLock.writeLock().lock(); - try { + try { + advancedCheckLock.writeLock().lockInterruptibly(); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + // Return cached result if available, otherwise report not degraded + return cachedAdvancedCheckResult != null && cachedAdvancedCheckResult.isDegraded; + } + try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 350 - 351, Replace the blocking advancedCheckLock.writeLock().lock() call with advancedCheckLock.writeLock().lockInterruptibly() in HealthService so the thread responds to cancel(true); wrap the lock acquisition in a try that catches InterruptedException, reasserts the interrupt (Thread.currentThread().interrupt()) and aborts/returns (or throws a checked exception) so the outer deadline propagates; ensure the existing finally block still calls advancedCheckLock.writeLock().unlock() only when the lock was successfully acquired (track a boolean locked flag) to avoid IllegalMonitorStateException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 401-419: The hasLockWaits method currently queries
INFORMATION_SCHEMA.PROCESSLIST for lock waits across all DB users; change the
query to restrict to the application's DB user (e.g., add "AND user = ?" or use
"AND user = SUBSTRING_INDEX(USER(), '@', 1)") and bind the app DB username
before executing the PreparedStatement. Retrieve the effective username from the
Connection (via connection.getMetaData().getUserName() or a SELECT USER() query)
and use that value for the prepared statement parameter in hasLockWaits, keeping
the existing exclusions and timeout and preserving the existing error
handling/logging behavior in the catch block.
---
Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 421-448: The hasDeadlocks method currently treats any historical
"LATEST DETECTED DEADLOCK" as a permanent DEGRAGED signal; instead update
hasDeadlocks(Connection) to parse the timestamp of the latest deadlock from the
innodbStatus blob returned by "SHOW ENGINE INNODB STATUS" and only return true
if that timestamp is within a configurable recent window (e.g., 5 minutes); keep
the deadlockCheckDisabled handling and SQL error-code checks, extract the
timestamp by locating the "LATEST DETECTED DEADLOCK" block and parsing the
preceding "LATEST DETECTED DEADLOCK at YYYY-MM-DD HH:MM:SS" (or similar) string
into an Instant, compare to Instant.now() with Duration threshold, and fall back
to returning false if parsing fails (and log a debug message).
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 350-351: Replace the blocking advancedCheckLock.writeLock().lock()
call with advancedCheckLock.writeLock().lockInterruptibly() in HealthService so
the thread responds to cancel(true); wrap the lock acquisition in a try that
catches InterruptedException, reasserts the interrupt
(Thread.currentThread().interrupt()) and aborts/returns (or throws a checked
exception) so the outer deadline propagates; ensure the existing finally block
still calls advancedCheckLock.writeLock().unlock() only when the lock was
successfully acquired (track a boolean locked flag) to avoid
IllegalMonitorStateException.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)
450-466:hasSlowQueriesis not scoped to the app's own DB user — inconsistent withhasLockWaits.
hasLockWaitswas specifically scoped to the application user (per the commit message), buthasSlowQueriesstill uses a broad exclusion list. In a shared MySQL instance, other applications' long-running queries will cause MMU-API to reportDEGRADED. If the intent is to only monitor this application's health, consider applying the same user-scoping here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 450 - 466, hasSlowQueries currently checks slow queries across all DB users which can surface other apps' long-running queries; update hasSlowQueries to scope to the application DB user the same way hasLockWaits does: change the SQL to filter by the app user (e.g., "AND user = ?") instead of the broad NOT IN list, bind the app DB username (obtain it the same way hasLockWaits does or accept the configured DB user) when preparing the statement, and keep existing timeout/threshold logic and error handling; reference the hasSlowQueries method and align its user-scoping with hasLockWaits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 160-162: The generic catch block for "catch (Exception e)"
currently only logs the error via logger.warn("Health check execution error:
{}", e.getMessage()) and fails to cancel the pending redisFuture; update that
catch to mirror the TimeoutException/InterruptedException branches by calling
redisFuture.cancel(true) (and optionally mysqlFuture.cancel(true) for symmetry)
and handle/suppress any InterruptedException when waiting/cleaning up so the
redis task is not left running after mysqlFuture.get() throws; modify the catch
surrounding mysqlFuture.get() (the catch for Exception e) to perform these
cancel operations before logging.
- Around line 189-209: checkMySQLHealthSync currently runs
performAdvancedMySQLChecksWithThrottle inline, which can block beyond the outer
~4s future and cause false DOWN reports; refactor so checkMySQLHealthSync only
executes the quick "SELECT 1" and reads a short-lived cached result for advanced
checks, while moving the heavy work in performAdvancedMySQLChecksWithThrottle
into a background ScheduledExecutorService task that runs on a fixed cadence
(e.g., every 30s), performs the multi-query checks with setQueryTimeout, and
updates a thread-safe cache/flag (AtomicBoolean or a small TTL cache) that
checkMySQLHealthSync consults; ensure the new background task handles exceptions
and logs but does not block the health-check hot path, and remove inline
long-running queries from checkMySQLHealthSync.
- Around line 401-419: The SQL in hasLockWaits currently compares
PROCESSLIST.user to USER(), which returns user@host and never matches; update
the query used in hasLockWaits(Connection) to compare PROCESSLIST.user to
SUBSTRING_INDEX(USER(), '@', 1) (i.e., replace "AND user = USER()" with "AND
user = SUBSTRING_INDEX(USER(), '@', 1)") so the username portion is matched
correctly; keep the same PreparedStatement, timeout, and ResultSet handling in
hasLockWaits and preserve the existing exception logging behavior.
---
Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 421-448: hasDeadlocks() currently treats any historical "LATEST
DETECTED DEADLOCK" as a permanent failure; instead parse the "LATEST DETECTED
DEADLOCK" block returned by "SHOW ENGINE INNODB STATUS" in hasDeadlocks(),
extract the deadlock timestamp from that block and compare it to a configurable
recency threshold (e.g., deadlockAgeThresholdMillis) so only recent deadlocks
mark DEGRADED; keep the existing permission handling (deadlockCheckDisabled,
errorCode checks 1142/1227) and, if no timestamp found or parsing fails, fall
back to ignoring the deadlock (return false) rather than treating it as
perpetual.
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 450-466: hasSlowQueries currently checks slow queries across all
DB users which can surface other apps' long-running queries; update
hasSlowQueries to scope to the application DB user the same way hasLockWaits
does: change the SQL to filter by the app user (e.g., "AND user = ?") instead of
the broad NOT IN list, bind the app DB username (obtain it the same way
hasLockWaits does or accept the configured DB user) when preparing the
statement, and keep existing timeout/threshold logic and error handling;
reference the hasSlowQueries method and align its user-scoping with
hasLockWaits.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/iemr/mmu/service/health/HealthService.java (2)
488-506: Consider using Javarecordtypes for these simple data carriers.Both
HealthCheckResultandAdvancedCheckResultare immutable data holders with final fields — a natural fit forrecordclasses, which would reduce boilerplate.Example
- private static class AdvancedCheckResult { - final boolean isDegraded; - - AdvancedCheckResult(boolean isDegraded) { - this.isDegraded = isDegraded; - } - } + private record AdvancedCheckResult(boolean isDegraded) {} - private static class HealthCheckResult { - final boolean isHealthy; - final String error; - final boolean isDegraded; - - HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) { - this.isHealthy = isHealthy; - this.error = error; - this.isDegraded = isDegraded; - } - } + private record HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) {}Note: Record accessor names use the field name directly (e.g.,
result.isHealthy()instead ofresult.isHealthy), so usages would need updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 488 - 506, Replace the two private static classes AdvancedCheckResult and HealthCheckResult with Java record types to remove boilerplate: declare "private static record AdvancedCheckResult(boolean isDegraded)" and "private static record HealthCheckResult(boolean isHealthy, String error, boolean isDegraded)"; then update all usages of the old fields (e.g., result.isHealthy, result.error, result.isDegraded) to use record accessors (result.isHealthy(), result.error(), result.isDegraded()) throughout HealthService.
386-389: Catch-all markingisDegraded=trueis overly conservative.If any advanced sub-check throws an unexpected exception (e.g., a
ClassCastExceptionfrom JMX, or a transient SQL error inhasSlowQueries), the whole system is reported asDEGRADEDfor the next 30 seconds. Since these are supplementary diagnostics, a failure in the diagnostic machinery itself shouldn't degrade the reported health.Suggested fix — fail open
} catch (Exception e) { - logger.debug("Advanced MySQL checks encountered exception, marking degraded"); - return new AdvancedCheckResult(true); + logger.debug("Advanced MySQL checks encountered exception, skipping", e); + return new AdvancedCheckResult(false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 386 - 389, The catch-all currently returns new AdvancedCheckResult(true) which marks the system DEGRADED on any unexpected exception; change this to fail-open by returning new AdvancedCheckResult(false) instead, while still logging the exception (include e in the log call) so diagnostics are preserved—update the catch block in HealthService (the catch handling Advanced MySQL checks) to log the exception and return AdvancedCheckResult(false) rather than marking degraded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 189-204: The health check currently calls
performAdvancedMySQLChecksWithThrottle inline, which can run multiple DB queries
and exceed the future timeout; instead, refactor so the health path in
HealthService reads a cached advanced-check result and does not execute
performAdvancedMySQLChecksWithThrottle synchronously. Implement a background
scheduled task (e.g., ScheduledExecutorService or framework `@Scheduled`) that
periodically invokes performAdvancedMySQLChecksWithThrottle and updates a
thread-safe cache/flag, ensure the main health-check method returns the cached
isDegraded value (with a safe default/fallback if the cache is empty), and
remove any inline invocation of performAdvancedMySQLChecksWithThrottle so the
future deadline cannot be exceeded.
- Around line 156-158: In the generic Exception catch block for the health check
(where you currently log "Health check execution error: {}" using
e.getMessage()), ensure the other pending task is cancelled to avoid orphaned
work: call redisFuture.cancel(true) (and also mysqlFuture.cancel(true) if
appropriate) before logging; update the catch(Exception e) handling around
mysqlFuture.get() to mirror the TimeoutException/InterruptedException branches
by cancelling the counterpart futures (redisFuture and mysqlFuture) and keep the
existing logging behavior.
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 488-506: Replace the two private static classes
AdvancedCheckResult and HealthCheckResult with Java record types to remove
boilerplate: declare "private static record AdvancedCheckResult(boolean
isDegraded)" and "private static record HealthCheckResult(boolean isHealthy,
String error, boolean isDegraded)"; then update all usages of the old fields
(e.g., result.isHealthy, result.error, result.isDegraded) to use record
accessors (result.isHealthy(), result.error(), result.isDegraded()) throughout
HealthService.
- Around line 386-389: The catch-all currently returns new
AdvancedCheckResult(true) which marks the system DEGRADED on any unexpected
exception; change this to fail-open by returning new AdvancedCheckResult(false)
instead, while still logging the exception (include e in the log call) so
diagnostics are preserved—update the catch block in HealthService (the catch
handling Advanced MySQL checks) to log the exception and return
AdvancedCheckResult(false) rather than marking degraded.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)
107-107:newFixedThreadPool(6)maintains 4 permanently idle threads.
checkHealth()submits exactly 2 tasks at a time (MySQL + Redis). A pool of 2 suffices and avoids wasting resources.♻️ Proposed change
- this.executorService = Executors.newFixedThreadPool(6); + this.executorService = Executors.newFixedThreadPool(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` at line 107, HealthService creates an executorService with Executors.newFixedThreadPool(6) which keeps unnecessary idle threads; change the pool size to 2 (or make it configurable) since checkHealth() only submits two tasks (MySQL + Redis). Update the executorService initialization in the HealthService constructor (or the field where executorService is created) from newFixedThreadPool(6) to newFixedThreadPool(2) and, if desired, add a constant or config property for pool size so future changes are configurable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 398-423: The method handleAdvancedChecksFuture has a missing
closing brace for the final catch (Exception ex) block causing a compile error;
fix it by adding the missing '}' to close that catch block before the method's
final closing brace so the method compiles correctly—locate
handleAdvancedChecksFuture (uses ADVANCED_CHECKS_TIMEOUT_MS,
AdvancedCheckResult, future.get(...)) and ensure the catch (Exception ex) block
is properly terminated (so performAdvancedMySQLChecks() follows as a separate
method).
- Around line 370-395: Multiple threads can pass the write-lock double-check and
race to submit and update the cache, causing lost-writes; introduce an
AtomicBoolean inFlightAdvancedCheck (or similarly named) as a CAS guard so only
one thread performs submit→wait→update while others return the existing
cachedAdvancedCheckResult (or re-check under read lock) instead of performing
the submit; modify the flow around advancedCheckLock,
advancedCheckExecutor.submit(this::performAdvancedMySQLChecks),
handleAdvancedChecksFuture, lastAdvancedCheckTime and cachedAdvancedCheckResult
so threads do: if (!inFlightAdvancedCheck.compareAndSet(false,true)) return
cached value (re-read under read lock), otherwise submit the task, handle the
future, then inside the write lock update lastAdvancedCheckTime and
cachedAdvancedCheckResult and finally inFlightAdvancedCheck.set(false) in a
finally block.
---
Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 176-178: The generic Exception catch in HealthService (around the
awaitHealthChecks call) currently logs the error but doesn't cancel running
tasks, leaving redisFuture/orphaned tasks in executorService; modify that catch
block to invoke the same cancellation logic used in the TimeoutException and
InterruptedException branches (call cancelFutures with the relevant futures such
as redisFuture and any other scheduled futures) before logging and
rethrowing/handling, so all branches consistently cancel outstanding futures
created by awaitHealthChecks.
---
Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Line 107: HealthService creates an executorService with
Executors.newFixedThreadPool(6) which keeps unnecessary idle threads; change the
pool size to 2 (or make it configurable) since checkHealth() only submits two
tasks (MySQL + Redis). Update the executorService initialization in the
HealthService constructor (or the field where executorService is created) from
newFixedThreadPool(6) to newFixedThreadPool(2) and, if desired, add a constant
or config property for pool size so future changes are configurable.
|



📋 Description
JIRA ID:
This PR implements /health and /version endpoints in the MMU-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Chores