Conversation
WalkthroughThis update introduces a comprehensive logging framework to the codebase. New aspect-oriented components are added to log HTTP requests and method invocations across controllers and service layers. Custom log info records and a formatter are implemented for structured, consistent log messages. A custom Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant HttpRequestLogAspect
participant MethodLogAspect
participant Service
Client->>Controller: HTTP Request
activate HttpRequestLogAspect
HttpRequestLogAspect->>Controller: Log HTTP Request Details
deactivate HttpRequestLogAspect
activate MethodLogAspect
MethodLogAspect->>Controller: Log Method Start
Controller->>Service: Service Call (with @ServiceLog)
MethodLogAspect->>Service: Log Method Start (if annotated)
Service-->>Controller: Return Result
MethodLogAspect->>Service: Log Method End (if annotated)
Controller-->>Client: HTTP Response
MethodLogAspect->>Controller: Log Method End
deactivate MethodLogAspect
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (5)
src/main/java/org/runimo/runimo/common/log/model/HttpRequestLogInfo.java (2)
3-10: Remove unused import.The
StringTokenizerimport on line 7 is not used in the code.-import java.util.StringTokenizer;
30-46: Consider enhancing query parameter parsing for edge cases.The current implementation has some limitations:
- It doesn't handle URL decoding of parameter values
- It doesn't support multiple values for the same parameter name
- It might not handle all edge cases for malformed query strings
Consider using Spring's
UriComponentsBuilderor servlet'srequest.getParameterMap()for more robust parsing.private static Map<String, String> getQueryParamMap(String queryString) { if (queryString == null || queryString.isBlank()) { return Collections.emptyMap(); } - Map<String, String> queryParams = new HashMap<>(); - - String[] paramPairs = queryString.split("&"); - for (String paramPair : paramPairs) { - String[] keyVal = paramPair.split("=", 2); - - String val = keyVal.length < 2 ? "" : keyVal[1]; - queryParams.put(keyVal[0], val); - } - - return queryParams; + // Use servlet API to handle all edge cases + Map<String, String> queryParams = new HashMap<>(); + Map<String, String[]> parameterMap = request.getParameterMap(); + + for (Map.Entry<String, String[]> entry : parameterMap.entrySet()) { + String[] values = entry.getValue(); + queryParams.put(entry.getKey(), values.length > 0 ? values[0] : ""); + } + + return queryParams; }Alternatively, if you want to keep the current parsing approach but with URL decoding:
private static Map<String, String> getQueryParamMap(String queryString) { if (queryString == null || queryString.isBlank()) { return Collections.emptyMap(); } Map<String, String> queryParams = new HashMap<>(); String[] paramPairs = queryString.split("&"); for (String paramPair : paramPairs) { String[] keyVal = paramPair.split("=", 2); - String val = keyVal.length < 2 ? "" : keyVal[1]; - queryParams.put(keyVal[0], val); + String key = java.net.URLDecoder.decode(keyVal[0], java.nio.charset.StandardCharsets.UTF_8); + String val = keyVal.length < 2 ? "" : java.net.URLDecoder.decode(keyVal[1], java.nio.charset.StandardCharsets.UTF_8); + queryParams.put(key, val); } return queryParams; }src/main/java/org/runimo/runimo/common/response/Response.java (1)
5-8: LGTM! Consistent use of @tostring across response classes.The addition of
@ToStringimproves logging and debugging by providing better string representations of Response objects. This aligns with similar annotations on child classes likeSuccessResponseandErrorResponse.For child classes, consider using
@ToString(callSuper=true)to include the parent class fields in their string representation and avoid duplication.src/main/java/org/runimo/runimo/common/log/HttpRequestLogAspect.java (1)
32-44: Consider enhancing error handling for more robust loggingThe aspect logs HTTP requests correctly, but could be improved in a few ways:
- Line 35-38: When
ServletRequestAttributesis null, consider including more context about which request is being processed- There's no exception handling around the formatter call, which could potentially fail
@Before("controller()") public void apiRequestLogger() { try { ServletRequestAttributes attributes = ((ServletRequestAttributes) RequestContextHolder.getRequestAttributes()); if (attributes == null) { - log.info("ServletRequestAttributes is null"); + log.warn("ServletRequestAttributes is null, unable to log HTTP request details"); return; } HttpServletRequest request = attributes.getRequest(); HttpRequestLogInfo logInfo = HttpRequestLogInfo.of(request); log.info(logMessageFormatter.toHttpRequestLogMessage(logInfo)); + } catch (Exception e) { + log.warn("Failed to log HTTP request: {}", e.getMessage()); + } }src/main/java/org/runimo/runimo/common/log/LogMessageFormatter.java (1)
55-60: Beware of hugetoString()outputs in thereturnlog.Large collections or binary payloads may explode your log volume and slow the JVM.
Consider truncating, hashing, or whitelisting types before callingString.valueOf()on the return value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/org/runimo/runimo/common/log/HttpRequestLogAspect.java(1 hunks)src/main/java/org/runimo/runimo/common/log/LogMessageFormatter.java(1 hunks)src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java(1 hunks)src/main/java/org/runimo/runimo/common/log/ServiceLog.java(1 hunks)src/main/java/org/runimo/runimo/common/log/model/HttpRequestLogInfo.java(1 hunks)src/main/java/org/runimo/runimo/common/log/model/MethodEndLogInfo.java(1 hunks)src/main/java/org/runimo/runimo/common/log/model/MethodStartLogInfo.java(1 hunks)src/main/java/org/runimo/runimo/common/response/ErrorResponse.java(1 hunks)src/main/java/org/runimo/runimo/common/response/Response.java(1 hunks)src/main/java/org/runimo/runimo/common/response/SuccessResponse.java(1 hunks)src/main/java/org/runimo/runimo/hatch/controller/HatchController.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/main/java/org/runimo/runimo/common/response/Response.java (2)
src/main/java/org/runimo/runimo/common/response/ErrorResponse.java (1)
ToString(6-24)src/main/java/org/runimo/runimo/common/response/SuccessResponse.java (1)
ToString(7-25)
src/main/java/org/runimo/runimo/common/response/SuccessResponse.java (2)
src/main/java/org/runimo/runimo/common/response/ErrorResponse.java (1)
ToString(6-24)src/main/java/org/runimo/runimo/common/response/Response.java (1)
ToString(8-30)
src/main/java/org/runimo/runimo/common/log/HttpRequestLogAspect.java (1)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (1)
Aspect(15-60)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (1)
src/main/java/org/runimo/runimo/common/log/HttpRequestLogAspect.java (1)
Aspect(20-46)
🔇 Additional comments (10)
src/main/java/org/runimo/runimo/common/log/ServiceLog.java (1)
8-15: LGTM! Simple and focused annotation for service method logging.The annotation is well-designed with appropriate retention policy and target elements, allowing it to be applied to both classes and methods. This flexible design supports the logging framework being introduced.
Consider adding configuration attributes in the future if you need more fine-grained control over logging behavior (e.g., excluding sensitive parameters, controlling log levels).
src/main/java/org/runimo/runimo/hatch/controller/HatchController.java (1)
27-27: LGTM! Good application of the ServiceLog annotation.The use of
@ServiceLogon thehatchmethod aligns with the PR objective of enabling method-level logging for the hatch API controller. This will log method execution details including parameters, return values, and execution time.src/main/java/org/runimo/runimo/common/response/SuccessResponse.java (1)
4-4: Adding @tostring enhances logging readability - good additionThe
@ToStringannotation will automatically generate a more informative string representation of this class, which is valuable for logging and debugging. This aligns well with the logging framework being introduced in this PR.Also applies to: 7-7
src/main/java/org/runimo/runimo/common/response/ErrorResponse.java (1)
3-3: Good consistency with @tostring across response classesAdding the
@ToStringannotation toErrorResponsemaintains consistency with other response classes and enhances the logging capabilities. This improves debuggability when these objects appear in logs.Also applies to: 6-6
src/main/java/org/runimo/runimo/common/log/HttpRequestLogAspect.java (2)
1-24: AOP configuration looks good for controller-level HTTP request loggingThe AspectJ configuration correctly targets controller methods and includes all necessary dependencies. The use of Lombok annotations for logging and constructor injection helps keep the code clean.
28-30: Pointcut definition effectively targets all controller methodsThe pointcut expression
execution(* org.runimo.runimo..controller.*Controller.*(..))will capture all methods in any class ending with "Controller" in the controller package hierarchy, which is appropriate for HTTP request logging.src/main/java/org/runimo/runimo/common/log/model/MethodEndLogInfo.java (2)
1-13: Record structure effectively captures method execution detailsThe record definition with
@Builderis clean and appropriate for representing method end log information. The private access level on the builder ensures proper instantiation through the factory method.
25-28: Class name extraction looks goodThe method correctly extracts the simple class name from the fully qualified name. While there might be alternative approaches (like using Class.getSimpleName() if available), the current implementation is straightforward and effective.
src/main/java/org/runimo/runimo/common/log/model/MethodStartLogInfo.java (1)
35-37:parameterNamesmay benullwithout the-parameterscompiler flag.
MethodSignature#getParameterNames()returnsnullunless the class is compiled with-parameters.
Consider falling back to placeholder names or logging a warning to avoidNullPointerExceptionin the loop above.src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (1)
46-51: Verify authentication status before trusting the principal.
authenticationcan exist whileisAuthenticated()isfalse(e.g., anonymous user).
Include this check to prevent misleading “authenticated=true” logs.- if (authentication != null) { + if (authentication != null && authentication.isAuthenticated()) { methodStartLogInfo = MethodStartLogInfo.of(pjp, true, authentication.getName());
src/main/java/org/runimo/runimo/common/log/model/MethodStartLogInfo.java
Show resolved
Hide resolved
| @Around("annotatedClassAndMethod()") | ||
| public Object calledMethodLogger(ProceedingJoinPoint pjp) throws Throwable { | ||
| MethodStartLogInfo methodStartLogInfo = getMethodStartLogInfo(pjp); | ||
| log.info(logMessageFormatter.toMethodStartLogMessage(methodStartLogInfo)); | ||
|
|
||
| long startTime = getCurrentTimeMillis(); | ||
| Object proceedReturn = pjp.proceed(); | ||
| long endTime = getCurrentTimeMillis(); | ||
|
|
||
| MethodEndLogInfo methodEndLogInfo = MethodEndLogInfo.of(pjp, endTime - startTime, | ||
| proceedReturn); | ||
| log.info(logMessageFormatter.toMethodEndLogMessage(methodEndLogInfo)); | ||
|
|
||
| return proceedReturn; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log method end and exceptions – move logging into try / catch / finally.
If the target method throws, the current implementation never emits an end‑log, contradicting the stated goal.
Wrap pjp.proceed() in try / catch (to log the exception) and finally (to guarantee an end record).
@Around("annotatedClassAndMethod()")
public Object calledMethodLogger(ProceedingJoinPoint pjp) throws Throwable {
MethodStartLogInfo startInfo = getMethodStartLogInfo(pjp);
log.info(logMessageFormatter.toMethodStartLogMessage(startInfo));
long startTime = getCurrentTimeMillis();
- Object proceedReturn = pjp.proceed();
- long endTime = getCurrentTimeMillis();
-
- MethodEndLogInfo endInfo = MethodEndLogInfo.of(pjp, endTime - startTime, proceedReturn);
- log.info(logMessageFormatter.toMethodEndLogMessage(endInfo));
- return proceedReturn;
+ try {
+ Object result = pjp.proceed();
+ MethodEndLogInfo endInfo =
+ MethodEndLogInfo.of(pjp, getCurrentTimeMillis() - startTime, result);
+ log.info(logMessageFormatter.toMethodEndLogMessage(endInfo));
+ return result;
+ } catch (Throwable ex) {
+ log.warn("METHOD_EXCEPTION [name={}, elapsed_time={}ms, exception={}]",
+ pjp.getSignature().toShortString(),
+ getCurrentTimeMillis() - startTime,
+ ex.toString());
+ throw ex;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
ekgns33
left a comment
ekgns33
left a comment
There was a problem hiding this comment.
고생하셨습니다! 이제 로그를 편하게 볼 수 있겠군요 ㅎㅎ
- NPE 관련해서 토끼가 두개 올려놨는데 해당 이슈만 resolve하고 머지해주세요!
src/main/java/org/runimo/runimo/common/log/model/HttpRequestLogInfo.java
Show resolved
Hide resolved
|
확인이 늦었네요. -> 내일까지.. 하겠읍니다.. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (1)
27-47: 🛠️ Refactor suggestionRestructure method to log end info in finally block
If an exception is thrown, the method end log won't be recorded, as mentioned in the PR objectives.
Apply this change to ensure method end logs are always recorded:
@Around("annotatedClassAndMethod()") public Object calledMethodLogger(ProceedingJoinPoint pjp) throws Throwable { MethodStartLogInfo methodStartLogInfo = getMethodStartLogInfo(pjp); log.info(logMessageFormatter.toMethodStartLogMessage(methodStartLogInfo)); long startTime = getCurrentTimeMillis(); - long endTime; Object proceedReturn = null; try { proceedReturn = pjp.proceed(); - } catch (Throwable ex) { - log.info(logMessageFormatter.toMethodErrorLogMessage(ex)); - } - endTime = getCurrentTimeMillis(); - - MethodEndLogInfo methodEndLogInfo = MethodEndLogInfo.of(pjp, endTime - startTime, - proceedReturn); - log.info(logMessageFormatter.toMethodEndLogMessage(methodEndLogInfo)); - - return proceedReturn; + return proceedReturn; + } catch (Throwable ex) { + log.error(logMessageFormatter.toMethodErrorLogMessage(ex)); + throw ex; + } finally { + long endTime = getCurrentTimeMillis(); + MethodEndLogInfo methodEndLogInfo = MethodEndLogInfo.of(pjp, endTime - startTime, proceedReturn); + log.info(logMessageFormatter.toMethodEndLogMessage(methodEndLogInfo)); + } }src/main/java/org/runimo/runimo/common/log/LogMessageFormatter.java (1)
102-107: 🛠️ Refactor suggestionConsider removing camelCase to snake_case conversion as noted in previous feedback
As mentioned in the previous review comment, this conversion might not be necessary for logs and could impact performance. The team has already agreed to remove it initially.
Remove the conversion as discussed in the previous feedback:
private StringBuilder convertMapToLogFormatString(Map<String, String> infoMap, StringBuilder sb) { sb.append("["); Iterator<Entry<String, String>> it = infoMap.entrySet().iterator(); while (it.hasNext()) { Entry<String, String> entry = it.next(); - sb.append(convertCamelCaseToSnakeCase(entry.getKey())); + sb.append(entry.getKey()); sb.append("="); sb.append(entry.getValue()); if (it.hasNext()) { sb.append(", "); } } sb.append("]"); return sb; } -private String convertCamelCaseToSnakeCase(String camelCase) { - return camelCase - .replaceAll("([A-Z])(?=[A-Z])", "$1_") - .replaceAll("([a-z])([A-Z])", "$1_$2") - .toLowerCase(); -}
🧹 Nitpick comments (4)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (1)
62-64: Consider using System.nanoTime() for more precise time measurementsFor measuring elapsed times,
System.nanoTime()provides better precision thancurrentTimeMillis()and is specifically designed for measuring time intervals.private long getCurrentTimeMillis() { - return System.currentTimeMillis(); + return System.nanoTime() / 1_000_000; // Convert nanoseconds to milliseconds }src/main/java/org/runimo/runimo/common/log/LogMessageFormatter.java (3)
15-15: Address the TODO comment about refactoring duplicated codeThere's a pattern of duplicated code across your logging methods. Consider creating a helper method to construct log messages.
Create a helper method to reduce code duplication across the formatting methods:
private String formatLogMessage(String prefix, Map<String, String> logFields) { StringBuilder sb = new StringBuilder(); sb.append(prefix).append(" "); convertMapToLogFormatString(logFields, sb); return sb.toString(); }Then use it in all your formatting methods to simplify them.
17-68: Refactor duplicate log formatting pattern across methodsThe HTTP request, method start, and method end log formatting methods all follow the same pattern:
- Create a map of fields
- Build a string with a prefix
- Convert the map to a string
- Return the result
This can be simplified with a helper method to reduce duplication.
Add a helper method and refactor the existing methods to use it:
private String formatLogEntry(String prefix, Map<String, String> fields) { StringBuilder sb = new StringBuilder(); sb.append(prefix).append(" "); convertMapToLogFormatString(fields, sb); return sb.toString(); }Then you can simplify the formatting methods, for example:
public String toHttpRequestLogMessage(HttpRequestLogInfo logInfo) { String queryParamString = convertMapToLogFormatString(logInfo.queryParams(), new StringBuilder()).toString(); Map<String, String> logs = new LinkedHashMap<>(); logs.put("method", logInfo.requestMethod()); logs.put("uri", logInfo.uri()); logs.put("query_params", queryParamString); logs.put("time", getCurrentTime()); return formatLogEntry("HTTP_REQUEST", logs); }
98-100: Consider adding a timestamp format string for consistent outputThe current implementation returns the raw string from
ZonedDateTime.now().toString(). For logging, it's often better to use a consistent, configurable format.Add a DateTimeFormatter for consistent timestamp output:
private String getCurrentTime() { - return ZonedDateTime.now().toString(); + DateTimeFormatter formatter = DateTimeFormatter.ISO_OFFSET_DATE_TIME; + return ZonedDateTime.now().format(formatter); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/runimo/runimo/common/log/LogMessageFormatter.java(1 hunks)src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java(1 hunks)src/main/java/org/runimo/runimo/common/log/model/MethodEndLogInfo.java(1 hunks)src/main/java/org/runimo/runimo/common/log/model/MethodStartLogInfo.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/runimo/runimo/common/log/model/MethodStartLogInfo.java
- src/main/java/org/runimo/runimo/common/log/model/MethodEndLogInfo.java
| try { | ||
| proceedReturn = pjp.proceed(); | ||
| } catch (Throwable ex) { | ||
| log.info(logMessageFormatter.toMethodErrorLogMessage(ex)); | ||
| } |
There was a problem hiding this comment.
Critical issue: Method swallows exceptions without rethrowing them
The current exception handling implementation catches all exceptions, logs them, but does not rethrow them. This causes two serious problems:
- Exceptions are silently suppressed, breaking the normal error propagation
- Methods will return null when they should be throwing exceptions
Apply this change to fix the exception handling:
try {
proceedReturn = pjp.proceed();
} catch (Throwable ex) {
- log.info(logMessageFormatter.toMethodErrorLogMessage(ex));
+ log.error(logMessageFormatter.toMethodErrorLogMessage(ex));
+ throw ex;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| proceedReturn = pjp.proceed(); | |
| } catch (Throwable ex) { | |
| log.info(logMessageFormatter.toMethodErrorLogMessage(ex)); | |
| } | |
| try { | |
| proceedReturn = pjp.proceed(); | |
| } catch (Throwable ex) { | |
| - log.info(logMessageFormatter.toMethodErrorLogMessage(ex)); | |
| + log.error(logMessageFormatter.toMethodErrorLogMessage(ex)); | |
| + throw ex; | |
| } |
| public String toMethodErrorLogMessage(Throwable ex) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| sb.append("METHOD_EXCEPTION "); | ||
| sb.append(ex.getMessage()); | ||
|
|
||
| return sb.toString(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error log format to include more debug information
The current method error log only includes the exception message. For effective debugging, include the exception type and method information as well.
Improve the exception logging by adding more context:
public String toMethodErrorLogMessage(Throwable ex) {
StringBuilder sb = new StringBuilder();
sb.append("METHOD_EXCEPTION ");
- sb.append(ex.getMessage());
+ sb.append("[exception_type=").append(ex.getClass().getName());
+ sb.append(", message=").append(ex.getMessage());
+ sb.append("]");
return sb.toString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String toMethodErrorLogMessage(Throwable ex) { | |
| StringBuilder sb = new StringBuilder(); | |
| sb.append("METHOD_EXCEPTION "); | |
| sb.append(ex.getMessage()); | |
| return sb.toString(); | |
| } | |
| public String toMethodErrorLogMessage(Throwable ex) { | |
| StringBuilder sb = new StringBuilder(); | |
| sb.append("METHOD_EXCEPTION "); | |
| sb.append("[exception_type=").append(ex.getClass().getName()); | |
| sb.append(", message=").append(ex.getMessage()); | |
| sb.append("]"); | |
| return sb.toString(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (2)
35-39:⚠️ Potential issueCritical issue: Method swallows exceptions without rethrowing
The current exception handling implementation catches all exceptions, logs them, but does not rethrow them. This causes two serious problems:
- Exceptions are silently suppressed, breaking the normal error propagation
- Methods will return null when they should be throwing exceptions
Apply this change to fix the exception handling:
try { proceedReturn = pjp.proceed(); } catch (Throwable ex) { log.error(logMessageFormatter.toMethodErrorLogMessage(ex)); + throw ex; }
27-47: 🛠️ Refactor suggestionLog method end and exceptions – improve with try/catch/finally pattern
The current implementation logs method end info even for failed method executions, but the timing logic and structure could be improved.
Apply this refactoring to improve exception handling and logging flow:
@Around("annotatedClassAndMethod()") public Object calledMethodLogger(ProceedingJoinPoint pjp) throws Throwable { MethodStartLogInfo methodStartLogInfo = getMethodStartLogInfo(pjp); log.info(logMessageFormatter.toMethodStartLogMessage(methodStartLogInfo)); long startTime = getCurrentTimeMillis(); - long endTime; Object proceedReturn = null; try { proceedReturn = pjp.proceed(); + return proceedReturn; } catch (Throwable ex) { log.error(logMessageFormatter.toMethodErrorLogMessage(ex)); + throw ex; + } finally { + long elapsedTime = getCurrentTimeMillis() - startTime; + MethodEndLogInfo methodEndLogInfo = MethodEndLogInfo.of(pjp, elapsedTime, proceedReturn); + log.info(logMessageFormatter.toMethodEndLogMessage(methodEndLogInfo)); } - endTime = getCurrentTimeMillis(); - - MethodEndLogInfo methodEndLogInfo = MethodEndLogInfo.of(pjp, endTime - startTime, - proceedReturn); - log.info(logMessageFormatter.toMethodEndLogMessage(methodEndLogInfo)); - - return proceedReturn; }
🧹 Nitpick comments (4)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (4)
38-38: Enhance exception log message with more detailsThe current error log only includes the exception message, which might not provide enough information for debugging complex issues.
Improve error logging to include exception type and stack trace:
- log.error(logMessageFormatter.toMethodErrorLogMessage(ex)); + log.error("METHOD_EXCEPTION in {}: {} - {}", + pjp.getSignature().toShortString(), + ex.getClass().getName(), + ex.getMessage(), + ex); // Passing exception as last param includes stack trace
62-64: Consider more precise time measurementFor method timing, especially for operations that complete very quickly, using
System.nanoTime()provides more precise measurements and is specifically designed for interval timing.private long getCurrentTimeMillis() { - return System.currentTimeMillis(); + return System.nanoTime() / 1_000_000; // Convert nanos to millis }
52-58: Add null safety to authentication logicThe current code checks if authentication is null but not if the authentication name could be null.
Add null safety to the authentication handling:
Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); if (authentication != null) { - methodStartLogInfo = MethodStartLogInfo.of(pjp, true, authentication.getName()); + String username = authentication.getName(); + methodStartLogInfo = MethodStartLogInfo.of(pjp, true, username != null ? username : "anonymous"); } else { methodStartLogInfo = MethodStartLogInfo.of(pjp, false, null); }
15-21: Add Javadoc for the class explaining its purposeAdding a class-level javadoc would help other developers understand the purpose and usage of this aspect.
+/** + * AOP aspect that provides method-level logging for classes or methods annotated with @ServiceLog. + * Logs method invocation starts, ends, execution time, and any exceptions thrown. + * Also captures authentication context when available. + */ @Aspect @Slf4j @RequiredArgsConstructor @Component public class MethodLogAspect {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/runimo/runimo/common/log/MethodLogAspect.java (2)
src/main/java/org/runimo/runimo/common/log/HttpRequestLogAspect.java (1)
Aspect(20-46)src/main/java/org/runimo/runimo/common/log/LogMessageFormatter.java (1)
Component(14-108)
|
로컬에선 pass 되는데 ci 환경에서만 테스트가 모두 yml, sql, entity 쪽 바꾼 게 없는데 뭘까요.. 내일 밤 이후에 다시 찾아보겠습니다 |
|
해당 PR시점의 |
2 participants
[작업 사항]
[전달 사항]
모든 HTTP 요청에 대한 로그가 기록됩니다.
클래스 또는 메서드에
@ServiceLog사용 시 클래스에 포함된 모든 메서드의 요청/종료 로그가 기록됩니다.[유의 사항]
코드 정리도 더 할 수 있을 것 같고 log 패키지 정리도 안되어보여서 신경쓰이네요
우선 기능 feature 부터 올립니다
Summary by CodeRabbit
New Features
Enhancements
Style