-
Notifications
You must be signed in to change notification settings - Fork 121
Enhance error logging with exception messages #3744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThe DocumentChangeExecutor Java class was modified to enhance error logging. Three error log statements were updated to include exception messages alongside stack traces when errors occur in worker operations, task accumulation, and change application processes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java (1)
185-185: Redundant exception message concatenation in error logs.SLF4J's
error(String, Throwable)method automatically includes the exception message and stack trace in the log output. Concatenatinge.getMessage()to the log message results in:
- Duplicate exception messages in logs
- Unnecessary string concatenation overhead
- Potential "null" text if
getMessage()returns nullThe standard SLF4J pattern is sufficient:
LOGGER.error("Context message", e);🔎 Proposed fix to follow SLF4J best practices
- LOGGER.error("Unexpected error in document executor worker: " + e.getMessage(), e); + LOGGER.error("Unexpected error in document executor worker", e);- LOGGER.error("Error while accumulating document change task: " + e.getMessage(), e); + LOGGER.error("Error while accumulating document change task", e);- LOGGER.error("Error while applying accumulated document changes: " + e.getMessage(), e); + LOGGER.error("Error while applying accumulated document changes", e);Also applies to: 228-228, 252-252
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/main/java/**/*.java: Follow the Style Guide for code formatting and conventions
Use Lombok annotations to reduce boilerplate code and enable annotation processing in IDE
Write JavaDoc for public APIs and include comments for complex logic
Use meaningful, descriptive names for classes and methods following Java naming conventions
Optimize imports before committing; DO NOT optimize imports across the entire project unless specifically working on that task
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
src/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Java 17 for language and follow Java naming conventions
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Analyse
- GitHub Check: build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request modifies error logging statements in the DocumentChangeExecutor class to append exception messages directly to the log text. However, this approach deviates from logging best practices and introduces unnecessary string concatenation overhead.
Key Changes
- Modified three
LOGGER.error()calls to concatenate exception messages using the '+' operator - Changes affect error handling in the document change executor's worker thread, accumulation phase, and flush phase
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
Show resolved
Hide resolved
|
Test Results1 872 files + 936 1 872 suites +936 27m 20s ⏱️ + 10m 15s Results for commit 755d8f0. ± Comparison against base commit 4fdc747. This pull request removes 8 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |


Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.