feat(health,version): add health and version endpoints#65
Merged
drtechie merged 4 commits intoPSMRI:mainfrom Mar 2, 2026
Merged
feat(health,version): add health and version endpoints#65drtechie merged 4 commits intoPSMRI:mainfrom
drtechie merged 4 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. WalkthroughAdds /health and /version REST endpoints, a concurrent HealthService with MySQL/Redis diagnostics, Maven plugins for jar and git metadata generation, and updates request filters/interceptor to allow unauthenticated access to the new endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HealthController
participant HealthService
participant MySQL
participant Redis
Client->>HealthController: GET /health
HealthController->>HealthService: checkHealth()
activate HealthService
par MySQL Check
HealthService->>MySQL: SELECT 1 (with timeout)
MySQL-->>HealthService: OK / error / timeout
Note over HealthService: Optional throttled advanced checks<br/>(lock waits, slow queries, pool metrics)
and Redis Check
HealthService->>Redis: PING
Redis-->>HealthService: PONG / error
end
HealthService->>HealthService: Aggregate component results<br/>Derive overall status
deactivate HealthService
HealthService-->>HealthController: health map
HealthController-->>Client: HTTP 200 or 503 (with payload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/com/iemr/tm/controller/version/VersionController.java (1)
50-68: Cachegit.properties— reloading from the classpath on every request is unnecessary.
git.propertiesis baked into the JAR/WAR at build time and never changes at runtime. Loading it once at construction time avoids repeated I/O on a potentially high-frequency polling endpoint.Also, the
logger.infocalls on Lines 53 and 66 will be noisy if the endpoint is polled frequently; considerlogger.debug.♻️ Proposed refactor — load once, cache the response map
`@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(); + } `@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)); - response.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); - response.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); - response.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); - } catch (Exception e) { - logger.error("Failed to load version information", e); - response.put("buildTimestamp", UNKNOWN_VALUE); - response.put("version", UNKNOWN_VALUE); - response.put("branch", UNKNOWN_VALUE); - response.put("commitHash", UNKNOWN_VALUE); - } - logger.info("version Controller End"); + logger.debug("Version endpoint called"); + return ResponseEntity.ok(versionInfo); + } + + private Map<String, String> loadVersionInfo() { + Map<String, String> response = new LinkedHashMap<>(); + try { + Properties gitProperties = loadGitProperties(); + response.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE)); + response.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); + response.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); + response.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); + } catch (Exception e) { + logger.error("Failed to load version information", e); + response.put("buildTimestamp", UNKNOWN_VALUE); + response.put("version", UNKNOWN_VALUE); + response.put("branch", UNKNOWN_VALUE); + response.put("commitHash", UNKNOWN_VALUE); + } return ResponseEntity.ok(response); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/controller/version/VersionController.java` around lines 50 - 68, Cache the git.properties and the response map instead of reloading on every request: call loadGitProperties() once during controller construction (or in a `@PostConstruct` init) and build a cached Map<String,String> (the same shape returned by versionInformation()) using UNKNOWN_VALUE defaults; have versionInformation() simply return ResponseEntity.ok(cachedResponse). Also change the noisy logger.info calls in versionInformation() to logger.debug() (or remove them) so frequent polling isn't noisy while preserving logger.error in the catch path.src/main/java/com/iemr/tm/service/health/HealthService.java (2)
102-112: Thread pool is oversized — only 2 tasks are ever submitted concurrently.
Executors.newFixedThreadPool(6)allocates 6 threads, butperformHealthChecksonly submits 2 futures (MySQL + Redis). A pool of 2 (or a cached pool) would be more appropriate and avoids idle thread overhead.♻️ Suggested 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/tm/service/health/HealthService.java` around lines 102 - 112, The constructor in HealthService currently creates executorService with Executors.newFixedThreadPool(6) but performHealthChecks only submits two concurrent tasks (MySQL and Redis); change the executorService creation (in the HealthService(DataSource,... ) constructor) to use a smaller pool such as Executors.newFixedThreadPool(2) or an appropriate cached/sizing strategy (e.g., Executors.newCachedThreadPool()) to avoid over-provisioning threads while keeping advancedCheckExecutor as-is.
422-429: Advanced checks acquire a separate connection — risk of self-induced pool exhaustion.
performAdvancedMySQLChecksopens a second connection from the pool (in addition to the one used by the basicSELECT 1check). Under pool-exhaustion conditions, this could block or contribute to the very problem it's trying to detect. Consider reusing the connection from the basic check, or setting a shortconnectionTimeoutfor this call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 422 - 429, performAdvancedMySQLChecks opens a fresh pool connection (dataSource.getConnection()) which risks exhausting the pool under load; either accept and bound that call with a very short acquisition timeout or reuse the already-open connection from the basic check and pass it into performAdvancedCheckLogic. Modify performAdvancedMySQLChecks to: 1) prefer accepting a Connection parameter (or overload) so performAdvancedCheckLogic can run on the same Connection used by the basic SELECT 1 check, or 2) if a separate getConnection() is required, acquire it with a short connectionTimeout/try-with-timeout and fail fast and log the timeout error; ensure you reference performAdvancedMySQLChecks and performAdvancedCheckLogic when making the change and keep proper closing behavior (try-with-resources) for any connection you open.pom.xml (1)
300-304:maven-jar-plugin3.0.2 is outdated and unnecessary for a WAR project.The project packaging is
war(Line 9). The jar plugin declaration has no custom configuration and likely serves no purpose here. If it is needed (e.g., for Spring Boot repackaging interactions), consider upgrading — 3.0.2 dates from 2016.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 300 - 304, The maven-jar-plugin declaration (artifactId: maven-jar-plugin, version: 3.0.2) is unnecessary for a WAR-packaged project; remove the <plugin> block for org.apache.maven.plugins:maven-jar-plugin entirely from the POM to avoid useless configuration, or if you actually need it for a special workflow (e.g., Spring Boot repackaging) replace the version with a current stable release and add the required configuration for that use case instead.
🤖 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/tm/service/health/HealthService.java`:
- Around line 394-420: The method handleAdvancedChecksFuture currently
re-interrupts the current thread when catching ExecutionException whose cause is
InterruptedException; instead, in the ExecutionException branch (inside
handleAdvancedChecksFuture) do NOT call Thread.currentThread().interrupt() when
ex.getCause() instanceof InterruptedException — simply log that the worker was
interrupted and return new AdvancedCheckResult(true); keep the existing
Thread.currentThread().interrupt() only in the explicit catch
(InterruptedException ex) block (which indicates the caller thread was
interrupted). Ensure references: handleAdvancedChecksFuture,
Future<AdvancedCheckResult> future, ExecutionException, InterruptedException,
ADVANCED_CHECKS_TIMEOUT_MS and AdvancedCheckResult.
In `@src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java`:
- Around line 61-64: The current change in HTTPRequestInterceptor (in the method
that checks the Authorization header) incorrectly returns true for any request
with a missing/empty Authorization header, bypassing authentication globally;
update the condition so the interceptor only skips validation when the header is
missing AND the request path is one of the intended public endpoints (e.g.,
"/health" or "/version"). In practice, read the request URI (via the
HttpServletRequest used by HTTPRequestInterceptor), and only return true for
missing/empty authorization if the URI equals or startsWith the allowed public
paths; otherwise preserve the previous behavior and allow the request to fall
through to the default rejection logic. Ensure you reference the same
Authorization check block and do not remove the default/null-check branch that
rejects unauthenticated requests for non-public paths.
---
Nitpick comments:
In `@pom.xml`:
- Around line 300-304: The maven-jar-plugin declaration (artifactId:
maven-jar-plugin, version: 3.0.2) is unnecessary for a WAR-packaged project;
remove the <plugin> block for org.apache.maven.plugins:maven-jar-plugin entirely
from the POM to avoid useless configuration, or if you actually need it for a
special workflow (e.g., Spring Boot repackaging) replace the version with a
current stable release and add the required configuration for that use case
instead.
In `@src/main/java/com/iemr/tm/controller/version/VersionController.java`:
- Around line 50-68: Cache the git.properties and the response map instead of
reloading on every request: call loadGitProperties() once during controller
construction (or in a `@PostConstruct` init) and build a cached Map<String,String>
(the same shape returned by versionInformation()) using UNKNOWN_VALUE defaults;
have versionInformation() simply return ResponseEntity.ok(cachedResponse). Also
change the noisy logger.info calls in versionInformation() to logger.debug() (or
remove them) so frequent polling isn't noisy while preserving logger.error in
the catch path.
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 102-112: The constructor in HealthService currently creates
executorService with Executors.newFixedThreadPool(6) but performHealthChecks
only submits two concurrent tasks (MySQL and Redis); change the executorService
creation (in the HealthService(DataSource,... ) constructor) to use a smaller
pool such as Executors.newFixedThreadPool(2) or an appropriate cached/sizing
strategy (e.g., Executors.newCachedThreadPool()) to avoid over-provisioning
threads while keeping advancedCheckExecutor as-is.
- Around line 422-429: performAdvancedMySQLChecks opens a fresh pool connection
(dataSource.getConnection()) which risks exhausting the pool under load; either
accept and bound that call with a very short acquisition timeout or reuse the
already-open connection from the basic check and pass it into
performAdvancedCheckLogic. Modify performAdvancedMySQLChecks to: 1) prefer
accepting a Connection parameter (or overload) so performAdvancedCheckLogic can
run on the same Connection used by the basic SELECT 1 check, or 2) if a separate
getConnection() is required, acquire it with a short
connectionTimeout/try-with-timeout and fail fast and log the timeout error;
ensure you reference performAdvancedMySQLChecks and performAdvancedCheckLogic
when making the change and keep proper closing behavior (try-with-resources) for
any connection you open.
src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/iemr/tm/service/health/HealthService.java (3)
456-493: Consider extracting magic numbers inhasSlowQueriesinto named constants.The query threshold
10(seconds) and count threshold3are hardcoded at lines 482 and 486. These are diagnostic tuning parameters that are easy to miss during maintenance.Proposed fix
+ private static final int SLOW_QUERY_TIME_THRESHOLD_SECONDS = 10; + private static final int SLOW_QUERY_COUNT_THRESHOLD = 3; + // ... in hasSlowQueries: - stmt.setInt(1, 10); + stmt.setInt(1, SLOW_QUERY_TIME_THRESHOLD_SECONDS); try (ResultSet rs = stmt.executeQuery()) { if (rs.next()) { int slowQueryCount = rs.getInt(1); - return slowQueryCount > 3; + return slowQueryCount > SLOW_QUERY_COUNT_THRESHOLD;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/tm/service/health/HealthService.java` around lines 456 - 493, Extract the magic numbers in hasSlowQueries into descriptive class-level constants (e.g., private static final int SLOW_QUERY_TIME_THRESHOLD_SEC = 10 and private static final int SLOW_QUERY_COUNT_THRESHOLD = 3), replace the literal 10 used in stmt.setInt(1, 10) with SLOW_QUERY_TIME_THRESHOLD_SEC and replace the literal 3 in the return comparison (slowQueryCount > 3) with SLOW_QUERY_COUNT_THRESHOLD so the thresholds are configurable and clearly documented.
102-112: Thread pool of 6 is oversized for 2 concurrent health-check tasks.
executorServiceis a fixed pool of 6 threads, butperformHealthChecksonly ever submits 2 tasks (MySQL + Redis). Consider sizing the pool to match actual concurrency, or using a cached thread pool if you anticipate adding more components later.Proposed fix
- 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/tm/service/health/HealthService.java` around lines 102 - 112, The fixed thread pool for executorService in the HealthService constructor is oversized (6) for only two concurrent health-check tasks; update the constructor to use a right-sized pool (e.g., Executors.newFixedThreadPool(2)) or swap to a cached/thread-per-task strategy (Executors.newCachedThreadPool()) if you expect more checks later, by changing the executorService initialization in the HealthService constructor; ensure any change still satisfies performHealthChecks and related shutdown behavior.
253-295:performHealthCheckreturn value is unused — consider making itvoid.The method returns
Map<String, Object>(lines 282, 293), but all callers invoke it in aRunnablelambda (lines 163, 165) discarding the return. The status is populated entirely via the side-effectingstatusparameter. The return value is misleading.Proposed fix
- private Map<String, Object> performHealthCheck(String componentName, + private void performHealthCheck(String componentName, Map<String, Object> status, Supplier<HealthCheckResult> checker) { long startTime = System.currentTimeMillis(); try { HealthCheckResult result = checker.get(); long responseTime = System.currentTimeMillis() - startTime; // ... status population ... - return status; } catch (Exception e) { long responseTime = System.currentTimeMillis() - startTime; logger.error("{} health check failed with exception: {}", componentName, e.getMessage(), e); // ... status population ... - 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/tm/service/health/HealthService.java` around lines 253 - 295, performHealthCheck currently returns Map<String,Object> but callers ignore the return and rely on the side-effecting status parameter; change the method signature to void performHealthCheck(String componentName, Map<String,Object> status, Supplier<HealthCheckResult> checker), remove all "return status" statements (use plain returns/void exit), and keep all existing status.put(...) and exception handling as-is; update any callers that assume a return value (e.g., Runnable lambdas invoking performHealthCheck) to simply call the method without using its result, and adjust method Javadoc/visibility if present.
🤖 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/tm/service/health/HealthService.java`:
- Around line 515-532: The debug message "Pool exhaustion check disabled:
HikariCP metrics unavailable" is always logged after the loop in
checkPoolMetricsViaJMX(), even when MBeans were found and evaluated; update
checkPoolMetricsViaJMX to log that message only when no MBeans are available or
an exception occurred (i.e., move or conditionally emit the logger.debug into
the catch block and/or after checking that mBeans is empty or null), and keep
the current early return behavior when evaluatePoolMetrics(...) returns true;
reference the checkPoolMetricsViaJMX method and the evaluatePoolMetrics call to
locate where the logging/control flow should be modified.
---
Nitpick comments:
In `@src/main/java/com/iemr/tm/service/health/HealthService.java`:
- Around line 456-493: Extract the magic numbers in hasSlowQueries into
descriptive class-level constants (e.g., private static final int
SLOW_QUERY_TIME_THRESHOLD_SEC = 10 and private static final int
SLOW_QUERY_COUNT_THRESHOLD = 3), replace the literal 10 used in stmt.setInt(1,
10) with SLOW_QUERY_TIME_THRESHOLD_SEC and replace the literal 3 in the return
comparison (slowQueryCount > 3) with SLOW_QUERY_COUNT_THRESHOLD so the
thresholds are configurable and clearly documented.
- Around line 102-112: The fixed thread pool for executorService in the
HealthService constructor is oversized (6) for only two concurrent health-check
tasks; update the constructor to use a right-sized pool (e.g.,
Executors.newFixedThreadPool(2)) or swap to a cached/thread-per-task strategy
(Executors.newCachedThreadPool()) if you expect more checks later, by changing
the executorService initialization in the HealthService constructor; ensure any
change still satisfies performHealthChecks and related shutdown behavior.
- Around line 253-295: performHealthCheck currently returns Map<String,Object>
but callers ignore the return and rely on the side-effecting status parameter;
change the method signature to void performHealthCheck(String componentName,
Map<String,Object> status, Supplier<HealthCheckResult> checker), remove all
"return status" statements (use plain returns/void exit), and keep all existing
status.put(...) and exception handling as-is; update any callers that assume a
return value (e.g., Runnable lambdas invoking performHealthCheck) to simply call
the method without using its result, and adjust method Javadoc/visibility if
present.
|
drtechie
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Description
JIRA ID:
This PR implements /health and /version endpoints in the Scheduler-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Chores