chore: standardize logger field name to LOGGER#558
Open
ThanuH wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAcross 8 engine classes, static logger fields named ChangesLogger Field Standardization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/internal/RestLogAdmin.java`:
- Line 117: The SSE event name in the RestLogAdmin.java file has been changed
from "log" to "LOGGER" in the sse.newEventBuilder().name() call, which breaks
existing clients that subscribe with addEventListener("log", ...). Revert the
event name parameter in the newEventBuilder().name() method call back to "log"
to maintain backward compatibility with existing log-stream consumers, while you
can keep any logger variable identifier changes separate from the event type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f96e099-87be-4f5c-b581-da2c3c5a6a18
📒 Files selected for processing (8)
src/main/java/ai/labs/eddi/engine/exception/ResourceStoreExceptionMapper.javasrc/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.javasrc/main/java/ai/labs/eddi/engine/internal/RestCoordinatorAdmin.javasrc/main/java/ai/labs/eddi/engine/internal/RestLogAdmin.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.javasrc/main/java/ai/labs/eddi/engine/runtime/rest/interceptors/DocumentDescriptorFilter.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/internal/RestLogAdmin.java`:
- Around line 98-106: Update all LOGGER.debugv log messages at lines 98, 106,
118, and 122 in the RestLogAdmin.java file to include both conversationId and
agentId as context identifiers in the log output. For each log message, add
conversationId and agentId as parameters to the debugv call and update the
format string to include placeholders for these values. Ensure all four log
messages follow a consistent pattern that includes these required context
identifiers for proper correlation during debugging and incident analysis.
- Around line 99-102: The catch block that handles Exception at lines 99-102
silently swallows the exception without logging, which hides cleanup failures
and weakens diagnosability. Replace the empty exception handler by adding a log
statement that captures the error with appropriate context (conversationId,
agentId if available) at a suitable level (DEBUG or WARN). Include an actionable
error message that describes what cleanup operation failed, while keeping the
exception handler in place to prevent the exception from propagating and killing
the pipeline. Ensure the log provides enough detail for debugging listener
removal or other cleanup failures without generating excessive noise during
normal operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f6ac3dc-0068-4b62-b1ca-207abb58423e
📒 Files selected for processing (1)
src/main/java/ai/labs/eddi/engine/internal/RestLogAdmin.java
Comment on lines
+98
to
+106
| LOGGER.debugv("SSE log listener {0} removed (client disconnected or max lifetime reached)", listenerId); | ||
| } catch (Exception e) { | ||
| // CDI container may already be shut down (e.g. during test teardown) — | ||
| // swallow to avoid noisy "ArC container not initialized" stacktraces | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| log.debugv("SSE log stream started (listenerId={0}, agentId={1}, level={2})", listenerId, agentId, level); | ||
| LOGGER.debugv("SSE log stream started (listenerId={0}, agentId={1}, level={2})", listenerId, agentId, level); |
Contributor
There was a problem hiding this comment.
Include conversationId + agentId in log messages for stream/send lifecycle events.
At Line 98, Line 106, Line 118, and Line 122, log messages currently omit one or both required context identifiers, which makes correlation harder during incident/debug analysis.
As per coding guidelines, “Include conversation context (conversationId, agentId) in log messages.”
Suggested fix
- LOGGER.debugv("SSE log listener {0} removed (client disconnected or max lifetime reached)", listenerId);
+ LOGGER.debugv(
+ "SSE log listener {0} removed (agentId={1}, conversationId={2}, client disconnected or max lifetime reached)",
+ listenerId, agentId, conversationId
+ );
- LOGGER.debugv("SSE log stream started (listenerId={0}, agentId={1}, level={2})", listenerId, agentId, level);
+ LOGGER.debugv(
+ "SSE log stream started (listenerId={0}, agentId={1}, conversationId={2}, level={3})",
+ listenerId, agentId, conversationId, level
+ );
- LOGGER.debugv("Failed to send SSE log event: {0}", t.getMessage());
+ LOGGER.debugv(
+ "Failed to send SSE log event (agentId={0}, conversationId={1}): {2}",
+ entry.agentId(), entry.conversationId(), t.getMessage()
+ );
- LOGGER.debugv("Error sending SSE log event: {0}", e.getMessage());
+ LOGGER.debugv(
+ "Error sending SSE log event (agentId={0}, conversationId={1}): {2}",
+ entry.agentId(), entry.conversationId(), e.getMessage()
+ );Also applies to: 118-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/ai/labs/eddi/engine/internal/RestLogAdmin.java` around lines 98
- 106, Update all LOGGER.debugv log messages at lines 98, 106, 118, and 122 in
the RestLogAdmin.java file to include both conversationId and agentId as context
identifiers in the log output. For each log message, add conversationId and
agentId as parameters to the debugv call and update the format string to include
placeholders for these values. Ensure all four log messages follow a consistent
pattern that includes these required context identifiers for proper correlation
during debugging and incident analysis.
Source: Coding guidelines
Comment on lines
99
to
102
| } catch (Exception e) { | ||
| // CDI container may already be shut down (e.g. during test teardown) — | ||
| // swallow to avoid noisy "ArC container not initialized" stacktraces | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t silently swallow cleanup exceptions in the listener teardown path.
Line 99-Line 102 catches Exception and suppresses it entirely, which hides cleanup failures (e.g., failed listener removal) and weakens runtime diagnosability.
As per coding guidelines, “Log errors with conversation context (conversationId, agentId) ... Never let exceptions kill the pipeline — handle gracefully with actionable error messages.”
Suggested fix
- } catch (Exception e) {
- // CDI container may already be shut down (e.g. during test teardown) —
- // swallow to avoid noisy "ArC container not initialized" stacktraces
- }
+ } catch (Exception e) {
+ LOGGER.debugv(
+ e,
+ "Failed to cleanup SSE listener (listenerId={0}, agentId={1}, conversationId={2})",
+ listenerId, agentId, conversationId
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/ai/labs/eddi/engine/internal/RestLogAdmin.java` around lines 99
- 102, The catch block that handles Exception at lines 99-102 silently swallows
the exception without logging, which hides cleanup failures and weakens
diagnosability. Replace the empty exception handler by adding a log statement
that captures the error with appropriate context (conversationId, agentId if
available) at a suitable level (DEBUG or WARN). Include an actionable error
message that describes what cleanup operation failed, while keeping the
exception handler in place to prevent the exception from propagating and killing
the pipeline. Ensure the log provides enough detail for debugging listener
removal or other cleanup failures without generating excessive noise during
normal operation.
Source: Coding guidelines
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| deployFuture.get(30, TimeUnit.SECONDS); | ||
| } catch (TimeoutException e) { | ||
| log.warn("Deployment wait timed out for Agent " + agentId + " v" + version); | ||
| LOGGER.warn("Deployment wait timed out for Agent " + agentId + " v" + version); |
| log.warn("Deployment failed for Agent " + agentId + " v" + version + ": " + (cause != null ? cause.getMessage() : e.getMessage()), | ||
| // LOGGER full details server-side, expose only safe message to client | ||
| LOGGER.warn( | ||
| "Deployment failed for Agent " + agentId + " v" + version + ": " + (cause != null ? cause.getMessage() : e.getMessage()), |
| undeploy(environment, agentId, version); | ||
| log.info(String.format("Successfully undeployed Agent (agentId=%s, agentVersion=%s, environment=%s)", agentId, version, environment)); | ||
| LOGGER.info( | ||
| String.format("Successfully undeployed Agent (agentId=%s, agentVersion=%s, environment=%s)", agentId, version, environment)); |
| private Status throwError(String agentId, Integer version, ServiceException e, String message) { | ||
| message = String.format(message, agentId, version); | ||
| log.error(message, e); | ||
| LOGGER.error(message, e); |
| String message = "Agent deployment is currently in progress! (agentId=%s , version=%s)"; | ||
| message = String.format(message, agentId, version); | ||
| log.error(message, e); | ||
| LOGGER.error(message, e); |
| } | ||
| } catch (Exception e) { | ||
| log.warnf(e, "[SCHEDULE] Failed to auto-disable schedules for Agent %s (non-fatal)", agentId); | ||
| LOGGER.warnf(e, "[SCHEDULE] Failed to auto-disable schedules for Agent %s (non-fatal)", agentId); |
| }); | ||
|
|
||
| log.debugv("SSE log stream started (listenerId={0}, agentId={1}, level={2})", listenerId, agentId, level); | ||
| LOGGER.debugv("SSE log stream started (listenerId={0}, agentId={1}, level={2})", listenerId, agentId, level); |
| }); | ||
|
|
||
| log.debugv("SSE log stream started (listenerId={0}, agentId={1}, level={2})", listenerId, agentId, level); | ||
| LOGGER.debugv("SSE log stream started (listenerId={0}, agentId={1}, level={2})", listenerId, agentId, level); |
| deleteAttachmentsForConversation(conversationId); | ||
| conversationMemoryStore.deleteConversationMemorySnapshot(conversationId); | ||
| log.info(format("Conversation has been permanently deleted (conversationId=%s)", conversationId)); | ||
| LOGGER.info(format("Conversation has been permanently deleted (conversationId=%s)", conversationId)); |
| long deleted = attachmentStorageInstance.get().deleteByConversation(conversationId); | ||
| if (deleted > 0) { | ||
| log.debug(format("Deleted %d attachments for conversation %s", deleted, conversationId)); | ||
| LOGGER.debug(format("Deleted %d attachments for conversation %s", deleted, conversationId)); |
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
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.
Summary
Standardizes the logger naming convention across 8 files in the engine package. Renamed log and LOG fields to LOGGER to ensure consistency with the project's established convention and improve grep-ability
Type of Change
Related Issue
Closes #
Changes Made
private static final Logger log/LOGtoprivate static final Logger LOGGERin 8 files.info,warn,error) to reference the newLOGGERfield.ResourceStoreExceptionMapper.javaHttpClientWrapper.javaRestAgentAdministration.javaRestAgentManagement.javaRestCoordinatorAdmin.javaRestLogAdmin.javaRestConversationStore.javaDocumentDescriptorFilter.javaHow to Test
./mvnw clean compileto ensure the syntax is correct../mvnw test -DskipITsto verify that existing unit tests pass after the refactoring.Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit
LOGGERconvention across REST and HTTP components.