Feature/health check plugin + Metric Plugin - #105
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds HealthCheckPlugin and MetricPlugin endpoints and registers them; augments HttpRequest with creationTimeNanos; introduces frontend health UI (HTML/CSS/JS) that polls /metric; adds build-time filtering and build.properties; includes a unit test for MetricPlugin. Changes
Sequence DiagramsequenceDiagram
participant Client as Browser Client
participant Server as HTTP Server
participant MetricPlugin as MetricPlugin
participant System as JVM/System
Client->>Server: GET /metric
Server->>MetricPlugin: handle(request, response)
MetricPlugin->>System: read build.properties
MetricPlugin->>System: Runtime.getRuntime() (memory)
MetricPlugin->>System: System.nanoTime() (now)
MetricPlugin->>MetricPlugin: compute responseTimeUs (now - request.creationTimeNanos)
MetricPlugin->>Server: set 200, headers, JSON body
Server->>Client: HTTP 200 + JSON (metrics)
Client->>Client: parse JSON & update DOM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~27 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/plugin/HealthCheckPlugin.java (2)
27-28: Compute the UTF-8 bytes once and reuse.
getBytes(StandardCharsets.UTF_8)is called twice — once to measure the length and again to set the body — resulting in two separate encode operations and two heap allocations. Compute it once and reference the array for both theContent-Lengthheader andsetBody.♻️ Proposed refactor
- res.setHeader("Content-Length", String.valueOf(jsonBody.getBytes(StandardCharsets.UTF_8).length)); - res.setBody(jsonBody.getBytes(StandardCharsets.UTF_8)); + byte[] bodyBytes = jsonBody.getBytes(StandardCharsets.UTF_8); + res.setHeader("Content-Length", String.valueOf(bodyBytes.length)); + res.setBody(bodyBytes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` around lines 27 - 28, The code calls jsonBody.getBytes(StandardCharsets.UTF_8) twice causing two encodings and allocations; compute the UTF-8 byte[] once (e.g., byte[] bodyBytes = jsonBody.getBytes(StandardCharsets.UTF_8)) and reuse that array for both res.setHeader("Content-Length", String.valueOf(bodyBytes.length)) and res.setBody(bodyBytes) inside the HealthCheckPlugin response handling so the header length and body come from the same bytes.
19-22: Extract the hardcoded server version string to a named constant.The inline literal
"juv25d-webserver/1.0"will need to be updated in sync with any future versioning change. A static constant is easier to maintain and update.♻️ Proposed refactor
public class HealthCheckPlugin implements Plugin { + private static final String SERVER_NAME = "juv25d-webserver/1.0"; `@Override` public void handle(HttpRequest req, HttpResponse res) throws IOException { String timestamp = Instant.now().toString(); String jsonBody = String.format( - "{\"status\": \"UP\", \"timestamp\": \"%s\", \"server\": \"juv25d-webserver/1.0\"}", + "{\"status\": \"UP\", \"timestamp\": \"%s\", \"server\": \"%s\"}", - timestamp + timestamp, SERVER_NAME );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` around lines 19 - 22, Replace the hardcoded server version literal in the JSON body with a named constant: add a private static final String SERVER_VERSION = "juv25d-webserver/1.0" to the HealthCheckPlugin class and update the String.format call (in the method that builds jsonBody) to use SERVER_VERSION instead of the inline literal; ensure any other occurrences in HealthCheckPlugin reference this constant so future version changes require only one edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Around line 27-28: The code calls jsonBody.getBytes(StandardCharsets.UTF_8)
twice causing two encodings and allocations; compute the UTF-8 byte[] once
(e.g., byte[] bodyBytes = jsonBody.getBytes(StandardCharsets.UTF_8)) and reuse
that array for both res.setHeader("Content-Length",
String.valueOf(bodyBytes.length)) and res.setBody(bodyBytes) inside the
HealthCheckPlugin response handling so the header length and body come from the
same bytes.
- Around line 19-22: Replace the hardcoded server version literal in the JSON
body with a named constant: add a private static final String SERVER_VERSION =
"juv25d-webserver/1.0" to the HealthCheckPlugin class and update the
String.format call (in the method that builds jsonBody) to use SERVER_VERSION
instead of the inline literal; ensure any other occurrences in HealthCheckPlugin
reference this constant so future version changes require only one edit.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/org/juv25d/plugin/HealthCheckPlugin.java (3)
28-28: Consider declaring the charset in theContent-Typeheader.
application/jsonis RFC 8259-compliant without a charset parameter (UTF-8 is implied), so this is not a bug. Adding; charset=utf-8makes the encoding contract explicit for clients that do not implement the RFC default.♻️ Proposed change
- res.setHeader("Content-Type", "application/json"); + res.setHeader("Content-Type", "application/json; charset=utf-8");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` at line 28, The Content-Type header set in HealthCheckPlugin currently uses "application/json" without an explicit charset; update the response header in the code that calls res.setHeader to include the charset (e.g., "application/json; charset=utf-8") so clients receive an explicit UTF-8 encoding contract—locate the res.setHeader call in HealthCheckPlugin and change its value accordingly.
20-23: Manual JSON construction has no escaping — fragile for future extension.
Instant.now().toString()and the hardcodedSERVER_NAMEare both free of JSON-unsafe characters today, so there is no immediate bug. However,String.formatperforms zero JSON escaping; any future field sourced from dynamic input (e.g., a host name, error message, or environment variable) will silently produce malformed or injectable JSON.Since the project intentionally avoids external dependencies, consider a small dedicated helper, or at minimum document the no-escaping assumption in a comment so future contributors don't add dynamic fields without noticing the risk.
♻️ Proposed minimal safe pattern (stdlib only)
- String jsonBody = String.format( - "{\"status\": \"UP\", \"timestamp\": \"%s\", \"server\": \"%s\"}", - timestamp, SERVER_NAME - ); + // Values are controlled (ISO-8601 instant + hardcoded literal) — no escaping needed. + // If any dynamic field is added, escape it with jsonEscape() first. + String jsonBody = "{\"status\": \"UP\", \"timestamp\": \"" + timestamp + + "\", \"server\": \"" + SERVER_NAME + "\"}";Or, if a lightweight JSON dependency is ever introduced (e.g., Jackson, Gson, or
org.json), replace the entire block with a proper object-to-JSON serialisation call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` around lines 20 - 23, The JSON body is built with String.format without escaping, which will break if timestamp or SERVER_NAME become JSON-unsafe; add a small stdlib JSON-escaping helper (e.g., a private static String escapeJson(String s) in HealthCheckPlugin) and use it when constructing jsonBody (escape timestamp and SERVER_NAME) instead of raw %s, or alternatively add a clear comment above the jsonBody construction that documents the no-escaping assumption; reference jsonBody, SERVER_NAME, timestamp and the new escapeJson helper to locate the change.
25-26: Redundant status-code and status-text setters.
HttpResponse's no-arg constructor already initialisesstatusCode = 200andstatusText = "OK"(seeHttpResponse.java, lines 16–19). Explicitly re-setting them here adds noise without effect.♻️ Proposed cleanup
- res.setStatusCode(200); - res.setStatusText("OK"); byte[] bodyBytes = jsonBody.getBytes(StandardCharsets.UTF_8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` around lines 25 - 26, In HealthCheckPlugin.java remove the redundant explicit settings of the default HTTP status in the method that builds the response (the two calls res.setStatusCode(200) and res.setStatusText("OK")); HttpResponse's no-arg constructor already initializes statusCode to 200 and statusText to "OK", so simply return the constructed HttpResponse instance (created via new HttpResponse()) without re-setting those fields to avoid noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Line 28: The Content-Type header set in HealthCheckPlugin currently uses
"application/json" without an explicit charset; update the response header in
the code that calls res.setHeader to include the charset (e.g.,
"application/json; charset=utf-8") so clients receive an explicit UTF-8 encoding
contract—locate the res.setHeader call in HealthCheckPlugin and change its value
accordingly.
- Around line 20-23: The JSON body is built with String.format without escaping,
which will break if timestamp or SERVER_NAME become JSON-unsafe; add a small
stdlib JSON-escaping helper (e.g., a private static String escapeJson(String s)
in HealthCheckPlugin) and use it when constructing jsonBody (escape timestamp
and SERVER_NAME) instead of raw %s, or alternatively add a clear comment above
the jsonBody construction that documents the no-escaping assumption; reference
jsonBody, SERVER_NAME, timestamp and the new escapeJson helper to locate the
change.
- Around line 25-26: In HealthCheckPlugin.java remove the redundant explicit
settings of the default HTTP status in the method that builds the response (the
two calls res.setStatusCode(200) and res.setStatusText("OK")); HttpResponse's
no-arg constructor already initializes statusCode to 200 and statusText to "OK",
so simply return the constructed HttpResponse instance (created via new
HttpResponse()) without re-setting those fields to avoid noise.
…gin responses Integrate `build.properties` for versioning and metadata. Enhance `/health` endpoint with response time, memory usage, and build details. Add UI widget for health status and dynamic frontend updates.
…gin responses Integrate `build.properties` for versioning and metadata. Enhance `/health` endpoint with response time, memory usage, and build details. Add UI widget for health status and dynamic frontend updates.
Refine response time calculations from milliseconds to microseconds. Update button interaction styles and enhance accuracy of frontend health status presentation.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/main/resources/static/js/health.js (2)
25-25: 1-second polling is aggressive for a status widget.Each open browser tab generates 3,600 requests/hour. A 5–10 second interval would meaningfully reduce server load while still providing timely updates.
♻️ Proposed change
-setInterval(loadHealth, 1_000); +setInterval(loadHealth, 10_000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/health.js` at line 25, The polling interval for the status widget is too aggressive: replace the current setInterval call that invokes loadHealth every 1_000 ms with a less frequent interval (e.g., 5_000–10_000 ms) to reduce server load; update the setInterval(loadHealth, 1_000) invocation in the file so it uses a 5000 (or 10000) millisecond interval instead while keeping the same loadHealth function and semantics.
19-21: Error handler leaves stale values in time/version/commit fields.After a successful load followed by a fetch failure,
health-statusswitches toDOWNbutlocal-time,utc-time,health-version, etc. retain their last known values. This presents a mixed/misleading state.♻️ Proposed fix: clear all fields on error
} catch (e) { document.getElementById("health-status").textContent = "DOWN"; + document.getElementById("local-time").textContent = ""; + document.getElementById("utc-time").textContent = ""; + document.getElementById("health-version").textContent = ""; + document.getElementById("health-commit").textContent = ""; + document.getElementById("health-response").textContent = ""; + document.getElementById("health-memory").textContent = ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/health.js` around lines 19 - 21, The catch block that currently only sets document.getElementById("health-status").textContent = "DOWN" leaves stale values in other fields; update the error handler in health.js to also clear or set to a neutral value (e.g., empty string or "N/A") the other DOM nodes such as "local-time", "utc-time", "health-version", "health-commit" (or any similar IDs used for time/version/commit) so that when the fetch fails all related fields are reset instead of showing outdated data.pom.xml (1)
229-244:buildnumber-maven-pluginin<pluginManagement>is redundant and missing its<configuration>.The plugin is already fully declared and configured in
<plugins>(lines 187–204). In a single-module project,<pluginManagement>entries are not executed independently — they only provide defaults to be inherited. Having the same plugin in both sections with a partial configuration (nodoCheck/doUpdate/shortRevisionLength) creates a maintenance hazard if the<plugins>entry is ever removed.🧹 Proposed fix: remove the redundant pluginManagement block
- <pluginManagement> - <plugins> - <plugin> - <groupId>org.codehaus.mojo</groupId> - <artifactId>buildnumber-maven-plugin</artifactId> - <version>3.2.0</version> - <executions> - <execution> - <phase>validate</phase> - <goals> - <goal>create</goal> - </goals> - </execution> - </executions> - </plugin> - </plugins> - </pluginManagement>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 229 - 244, The POM contains a redundant buildnumber-maven-plugin entry under pluginManagement that is incomplete (missing configuration like doCheck, doUpdate, shortRevisionLength) while a full declaration already exists under plugins; remove the plugin element from the pluginManagement section so there is a single authoritative declaration of org.codehaus.mojo:buildnumber-maven-plugin (the entry with execution goal "create" in pluginManagement) and rely on the fully-configured plugin in the <plugins> section to avoid configuration drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 246-251: The resources block in pom.xml currently sets filtering
true for the entire src/main/resources which causes Maven to attempt property
substitution in all files (including static JS like health.js) and can corrupt
binaries; change the Maven resource configuration to only filter a dedicated
filtered resource directory (e.g., src/main/resources-filtered) containing
build.properties and leave src/main/resources unfiltered, then move
build.properties into src/main/resources-filtered and update the <resources> and
<filteredResources> (or separate <resource> entries) so only the filtered
directory has <filtering>true</filtering>.
In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Around line 51-74: The JSON string built in HealthCheckPlugin (the multi-line
String.format block that interpolates localTime, utcTime, SERVER_NAME, version,
commit, responseTimeMs, usedMemory, maxMemory) injects version and commit raw,
risking broken JSON if they contain " or \; update the code to escape those
fields before interpolation (e.g., replace backslashes and quotes or better:
construct the response with a JSON builder/mapper such as Jackson/Gson or
org.json to serialize a Map/object) so version and commit are safely serialized
into the JSON output.
- Around line 27-37: The build.properties loading is happening per-request; move
that I/O into a one-time initialization by reading build.properties in the
HealthCheckPlugin constructor or a static initializer and storing the values in
final fields (e.g. private final String version; private final String commit)
instead of reloading the Properties on each /health request; update the request
handler (the method that currently reads props) to simply use these cached
version and commit fields.
In `@src/main/resources/static/css/styles.css`:
- Around line 43-78: Remove the blank lines that trigger Stylelint's
declaration-empty-line-before rule: in the `#health-box` button block (referenced
by selector "#health-box button") delete the empty line immediately before the
"transition" declaration so it directly follows the preceding rule, and in the
.health-content block (selector ".health-content") delete the empty line
immediately before the "position: absolute" declaration so that property sits
directly after the preceding declaration; preserve existing indentation and
spacing for other properties.
In `@src/main/resources/static/index.html`:
- Line 11: The button element with content "🩺" (the health button) is missing
an aria-label for screen readers; update the <button
popovertarget="health-content"> element to include a descriptive aria-label (for
example aria-label="Show health panel" or "Open health status") so that
assistive technologies convey the button's action clearly.
In `@src/main/resources/static/js/health.js`:
- Around line 3-4: The fetch result handling in health.js must guard the
response before calling res.json(): check res.ok after const res = await
fetch(...) and handle non-2xx responses (e.g., log or read an error body and set
the health state to DOWN) instead of unconditionally calling const data = await
res.json(); update the code around the res and data variables so non-ok
responses are parsed/treated as errors and only successful responses are passed
to the existing health payload handling logic.
---
Nitpick comments:
In `@pom.xml`:
- Around line 229-244: The POM contains a redundant buildnumber-maven-plugin
entry under pluginManagement that is incomplete (missing configuration like
doCheck, doUpdate, shortRevisionLength) while a full declaration already exists
under plugins; remove the plugin element from the pluginManagement section so
there is a single authoritative declaration of
org.codehaus.mojo:buildnumber-maven-plugin (the entry with execution goal
"create" in pluginManagement) and rely on the fully-configured plugin in the
<plugins> section to avoid configuration drift.
In `@src/main/resources/static/js/health.js`:
- Line 25: The polling interval for the status widget is too aggressive: replace
the current setInterval call that invokes loadHealth every 1_000 ms with a less
frequent interval (e.g., 5_000–10_000 ms) to reduce server load; update the
setInterval(loadHealth, 1_000) invocation in the file so it uses a 5000 (or
10000) millisecond interval instead while keeping the same loadHealth function
and semantics.
- Around line 19-21: The catch block that currently only sets
document.getElementById("health-status").textContent = "DOWN" leaves stale
values in other fields; update the error handler in health.js to also clear or
set to a neutral value (e.g., empty string or "N/A") the other DOM nodes such as
"local-time", "utc-time", "health-version", "health-commit" (or any similar IDs
used for time/version/commit) so that when the fetch fails all related fields
are reset instead of showing outdated data.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/org/juv25d/plugin/HealthCheckPlugin.java`:
- Around line 42-43: Rename the microsecond-valued field and references from
responseTimeMs to responseTimeUs: change the variable in HealthCheckPlugin from
responseTimeMs to responseTimeUs (computed as (System.nanoTime() -
req.creationTimeNanos()) / 1_000), update the JSON key emitted by
HealthCheckPlugin from "responseTimeMs" to "responseTimeUs", update the frontend
health.js to read data.responseTimeUs instead of data.responseTimeMs, and update
the assertion in HealthCheckPluginTest to expect "responseTimeUs".
---
Duplicate comments:
In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Around line 27-34: The HealthCheckPlugin currently performs classpath I/O on
every /health request by loading build.properties inside the request handler;
move that loading into the class constructor or a static initializer and store
the parsed values in instance (or static) fields (e.g., buildVersion,
buildTimestamp) so the request method only reads the cached fields. Update the
code that currently uses Properties props in the request path to reference those
cached fields and remove the getResourceAsStream call from the per-request logic
in HealthCheckPlugin.
- Around line 51-74: The JSON construction in HealthCheckPlugin where jsonBody
is built interpolates version and commit raw (variables version and commit),
which breaks JSON if they contain " or \; replace this manual text block with a
proper JSON builder/serializer (e.g., construct an object and call
ObjectMapper.writeValueAsString or use JSONObject) to produce the payload, or at
minimum escape backslashes and quotes in version and commit before interpolation
(e.g., replace \ with \\ and " with \"); update the jsonBody creation code to
use the chosen approach so version and commit are safely serialized.
In `@src/main/resources/static/css/styles.css`:
- Around line 52-53: Remove the empty line immediately before the transition
declaration to satisfy the Stylelint declaration-empty-line-before rule; locate
the CSS rule containing the "transition: 150ms ease;" declaration and ensure
there's no blank line above it (or, if intentional, add a rule-specific
Stylelint disable comment for the property), then run Stylelint to confirm the
violation is resolved.
- Around line 73-74: Remove the empty line immediately before the CSS
declaration "position: absolute" to satisfy the stylelint rule
`declaration-empty-line-before`; locate the "position" declaration in the
stylesheet (the rule containing "position: absolute") and ensure there is no
blank line separating it from the previous declaration or selector, so the
declarations are contiguous and pass the linter.
In `@src/main/resources/static/js/health.js`:
- Around line 3-4: The code fetches "/health" and calls res.json()
unconditionally (res and data in this snippet); add a res.ok check after
fetching so non-2xx responses are not parsed as a valid health payload—if
!res.ok, either throw an Error or construct a fallback health object with status
"DOWN" (and optionally include res.status/text) so the catch path runs; update
the block around the const res = await fetch("/health") / const data = await
res.json() to perform this check and handle non-ok responses accordingly.
…esilience; update resource filtering and enhance accessibility for health status UI
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/juv25d/plugin/HealthCheckPlugin.java (2)
34-34: ReplaceSystem.out.printlnwith a proper logger.Plain stdout output is not structured, not level-controlled, and easily lost in production.
♻️ Proposed refactor
+import java.util.logging.Level; +import java.util.logging.Logger; public class HealthCheckPlugin implements Plugin { + private static final Logger LOGGER = + Logger.getLogger(HealthCheckPlugin.class.getName()); private static final String SERVER_NAME = "juv25d-webserver"; ... - System.out.println("Error loading build.properties: " + e.getMessage()); + LOGGER.log(Level.WARNING, "Error loading build.properties", e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` at line 34, Replace the plain System.out.println in HealthCheckPlugin with a proper logger: add a class-level logger (e.g., org.slf4j.Logger or java.util.logging.Logger) in HealthCheckPlugin and change the System.out.println("Error loading build.properties: " + e.getMessage()) to a logger.error (or logger.log with SEVERE) call that includes both a descriptive message and the exception (pass the exception object for stacktrace); also add the necessary import and initialize the logger (e.g., getLogger for HealthCheckPlugin).
60-65: Capture a single timestamp, then format it in both zones.Two back-to-back
ZonedDateTime.now()calls can return different instants if the clock ticks between them.♻️ Proposed refactor
- String localTime = ZonedDateTime - .now(ZoneId.systemDefault()) - .format(TIME_FORMAT); - String utcTime = ZonedDateTime - .now(ZoneId.of("UTC")) - .format(TIME_FORMAT); + ZonedDateTime nowLocal = ZonedDateTime.now(ZoneId.systemDefault()); + String localTime = nowLocal.format(TIME_FORMAT); + String utcTime = nowLocal.withZoneSameInstant(ZoneId.of("UTC")).format(TIME_FORMAT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` around lines 60 - 65, In HealthCheckPlugin, avoid calling ZonedDateTime.now() twice; capture a single instant (e.g., Instant.now() or a single ZonedDateTime variable) and then derive both local and UTC formatted strings from that same instant using TIME_FORMAT so localTime and utcTime reflect the exact same instant; update the code that sets localTime and utcTime to use that single captured timestamp and then convert/format it for ZoneId.systemDefault() and ZoneId.of("UTC").src/main/resources/static/js/health.js (1)
22-24: Catch block leaves other fields showing stale data.When
loadHealth()fails, onlyhealth-statusis updated to "DOWN" while the previous values forlocal-time,utc-time,health-version,health-commit,health-response, andhealth-memoryremain visible — mixing a failure state with stale success data.♻️ Proposed refactor
} catch (e) { document.getElementById("health-status").textContent = "DOWN"; + ["local-time", "utc-time", "health-version", + "health-commit", "health-response", "health-memory"] + .forEach(id => { + const el = document.getElementById(id); + if (el) el.textContent = "—"; + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/health.js` around lines 22 - 24, When loadHealth() catches an error, it only sets the "health-status" to "DOWN" and leaves other DOM fields showing stale data; update the catch block in loadHealth() to also reset/clear the other elements (document.getElementById("local-time"), "utc-time", "health-version", "health-commit", "health-response", "health-memory") to a safe fallback value (e.g., "-" or empty string) so no stale success data remains visible when an error occurs.
🤖 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/resources/static/js/health.js`:
- Around line 17-18: The code accesses data.memory.usedBytes and
data.memory.maxBytes directly (usedMb/maxMb) which throws if data.memory is
undefined; before computing usedMb/maxMb or updating the DOM, guard that data &&
data.memory exist and only compute usedMb/maxMb when present (otherwise set safe
fallback values like 'N/A' or skip updating those DOM fields), and ensure DOM
updates for memory happen after this validation so the page won't be left in a
partially-updated state if memory is missing; update the logic around the
usedMb/maxMb calculations in health.js to check data.memory first and handle the
absent-memory case gracefully.
---
Duplicate comments:
In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Around line 58-59: Rename the incorrectly named responseTimeMs to
responseTimeUs in HealthCheckPlugin (change the variable responseTimeMs computed
with (System.nanoTime() - req.creationTimeNanos()) / 1_000 to responseTimeUs)
and update the JSON payload key (replace "responseTimeMs" with "responseTimeUs")
and all references; then update the frontend in health.js to use
data.responseTimeUs instead of data.responseTimeMs (and any UI labels/fields
that reference the old name).
---
Nitpick comments:
In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Line 34: Replace the plain System.out.println in HealthCheckPlugin with a
proper logger: add a class-level logger (e.g., org.slf4j.Logger or
java.util.logging.Logger) in HealthCheckPlugin and change the
System.out.println("Error loading build.properties: " + e.getMessage()) to a
logger.error (or logger.log with SEVERE) call that includes both a descriptive
message and the exception (pass the exception object for stacktrace); also add
the necessary import and initialize the logger (e.g., getLogger for
HealthCheckPlugin).
- Around line 60-65: In HealthCheckPlugin, avoid calling ZonedDateTime.now()
twice; capture a single instant (e.g., Instant.now() or a single ZonedDateTime
variable) and then derive both local and UTC formatted strings from that same
instant using TIME_FORMAT so localTime and utcTime reflect the exact same
instant; update the code that sets localTime and utcTime to use that single
captured timestamp and then convert/format it for ZoneId.systemDefault() and
ZoneId.of("UTC").
In `@src/main/resources/static/js/health.js`:
- Around line 22-24: When loadHealth() catches an error, it only sets the
"health-status" to "DOWN" and leaves other DOM fields showing stale data; update
the catch block in loadHealth() to also reset/clear the other elements
(document.getElementById("local-time"), "utc-time", "health-version",
"health-commit", "health-response", "health-memory") to a safe fallback value
(e.g., "-" or empty string) so no stale success data remains visible when an
error occurs.
|
@bamsemats I wonder if the endpoint for receiving information about the server could be called something like /metrics instead and we can use /health to only report a simple OK or if we want it in json format {"status":"OK"} or {"status":"UP"} Would like a very simple health endpoint to call when deploying so we can have automatic checks for if the server is down and needs to be restarted. |
kristinaxm
previously approved these changes
Feb 22, 2026
kristinaxm
self-requested a review
February 22, 2026 16:36
bamsemats
changed the title
Author
We will adjust this and put only the Status: UP into the HealthCheckPlugin, whilst implementing a new plugin called MetricPlugin with the rest of the stuff. How should we think regarding the automatic checks for it the server is down? |
…related routes, tests, and UI for enhanced monitoring capabilities.
…related routes, tests, and UI for enhanced monitoring capabilities.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/test/java/org/juv25d/plugin/MetricPluginTest.java (1)
18-18: Test uses/healthpath but MetricPlugin serves/metric.The
HttpRequestis constructed with path/health, butMetricPluginis registered at/metricinApp.java. While the plugin ignores the path in itshandle()method, using the correct path improves test clarity and documentation.♻️ Fix the path to match the actual endpoint
- HttpRequest req = new HttpRequest("GET", "/health", null, "HTTP/1.1", Map.of(), new byte[0], "HEALTH"); + HttpRequest req = new HttpRequest("GET", "/metric", null, "HTTP/1.1", Map.of(), new byte[0], "METRIC");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/plugin/MetricPluginTest.java` at line 18, Update the HttpRequest created in the MetricPluginTest by changing the request path from "/health" to "/metric" so the test request matches the registered endpoint for MetricPlugin; locate the HttpRequest instantiation (the new HttpRequest("GET", "/health", ...)) in MetricPluginTest and replace the path argument with "/metric" to improve clarity and correctness relative to MetricPlugin and the App registration.src/main/java/org/juv25d/plugin/MetricPlugin.java (2)
29-31: Consider using the project's logger instead ofSystem.out.The codebase uses
ServerLogging.getLogger()for logging (as seen inApp.java). UsingSystem.out.printlnbypasses the logging framework, making it harder to manage log levels and output destinations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/MetricPlugin.java` around lines 29 - 31, Replace the System.out.println call in MetricPlugin's IOException catch (the block handling loading of build.properties) with the project's logger: obtain the logger via ServerLogging.getLogger() and log the error (e.g., logger.error or logger.warn) including the exception or its message; update imports if needed and ensure the log message gives the same context ("Error loading build.properties") while passing the exception for full stacktrace.
41-46: Incomplete JSON escaping may produce invalid output.The
escapeJsonmethod only escapes\and", but JSON requires escaping control characters (newlines, tabs, etc.). Ifbuild.propertiesvalues contain such characters, the output will be malformed JSON.Consider handling at minimum:
\n,\r,\t, and other control characters (U+0000–U+001F).♻️ More robust escapeJson implementation
private String escapeJson(String value) { if (value == null) { return ""; } - return value.replace("\\", "\\\\").replace("\"", "\\\""); + StringBuilder sb = new StringBuilder(); + for (char c : value.toCharArray()) { + switch (c) { + case '\\' -> sb.append("\\\\"); + case '"' -> sb.append("\\\""); + case '\n' -> sb.append("\\n"); + case '\r' -> sb.append("\\r"); + case '\t' -> sb.append("\\t"); + default -> { + if (c < 0x20) { + sb.append(String.format("\\u%04x", (int) c)); + } else { + sb.append(c); + } + } + } + } + return sb.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/MetricPlugin.java` around lines 41 - 46, The escapeJson method in MetricPlugin only handles backslashes and double quotes, which can produce invalid JSON when values contain control characters; update the escapeJson(String value) implementation to iterate characters and replace backspace (\b), formfeed (\f), newline (\n), carriage return (\r), tab (\t), backslash and quote with their JSON escapes and convert any control characters U+0000–U+001F to "\\uXXXX" escapes (or alternatively delegate to a JSON library like Jackson's ObjectMapper.writeValueAsString to produce a safe JSON string); locate the escapeJson method in MetricPlugin and implement the character-by-character escaping or call a proven library function to ensure all required escapes are covered.src/main/resources/static/js/metric.js (2)
26-28: Consider clearing stale data on error.When the fetch fails, only
health-statusis updated to "DOWN" while other fields retain their previous values. This could confuse users who see an old timestamp paired with a DOWN status.♻️ Clear fields on error
} catch (e) { document.getElementById("health-status").textContent = "DOWN"; + document.getElementById("local-time").textContent = "—"; + document.getElementById("utc-time").textContent = "—"; + document.getElementById("health-version").textContent = "—"; + document.getElementById("health-commit").textContent = "—"; + document.getElementById("health-response").textContent = "—"; + document.getElementById("health-memory").textContent = "—"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/metric.js` around lines 26 - 28, In the catch block in metric.js where you set document.getElementById("health-status").textContent = "DOWN", also clear other UI fields to avoid showing stale data: locate the catch block and set document.getElementById("version").textContent and document.getElementById("last-checked").textContent (and any metric elements rendered by this file) to a neutral placeholder like "-" or "N/A", and remove or update any status-related CSS classes on the "health-status" element so the UI reflects the error state consistently.
13-14: Unit label "µs" contradicts field nameresponseTimeMs.The display uses "µs" (microseconds), which actually matches what the backend calculates (nanoseconds / 1000), but contradicts the JSON field name
responseTimeMs. Once the backend naming is corrected (see MetricPlugin.java review), update this accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/metric.js` around lines 13 - 14, The UI currently appends " µs" while reading data.responseTimeMs; change the client to use the corrected backend field name and matching unit — replace the assignment that sets document.getElementById("health-response").textContent to use data.responseTimeUs + " µs" (or if the backend instead becomes milliseconds, use data.responseTimeMs + " ms"); ensure the symbol referenced is document.getElementById("health-response") and the response field is data.responseTimeUs (or data.responseTimeMs) so the label matches the backend rename.
🤖 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/org/juv25d/App.java`:
- Around line 54-55: HealthCheckPlugin contains an invalid JSON response with a
trailing comma; open the HealthCheckPlugin class
(org.juv25d.plugin.HealthCheckPlugin) and fix the JSON literal returned (in the
method that builds/returns the health JSON) by removing the extra comma so it
becomes {"status":"UP"}; ensure any string construction or serializer produces
valid JSON without dangling commas.
In `@src/main/java/org/juv25d/plugin/MetricPlugin.java`:
- Around line 54-55: The variable responseTimeMs is computed as
(System.nanoTime() - req.creationTimeNanos()) / 1_000 which yields microseconds,
so change the code to consistently use microseconds: rename the variable
responseTimeMs to responseTimeUs, update the JSON field name from
"responseTimeMs" to "responseTimeUs", and update any formatting/serialization
call that references responseTimeMs (e.g., the format argument used when
building the JSON) so all references (variable, JSON key, and any format/field
name passed to the serializer) use responseTimeUs; alternatively, if you really
want milliseconds, divide by 1_000_000 instead—pick one approach and make the
variable name, division, and JSON field consistent (preferred: rename to
responseTimeUs to match the frontend's µs display).
---
Nitpick comments:
In `@src/main/java/org/juv25d/plugin/MetricPlugin.java`:
- Around line 29-31: Replace the System.out.println call in MetricPlugin's
IOException catch (the block handling loading of build.properties) with the
project's logger: obtain the logger via ServerLogging.getLogger() and log the
error (e.g., logger.error or logger.warn) including the exception or its
message; update imports if needed and ensure the log message gives the same
context ("Error loading build.properties") while passing the exception for full
stacktrace.
- Around line 41-46: The escapeJson method in MetricPlugin only handles
backslashes and double quotes, which can produce invalid JSON when values
contain control characters; update the escapeJson(String value) implementation
to iterate characters and replace backspace (\b), formfeed (\f), newline (\n),
carriage return (\r), tab (\t), backslash and quote with their JSON escapes and
convert any control characters U+0000–U+001F to "\\uXXXX" escapes (or
alternatively delegate to a JSON library like Jackson's
ObjectMapper.writeValueAsString to produce a safe JSON string); locate the
escapeJson method in MetricPlugin and implement the character-by-character
escaping or call a proven library function to ensure all required escapes are
covered.
In `@src/main/resources/static/js/metric.js`:
- Around line 26-28: In the catch block in metric.js where you set
document.getElementById("health-status").textContent = "DOWN", also clear other
UI fields to avoid showing stale data: locate the catch block and set
document.getElementById("version").textContent and
document.getElementById("last-checked").textContent (and any metric elements
rendered by this file) to a neutral placeholder like "-" or "N/A", and remove or
update any status-related CSS classes on the "health-status" element so the UI
reflects the error state consistently.
- Around line 13-14: The UI currently appends " µs" while reading
data.responseTimeMs; change the client to use the corrected backend field name
and matching unit — replace the assignment that sets
document.getElementById("health-response").textContent to use
data.responseTimeUs + " µs" (or if the backend instead becomes milliseconds, use
data.responseTimeMs + " ms"); ensure the symbol referenced is
document.getElementById("health-response") and the response field is
data.responseTimeUs (or data.responseTimeMs) so the label matches the backend
rename.
In `@src/test/java/org/juv25d/plugin/MetricPluginTest.java`:
- Line 18: Update the HttpRequest created in the MetricPluginTest by changing
the request path from "/health" to "/metric" so the test request matches the
registered endpoint for MetricPlugin; locate the HttpRequest instantiation (the
new HttpRequest("GET", "/health", ...)) in MetricPluginTest and replace the path
argument with "/metric" to improve clarity and correctness relative to
MetricPlugin and the App registration.
…ckend and frontend
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/org/juv25d/plugin/HealthCheckPlugin.java (1)
20-24: Nit: inconsistent closing brace indentation in the JSON text block.The closing
}is indented with extra spaces compared to the opening{, producing slightly misaligned JSON output. Cosmetic only.♻️ Suggested tweak
String jsonBody = """ { "status": "UP" - } + } """;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java` around lines 20 - 24, The JSON text block assigned to jsonBody in HealthCheckPlugin.java has a misaligned closing brace; fix the multi-line string so the closing `}` lines up with the opening `{` (i.e., remove the extra leading spaces before the closing brace) to produce aligned JSON output from the jsonBody variable.src/main/java/org/juv25d/plugin/MetricPlugin.java (1)
56-61:localTime/utcTimeare interpolated without JSON escaping.These formatted time strings are injected directly into the JSON template via
%s. While standard time zone abbreviations are safe, running on a JVM with an unusual locale/zone configuration could theoretically produce characters that break the JSON. Consider passing them throughescapeJson()for consistency with the other string fields.♻️ Suggested tweak
- String localTime = ZonedDateTime + String localTime = escapeJson(ZonedDateTime .now(ZoneId.systemDefault()) - .format(TIME_FORMAT); - String utcTime = ZonedDateTime + .format(TIME_FORMAT)); + String utcTime = escapeJson(ZonedDateTime .now(ZoneId.of("UTC")) - .format(TIME_FORMAT); + .format(TIME_FORMAT));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/MetricPlugin.java` around lines 56 - 61, Escape the formatted time strings before injecting into the JSON template: replace direct uses of localTime and utcTime in MetricPlugin (where they are built with ZonedDateTime.now(...).format(TIME_FORMAT)) with escaped values by passing them through escapeJson(), e.g. compute escapedLocalTime = escapeJson(localTime) and escapedUtcTime = escapeJson(utcTime) and use those escaped variables in the JSON interpolation to match how other string fields are handled.src/main/resources/static/js/metric.js (1)
16-21: Memory field-level null check is shallow.
data.memorybeing truthy doesn't guaranteeusedBytes/maxBytesare present. If the backend returns"memory": {}, the division producesNaN. Consider guarding individual fields as well.♻️ Suggested improvement
- const usedMb = data.memory - ? (data.memory.usedBytes / 1024 / 1024).toFixed(1) + const usedMb = data.memory?.usedBytes != null + ? (data.memory.usedBytes / 1024 / 1024).toFixed(1) : "N/A"; - const maxMb = data.memory - ? (data.memory.maxBytes / 1024 / 1024).toFixed(1) + const maxMb = data.memory?.maxBytes != null + ? (data.memory.maxBytes / 1024 / 1024).toFixed(1) : "N/A";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/metric.js` around lines 16 - 21, The shallow null check on data.memory can still yield NaN when usedBytes or maxBytes are missing; update the usedMb and maxMb assignments to individually guard and validate data.memory.usedBytes and data.memory.maxBytes (e.g., check existence and Number.isFinite or typeof === "number") before performing the division and toFixed, and fall back to "N/A" if the field is absent/invalid—apply this to the expressions that compute usedMb and maxMb referencing data.memory.usedBytes and data.memory.maxBytes.src/test/java/org/juv25d/plugin/MetricPluginTest.java (1)
18-18: Test request path is/healthbut MetricPlugin serves/metric.Since the plugin is invoked directly (not via the router), this doesn't affect correctness, but updating the path to
"/metric"would make the test self-documenting.♻️ Suggested tweak
- HttpRequest req = new HttpRequest("GET", "/health", null, "HTTP/1.1", Map.of(), new byte[0], "HEALTH"); + HttpRequest req = new HttpRequest("GET", "/metric", null, "HTTP/1.1", Map.of(), new byte[0], "METRIC");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/plugin/MetricPluginTest.java` at line 18, Update the test's HttpRequest to use the plugin's actual path so it's self-documenting: change the request path argument passed to the HttpRequest constructor in MetricPluginTest from "/health" to "/metric" (the HttpRequest instantiation that currently reads HttpRequest req = new HttpRequest("GET", "/health", ...)); no other behavior changes required since MetricPlugin is invoked directly.
🤖 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/resources/static/js/metric.js`:
- Around line 1-29: The loadHealth function's success path never sets the
health-status element back to "UP", so once the catch sets it to "DOWN" it
remains that way; update loadHealth to explicitly set
document.getElementById("health-status").textContent = "UP" (or equivalent
healthy indicator) in the successful branch after data is applied (e.g., near
the end of the try block after updating health-memory) and ensure the catch
still sets it to "DOWN" to restore correct status transitions.
In `@src/test/java/org/juv25d/plugin/MetricPluginTest.java`:
- Line 33: The test assertion in MetricPluginTest is still checking for the old
JSON field name "responseTimeMs" while MetricPlugin now emits "responseTimeUs";
update the assertion in the test (the assertTrue call that checks
body.contains(...)) to look for "\"responseTimeUs\"" and adjust the assertion
message accordingly so it verifies the new field emitted by MetricPlugin.
---
Duplicate comments:
In `@src/main/java/org/juv25d/App.java`:
- Around line 54-55: The route registration for the specific endpoints is
correct; ensure you keep the two calls router.registerPlugin("/metric", new
MetricPlugin()) and router.registerPlugin("/health", new HealthCheckPlugin())
placed before any wildcard registrations (e.g., "/", "/*") so they take
precedence, and verify MetricPlugin and HealthCheckPlugin constructors and
exported types match the router.registerPlugin signature used elsewhere.
---
Nitpick comments:
In `@src/main/java/org/juv25d/plugin/HealthCheckPlugin.java`:
- Around line 20-24: The JSON text block assigned to jsonBody in
HealthCheckPlugin.java has a misaligned closing brace; fix the multi-line string
so the closing `}` lines up with the opening `{` (i.e., remove the extra leading
spaces before the closing brace) to produce aligned JSON output from the
jsonBody variable.
In `@src/main/java/org/juv25d/plugin/MetricPlugin.java`:
- Around line 56-61: Escape the formatted time strings before injecting into the
JSON template: replace direct uses of localTime and utcTime in MetricPlugin
(where they are built with ZonedDateTime.now(...).format(TIME_FORMAT)) with
escaped values by passing them through escapeJson(), e.g. compute
escapedLocalTime = escapeJson(localTime) and escapedUtcTime =
escapeJson(utcTime) and use those escaped variables in the JSON interpolation to
match how other string fields are handled.
In `@src/main/resources/static/js/metric.js`:
- Around line 16-21: The shallow null check on data.memory can still yield NaN
when usedBytes or maxBytes are missing; update the usedMb and maxMb assignments
to individually guard and validate data.memory.usedBytes and
data.memory.maxBytes (e.g., check existence and Number.isFinite or typeof ===
"number") before performing the division and toFixed, and fall back to "N/A" if
the field is absent/invalid—apply this to the expressions that compute usedMb
and maxMb referencing data.memory.usedBytes and data.memory.maxBytes.
In `@src/test/java/org/juv25d/plugin/MetricPluginTest.java`:
- Line 18: Update the test's HttpRequest to use the plugin's actual path so it's
self-documenting: change the request path argument passed to the HttpRequest
constructor in MetricPluginTest from "/health" to "/metric" (the HttpRequest
instantiation that currently reads HttpRequest req = new HttpRequest("GET",
"/health", ...)); no other behavior changes required since MetricPlugin is
invoked directly.
…t, and frontend status enhancements
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/org/juv25d/plugin/MetricPlugin.java (3)
45-50:escapeJsondoes not escape control characters — could produce invalid JSON.The method only handles
\and". If abuild.propertiesvalue contains a literal newline, tab, carriage return, or other control character (U+0000–U+001F), the JSON output will be syntactically invalid. A minimal fix:♻️ Proposed refactor
private String escapeJson(String value) { if (value == null) { return ""; } - return value.replace("\\", "\\\\").replace("\"", "\\\""); + return value.replace("\\", "\\\\") + .replace("\"", "\\\"") + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("\t", "\\t") + .replace("\b", "\\b") + .replace("\f", "\\f"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/MetricPlugin.java` around lines 45 - 50, The escapeJson method in MetricPlugin only handles backslash and double-quote and must also escape JSON control characters (U+0000–U+001F) to produce valid JSON; update the escapeJson(String value) implementation to iterate characters and replace control chars with their JSON escapes (\b, \f, \n, \r, \t) and use \\uXXXX escapes for any other codepoints <= 0x1F, while still escaping backslash and double-quote, so that any build.properties value containing newlines, tabs, nulls, etc. is safely serialized.
67-89:localTimeandutcTimebypassescapeJson; consider a JSON builder for robustness.There are two related issues:
localTimeandutcTimeare injected via%swithout sanitization. While theDateTimeFormatteroutput is safe today, if the system's timezone abbreviation changes to contain\or", the JSON silently breaks.- The hand-rolled
String.formattemplate is a fragile baseline — each new string field requires a manual decision to escape, and it's easy to miss (as happened here with the time fields).The minimal fix is to pass all string fields through
escapeJson:♻️ Proposed minimal fix
String jsonBody = String.format(""" { "localTime": "%s", "utcTime": "%s", "server": "%s", "buildVersion": "%s", "gitCommit": "%s", "responseTimeUs": %d, "memory": { "usedBytes": %d, "maxBytes": %d } } """, - localTime, - utcTime, - SERVER_NAME, + escapeJson(localTime), + escapeJson(utcTime), + escapeJson(SERVER_NAME), version, commit,As a longer-term improvement, consider replacing the manual template with a lightweight JSON construction approach (e.g.,
jakarta.json,org.json, or even a simpleJsonObjectbuilder from a library already in the project) to avoid the class of "forgot to escape" bugs entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/MetricPlugin.java` around lines 67 - 89, In MetricPlugin where the JSON body is built with String.format, the time fields localTime and utcTime (and any other string fields such as SERVER_NAME, version, commit) are inserted without escaping; update the String.format invocation to wrap every string argument with escapeJson (e.g., escapeJson(localTime), escapeJson(utcTime), escapeJson(SERVER_NAME), escapeJson(version), escapeJson(commit)) so all string fields are escaped before being placed into the JSON template; for a longer-term fix consider replacing this hand-rolled template in MetricPlugin with a proper JSON builder (jakarta.json/org.json/JsonObject) to avoid future missed escapes.
60-65: Two separateZonedDateTime.now()calls capture different instants.
localTimeandutcTimeare snapshotted from two distinct clock reads, so they can theoretically reflect different timestamps under load. Capture once and convert:♻️ Proposed refactor
- String localTime = ZonedDateTime - .now(ZoneId.systemDefault()) - .format(TIME_FORMAT); - String utcTime = ZonedDateTime - .now(ZoneId.of("UTC")) - .format(TIME_FORMAT); + ZonedDateTime nowUtc = ZonedDateTime.now(ZoneId.of("UTC")); + String utcTime = nowUtc.format(TIME_FORMAT); + String localTime = nowUtc.withZoneSameInstant(ZoneId.systemDefault()).format(TIME_FORMAT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/plugin/MetricPlugin.java` around lines 60 - 65, In MetricPlugin replace the two separate ZonedDateTime.now(...) calls with a single snapshot: get one ZonedDateTime (e.g., ZonedDateTime now = ZonedDateTime.now(ZoneId.systemDefault());), format localTime from that snapshot using TIME_FORMAT, and derive utcTime by converting the same instant with now.withZoneSameInstant(ZoneId.of("UTC")).format(TIME_FORMAT) so both strings reflect the exact same instant.
🤖 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/org/juv25d/plugin/MetricPlugin.java`:
- Around line 58-59: The variable naming/unit is already corrected: ensure the
computed value in MetricPlugin (variable responseTimeUs in the code that does
(System.nanoTime() - req.creationTimeNanos()) / 1_000) and the JSON field
"responseTimeUs" remain consistent; no code changes required—just keep
responseTimeUs as the microsecond value and the JSON key "responseTimeUs".
---
Nitpick comments:
In `@src/main/java/org/juv25d/plugin/MetricPlugin.java`:
- Around line 45-50: The escapeJson method in MetricPlugin only handles
backslash and double-quote and must also escape JSON control characters
(U+0000–U+001F) to produce valid JSON; update the escapeJson(String value)
implementation to iterate characters and replace control chars with their JSON
escapes (\b, \f, \n, \r, \t) and use \\uXXXX escapes for any other codepoints <=
0x1F, while still escaping backslash and double-quote, so that any
build.properties value containing newlines, tabs, nulls, etc. is safely
serialized.
- Around line 67-89: In MetricPlugin where the JSON body is built with
String.format, the time fields localTime and utcTime (and any other string
fields such as SERVER_NAME, version, commit) are inserted without escaping;
update the String.format invocation to wrap every string argument with
escapeJson (e.g., escapeJson(localTime), escapeJson(utcTime),
escapeJson(SERVER_NAME), escapeJson(version), escapeJson(commit)) so all string
fields are escaped before being placed into the JSON template; for a longer-term
fix consider replacing this hand-rolled template in MetricPlugin with a proper
JSON builder (jakarta.json/org.json/JsonObject) to avoid future missed escapes.
- Around line 60-65: In MetricPlugin replace the two separate
ZonedDateTime.now(...) calls with a single snapshot: get one ZonedDateTime
(e.g., ZonedDateTime now = ZonedDateTime.now(ZoneId.systemDefault());), format
localTime from that snapshot using TIME_FORMAT, and derive utcTime by converting
the same instant with
now.withZoneSameInstant(ZoneId.of("UTC")).format(TIME_FORMAT) so both strings
reflect the exact same instant.
kristinaxm
previously approved these changes
Feb 22, 2026
fmazmz
approved these changes
Feb 22, 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
5 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.
Closes #104
Closes #115
This PR introduces the HealthCheckPlugin and MetricPlugin, providing a standardized JSON health check endpoint at /health - this is the first functional plugin added to our server's architecture, demonstrating how to handle dynamic JSON responses and route-specific plugin registration.
Additionally, another plugin (MetricPlugin) is implemented, which checks and returns data on:
Current timestamp (with timezone)
Application version
Git commit hash
Server uptime / response time
JVM memory usage (used / max)
Summary by CodeRabbit
New Features
Chores
Tests