feat(logging): replace SLF4J logger with LogManager for consistent lo…#2502
feat(logging): replace SLF4J logger with LogManager for consistent lo…#2502
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @robfrank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refactors the logging infrastructure within the ArcadeDB gRPC service. The primary goal is to achieve logging consistency and maintainability by migrating from SLF4J to ArcadeDB's native LogManager. This change ensures that all log messages, including debug, info, warn, and error, adhere to a single, standardized format and mechanism, improving the clarity and manageability of system diagnostics.
Highlights
- Logging System Migration: Replaced all instances of SLF4J's
Loggerwith ArcadeDB's internalLogManageracross theArcadeDbGrpcServiceand related gRPC components to centralize and standardize logging practices. - Consistent Log Formatting: Updated all log statements to use
LogManager.instance().log(...)with JavaLevelconstants (e.g.,Level.FINE,Level.SEVERE) and adopted%splaceholders for message formatting, ensuring uniformity. - Simplified Debug Logging: Removed conditional checks like
logger.isDebugEnabled()for debug logs, replacing them with unconditional calls toLogManagerat the debug level, streamlining the logging code. - Standardized Error Reporting: Refactored error logging within
catchblocks to utilizeLogManagerinstead oflogger.error, contributing to a more consistent error reporting mechanism throughout the service.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the logging mechanism from SLF4J to ArcadeDB's LogManager, ensuring consistency across the codebase. The changes are well-executed and cover all relevant files. I have identified a couple of areas for improvement. Firstly, numerous debug logging statements are wrapped in if (true) blocks, which are redundant and should be removed to enhance code readability. Secondly, in GrpcServerPlugin, the switch statement for server mode selection should throw an exception for invalid modes to ensure a fail-fast behavior, rather than just logging an error. Overall, this is a solid refactoring effort.
| default: | ||
| logger.error("Invalid gRPC mode: {}. Use 'standard', 'xds', or 'both'", mode); | ||
| } | ||
| default -> LogManager.instance().log(this, Level.SEVERE, "Invalid gRPC mode: %s. Use 'standard', 'xds', or 'both'", mode); |
There was a problem hiding this comment.
For an invalid gRPC mode, it's better to throw an exception to fail fast rather than just logging an error. This prevents the server from starting in a misconfigured state, which could lead to unexpected behavior.
| default -> LogManager.instance().log(this, Level.SEVERE, "Invalid gRPC mode: %s. Use 'standard', 'xds', or 'both'", mode); | |
| default -> throw new IllegalArgumentException("Invalid gRPC mode: " + mode + ". Use 'standard', 'xds', or 'both'"); |
grpcw/src/main/java/com/arcadedb/server/grpc/ArcadeDbGrpcService.java
Outdated
Show resolved
Hide resolved
| if (true) { | ||
| LogManager.instance().log(this, Level.FINE, "beginTransaction(): resolved database instance dbName=%s class=%s hash=%s", | ||
| (database != null ? database.getName() : "<null>"), (database != null ? database.getClass().getSimpleName() : "<null>"), | ||
| (database != null ? System.identityHashCode(database) : 0)); | ||
| logger.debug("beginTransaction(): calling database.begin()"); | ||
| LogManager.instance().log(this, Level.FINE, "beginTransaction(): calling database.begin()"); | ||
| } |
There was a problem hiding this comment.
This if (true) block is unnecessary. The logging calls can be placed directly in the try block, which improves readability by removing the redundant conditional.
LogManager.instance().log(this, Level.FINE, "beginTransaction(): resolved database instance dbName=%s class=%s hash=%s",
(database != null ? database.getName() : "<null>"), (database != null ? database.getClass().getSimpleName() : "<null>"),
(database != null ? System.identityHashCode(database) : 0));
LogManager.instance().log(this, Level.FINE, "beginTransaction(): calling database.begin()");
grpcw/src/main/java/com/arcadedb/server/grpc/ArcadeDbGrpcService.java
Outdated
Show resolved
Hide resolved
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
…conditional checks
This pull request refactors logging throughout the
ArcadeDbGrpcServiceclass to replace usage of SLF4J'sLoggerwith ArcadeDB's ownLogManager. All log statements have been updated to useLogManager.instance().log(...)with appropriate log levels, and message formatting has been adapted to use%splaceholders. Additionally, some conditional logging checks (logger.isDebugEnabled()) have been replaced with unconditional logging.Logging Refactor
Loggerimports and usage with ArcadeDB'sLogManagerfor logging across the service, including error, debug, and info messages. [1] [2]close,executeCommand,updateRecord,executeQuery,beginTransaction, andcommitTransactionto useLogManager.instance().log(...)with JavaLevelconstants (e.g.,Level.FINE,Level.SEVERE) and%sformatting. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18]logger.isDebugEnabled()), replacing them with unconditional calls toLogManagerfor debug-level logs. [1] [2] [3] [4]Error Handling
catchblocks to useLogManagerinstead oflogger.error, ensuring consistent error reporting throughout the service. [1] [2] [3] [4] [5] [6]These changes standardize logging within the gRPC service to use ArcadeDB's preferred logging mechanism, improving consistency and maintainability.