feature/ServerLogging - #22
Conversation
…class in SocketServer.java to return logging information upon opening socket and user connecting to server.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a centralized java.util.logging configuration class (ServerLogging) and integrates its Logger into SocketServer, replacing System.out and printStackTrace calls with logger calls for server startup, client connections, incoming request method/path, and error logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as SocketServer
participant Logger as ServerLogging
Client->>Server: TCP connect / send HTTP request
Server->>Logger: log(INFO, "Server started")
Server->>Logger: log(INFO, "Client connected: <addr>:<port>")
Server->>Logger: log(INFO, "Request: <METHOD> <PATH>")
Server->>Client: respond (HTTP)
alt on exception
Server->>Logger: log(WARNING/SEVERE, "Error handling request", exception)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/logging/ServerLogging.java`:
- Around line 11-25: The logger configuration in ServerLogging never runs
because it's in an unused constructor; move the ConsoleHandler/SimpleFormatter,
setUseParentHandlers(false), and setLevel(Level.INFO) logic into a static
initializer that configures the static Logger logger used by getLogger(), remove
the unused HttpParser parameter and the public constructor, and add a private
no-op constructor to make ServerLogging a true utility class; ensure
SocketServer continues to call ServerLogging.getLogger() and that the static
block references Logger.getLogger(SocketServer.class.getName()) (the existing
logger field) so configuration is applied at class load time.
🧹 Nitpick comments (5)
src/main/java/org/juv25d/logging/ServerLogging.java (1)
3-4: Circular dependency and tight coupling toSocketServer.
ServerLoggingimportsSocketServerjust to use its class name for the logger, whileSocketServerimportsServerLogging— creating a circular dependency between packages. TheHttpParserimport is also unnecessary once the constructor is removed.Consider accepting the logger name as a parameter or using the
ServerLoggingclass name itself:♻️ Proposed refactor
-import org.juv25d.SocketServer; -import org.juv25d.parser.HttpParser; // ... - private static final Logger logger = Logger.getLogger(SocketServer.class.getName()); + private static final Logger logger = Logger.getLogger("org.juv25d");Or provide a factory method so each caller gets a properly-named logger:
public static Logger getLogger(Class<?> clazz) { return Logger.getLogger(clazz.getName()); }Also applies to: 12-12
src/main/java/org/juv25d/SocketServer.java (4)
28-28: Inconsistent logging style: mix of string concatenation and parameterized messages.Lines 32 and 38 correctly use
logger.log(Level.INFO, "... {0}", arg)(parameterized), but lines 28, 49, 50, and 53 use string concatenation ("..." + value). Parameterized logging avoids the cost of string concatenation when the log level is disabled and is the idiomatic JUL pattern.♻️ Suggested: use parameterized logging consistently
- logger.info("Server started at port: " + serverSocket.getLocalPort()); + logger.log(Level.INFO, "Server started at port: {0}", serverSocket.getLocalPort());- logger.info("Method: " + request.method()); - logger.info("Path: " + request.path()); + logger.log(Level.INFO, "Request: {0} {1}", new Object[]{request.method(), request.path()});- logger.warning("Error handling client: " + e.getMessage()); + logger.log(Level.WARNING, "Error handling client: {0}", e.getMessage());Also applies to: 49-50, 53-53
37-39: Consider logging the full exception forSEVEREerrors.On a severe server socket error, only the message string is logged. The stack trace is lost, making debugging harder. Pass the exception as a parameter to preserve it:
♻️ Proposed fix
- logger.log(Level.SEVERE, "Server socket error: {0}", e.getMessage()); + logger.log(Level.SEVERE, "Server socket error", e);
52-54: Same here — preserve the exception stack trace for client errors.
logger.warning(msg)discards the stack trace. Uselogger.log(Level.WARNING, msg, e)to retain it for diagnostics.♻️ Proposed fix
- logger.warning("Error handling client: " + e.getMessage()); + logger.log(Level.WARNING, "Error handling client", e);
21-21: Logger field placement: move above the constructor for conventional ordering.Static final fields are conventionally declared at the top of the class, before instance fields and constructors. Currently
loggeris declared after the constructor.♻️ Suggested reorder
public class SocketServer { + private static final Logger logger = ServerLogging.getLogger(); private final HttpParser httpParser; public SocketServer(HttpParser httpParser) { this.httpParser = httpParser; } - private static final Logger logger = ServerLogging.getLogger(); - static void createSocket() {
…n empty utility class to prevent instantiation.
fmazmz
left a comment
fmazmz
left a comment
There was a problem hiding this comment.
Nice!
Only one suggestion regarding logging level.
Instead of hard coding Level.INFO, should we maybe read it form System properties so that we can add debug logs and switch the logging level easily in JVM args when running the program.
Example:
java -Dlog.level=FINE org.juv25d.Main || java -Dlog.level=ALL org.juv25d.Main
Something like this?:
String levelName = System.getProperty(
"log.level",
System.getenv().getOrDefault("LOG_LEVEL", "INFO")
);
Level level = Level.parse(levelName.toUpperCase());More info:
https://docs.oracle.com/javase/8/docs/api/java/util/logging/Level.html
…tiated and allow for log level to be set by args in JVM (default level 'INFO' if no args provided).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/SocketServer.java`:
- Around line 36-38: The catch blocks in SocketServer that currently call
logger.severe(e.getMessage()) lose stack traces; import java.util.logging.Level
and replace those calls with logger.log(Level.SEVERE, "<descriptive message>",
e) (e.g., in the server socket catch in run/start method and the other catch at
the later catch block referenced) so the full Throwable is preserved in the
logs; update both occurrences (use the exact logger variable and the existing
exception variable name `e`) and keep descriptive messages like "Server socket
error" or the corresponding context.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/SocketServer.java (2)
20-20: Nitpick: Declarestatic finalfield before the constructor.Java convention is to place static fields at the top of the class, before instance fields and constructors.
♻️ Suggested reordering
public class SocketServer { + private static final Logger logger = ServerLogging.getLogger(); + private final HttpParser httpParser; public SocketServer(HttpParser httpParser) { this.httpParser = httpParser; } - private static final Logger logger = ServerLogging.getLogger(); -
48-49: Consider combining method and path into a single log line.Two separate
logger.infocalls for one request add noise and make log correlation harder. A single entry is easier to grep and reason about.♻️ Proposed change
- logger.info("Method: " + request.method()); - logger.info("Path: " + request.path()); + logger.info("Request: " + request.method() + " " + request.path());
6 participants
Closes #19
This PR introduces basic logging to the HTTP server.
Changes
Scope
Logging is currently applied to "SocketServer", which is the entry point for server and connection handling.
Notes
java.util.logging)Verification
Server startup is logged
Client connections are logged
HTTP method and path are logged on incoming requests
No unit tests added as logging contains no business logic
Summary by CodeRabbit
New Features
Bug Fixes