Adds a connection id to an incoming request that is included in all log messages from code handling that request - #97
Merged
Merged
Conversation
…and LoggingFilter to use the logger, ensuring connection ID is included in the logs
…n ID is correctly included in the logs
…ad of System.out, reflecting recent changes to LoggingFilter
…ad of System.out, reflecting recent changes to LoggingFilter (#95)
Tyreviel
changed the title
📝 WalkthroughWalkthroughGenerates an 8-character connection ID per request in ConnectionHandler, stores it in a ThreadLocal via LogContext, uses ServerLogFormatter to include the connection ID in log lines, replaces println logging with java.util.logging in LoggingFilter, and clears the LogContext after request processing. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ConnectionHandler
participant LogContext
participant LoggingFilter
participant Logger
participant ServerLogFormatter
Client->>ConnectionHandler: send request
ConnectionHandler->>ConnectionHandler: generate 8-char id
ConnectionHandler->>LogContext: setConnectionId(id)
ConnectionHandler->>LoggingFilter: forward request
LoggingFilter->>Logger: logger.info(method + " " + path)
Logger->>ServerLogFormatter: format(LogRecord)
ServerLogFormatter->>LogContext: getConnectionId()
LogContext-->>ServerLogFormatter: return id
ServerLogFormatter-->>Logger: formatted message (timestamp, level, [id], msg)
Logger-->>LoggingFilter: emit log
LoggingFilter-->>ConnectionHandler: processing done
ConnectionHandler->>LogContext: clear()
ConnectionHandler-->>Client: respond
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/juv25d/logging/ServerLogging.java (1)
8-8:⚠️ Potential issue | 🔴 CriticalRemove unused
SimpleFormatterimport — Spotless CI failure.
SimpleFormatteris no longer referenced after the formatter was replaced withServerLogFormatter, causing the pipeline to fail. Runmvn spotless:applyto clean up.🐛 Proposed fix
-import java.util.logging.SimpleFormatter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/logging/ServerLogging.java` at line 8, Remove the unused import of SimpleFormatter from the top of the ServerLogging class: delete the line importing java.util.logging.SimpleFormatter, verify there are no remaining references to SimpleFormatter in ServerLogging (the class now uses ServerLogFormatter), then run mvn spotless:apply to reformat and ensure the Spotless CI failure is resolved.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/logging/LogContext.java (1)
3-16: Add a private constructor to prevent instantiation of this utility class.All members are static; allowing
new LogContext()is misleading.♻️ Proposed fix
public class LogContext { private static final ThreadLocal<String> connectionId = new ThreadLocal<>(); + + private LogContext() {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/logging/LogContext.java` around lines 3 - 16, This utility class LogContext is instantiable but only exposes static members (connectionId, setConnectionId, getConnectionId, clear); add a private no-arg constructor to prevent creating instances by declaring "private LogContext() { }" in the class so it cannot be instantiated while preserving existing static ThreadLocal connectionId and static methods.src/main/java/org/juv25d/ConnectionHandler.java (1)
28-29: Prefer imports over fully-qualified class names.
java.util.UUIDandorg.juv25d.logging.LogContextare used inline rather than imported at the top of the file.♻️ Proposed cleanup
+import java.util.UUID; +import org.juv25d.logging.LogContext; ... - String connectionId = java.util.UUID.randomUUID().toString().substring(0, 8); - org.juv25d.logging.LogContext.setConnectionId(connectionId); + String connectionId = UUID.randomUUID().toString().substring(0, 8); + LogContext.setConnectionId(connectionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/ConnectionHandler.java` around lines 28 - 29, Replace the inline fully-qualified usages with imports: add import java.util.UUID and import org.juv25d.logging.LogContext at the top of ConnectionHandler.java, then change the lines that create connectionId and set the context to use UUID.randomUUID() and LogContext.setConnectionId(connectionId) (and update any other occurrences of the fully-qualified names in this class) to improve readability and consistency.
🤖 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/logging/ServerLogFormatter.java`:
- Around line 12-22: The formatter ServerLogFormatter.format currently returns
only formatMessage(record) and drops any attached Throwable; update the method
(in ServerLogFormatter.format) to call record.getThrown(), and if non-null
capture its stack trace into a string (e.g., via StringWriter/PrintWriter) and
append that stack-trace text to the returned message (after the formatted
message and a newline) so exceptions logged with logger.log(..., e) are
rendered; keep using formatMessage(record) for the main message and only append
the stack trace when record.getThrown() is present.
In `@src/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java`:
- Line 16: Spotless formatting failed in the test class ConnectionIdLoggingTest;
run the formatter and commit the changes by executing mvn spotless:apply (or
apply your project's Spotless rules in your IDE), then stage and commit the
modified files so the Spotless CI check passes.
- Around line 17-46: The test leaks handlers and can fail silently: in
ConnectionIdLoggingTest ensure you remove the temporary Handler from the Logger
in the finally block (call logger.removeHandler(handler)), explicitly set the
effective log level so the INFO record is emitted (e.g.,
logger.setLevel(Level.INFO) and handler.setLevel(Level.ALL)), and guard the list
access by asserting formattedMessages is not empty before calling
formattedMessages.get(0) (or use an assert on size/contains) so failures produce
a clear assertion rather than IndexOutOfBoundsException; keep references to
logger, handler, formattedMessages, ServerLogFormatter,
LogContext.setConnectionId and LogContext.clear when making these changes.
---
Outside diff comments:
In `@src/main/java/org/juv25d/logging/ServerLogging.java`:
- Line 8: Remove the unused import of SimpleFormatter from the top of the
ServerLogging class: delete the line importing
java.util.logging.SimpleFormatter, verify there are no remaining references to
SimpleFormatter in ServerLogging (the class now uses ServerLogFormatter), then
run mvn spotless:apply to reformat and ensure the Spotless CI failure is
resolved.
---
Nitpick comments:
In `@src/main/java/org/juv25d/ConnectionHandler.java`:
- Around line 28-29: Replace the inline fully-qualified usages with imports: add
import java.util.UUID and import org.juv25d.logging.LogContext at the top of
ConnectionHandler.java, then change the lines that create connectionId and set
the context to use UUID.randomUUID() and
LogContext.setConnectionId(connectionId) (and update any other occurrences of
the fully-qualified names in this class) to improve readability and consistency.
In `@src/main/java/org/juv25d/logging/LogContext.java`:
- Around line 3-16: This utility class LogContext is instantiable but only
exposes static members (connectionId, setConnectionId, getConnectionId, clear);
add a private no-arg constructor to prevent creating instances by declaring
"private LogContext() { }" in the class so it cannot be instantiated while
preserving existing static ThreadLocal connectionId and static methods.
There was a problem hiding this comment.
🤖 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/test/java/org/juv25d/logging/ConnectionIdLoggingTest.java`:
- Around line 17-47: The test leaks a Handler and can silently do nothing or
crash: ensure the test pins the logger level, removes the handler in the finally
block, and guards the list access; specifically, on the Logger instance created
in this test (logger) call logger.setLevel(Level.INFO) so logger.info(...) will
emit, register the anonymous Handler as before but ensure
logger.removeHandler(handler) is called in the finally block after
LogContext.clear(), and replace the unguarded formattedMessages.get(0) access
with a safe assertion (e.g., assertFalse(formattedMessages.isEmpty()) or
assertTrue(formattedMessages.size() > 0) then inspect formattedMessages.get(0));
also add the missing static import for assertFalse/assertTrue in the test
imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/juv25d/logging/ServerLogFormatter.java (1)
16-17:LogContext.getConnectionId()read on the formatter thread — breaks under async logging.Reading the
ThreadLocalhere is correct for synchronous j.u.l. (formatter runs on the logging-caller thread), but if anAsyncHandleror any other off-thread dispatch is introduced later, theThreadLocalwill not carry the connection ID to the formatter thread and the field will silently benull.An alternative is to capture the connection ID into the
LogRecordat the call site (e.g. viaMDC-style parameters or a customLogRecordsubclass), making the formatter purely data-driven and independent of the calling thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/logging/ServerLogFormatter.java` around lines 16 - 17, The formatter currently calls LogContext.getConnectionId() on the formatter thread (in ServerLogFormatter), which breaks when logging is handled off-thread; instead capture the connection id at the logging call site and put it on the LogRecord so the formatter is data-driven: modify call sites to read LogContext.getConnectionId() before logging and attach it to the LogRecord (e.g., via LogRecord.setParameters(...) or by creating and using a small custom LogRecord subclass that carries a getConnectionId() field), then update ServerLogFormatter.format(LogRecord) to read the connection id from record.getParameters() or the custom LogRecord accessor and remove direct calls to LogContext.getConnectionId() inside the formatter.
🤖 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/logging/ServerLogFormatter.java`:
- Line 21: The formatter currently uses ZonedDateTime.now(...) which records the
formatting time; update ServerLogFormatter to use the LogRecord's event instant
instead: replace the ZonedDateTime.now(ZoneId.systemDefault()).format(dtf) call
with ZonedDateTime.ofInstant(record.getInstant(),
ZoneId.systemDefault()).format(dtf) (use record.getInstant() from the LogRecord)
so timestamps reflect the actual event time.
---
Nitpick comments:
In `@src/main/java/org/juv25d/logging/ServerLogFormatter.java`:
- Around line 16-17: The formatter currently calls LogContext.getConnectionId()
on the formatter thread (in ServerLogFormatter), which breaks when logging is
handled off-thread; instead capture the connection id at the logging call site
and put it on the LogRecord so the formatter is data-driven: modify call sites
to read LogContext.getConnectionId() before logging and attach it to the
LogRecord (e.g., via LogRecord.setParameters(...) or by creating and using a
small custom LogRecord subclass that carries a getConnectionId() field), then
update ServerLogFormatter.format(LogRecord) to read the connection id from
record.getParameters() or the custom LogRecord accessor and remove direct calls
to LogContext.getConnectionId() inside the formatter.
annikaholmqvist94
approved these changes
Feb 19, 2026
annikaholmqvist94
left a comment
annikaholmqvist94
left a comment
There was a problem hiding this comment.
Great job! Moving to structured logging makes the logs more readable and easier to debug
SandraNelj
approved these changes
Feb 19, 2026
SandraNelj
left a comment
SandraNelj
left a comment
There was a problem hiding this comment.
This is a great addition. The logging feels more consistent now and the connection ID makes debugging clearer. Good job!
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
3 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 #95
Summary by CodeRabbit
New Features
Refactor
Tests