[BE-Feat] 로그 세팅 추가 #97
Conversation
WalkthroughThe changes introduce a new configuration class, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LogFilter
participant JwtAuthFilter
participant Controller
Client->>LogFilter: HTTP Request
Note over LogFilter: Log request details and check URI
LogFilter->>JwtAuthFilter: Forward filtered request
JwtAuthFilter->>Controller: Validate JWT and process request
Controller-->>JwtAuthFilter: Response
JwtAuthFilter-->>LogFilter: Response passed along
LogFilter-->>Client: HTTP Response
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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: 0
🧹 Nitpick comments (4)
backend/src/main/java/endolphin/backend/global/config/FilterConfig.java (1)
13-19: Align URL pattern with LogFilter's path filteringThe URL pattern "/*" matches all requests, but LogFilter's
shouldNotFiltermethod only processes paths starting with "/api/v1". Consider updating the URL pattern to match the filter's behavior.- filterRegistrationBean.addUrlPatterns("/*"); + filterRegistrationBean.addUrlPatterns("/api/v1/*");backend/src/main/java/endolphin/backend/global/logging/LogFilter.java (1)
14-28: Enhance logging details and timing precisionConsider the following improvements:
- Use
System.nanoTime()for more precise timing- Add request headers and query parameters for better debugging
- Include response headers in the log
- long startTime = System.currentTimeMillis(); + long startTime = System.nanoTime(); log.info("Incoming Request: [{}] {} {}", request.getMethod(), request.getRequestURI(), + request.getQueryString() != null ? "?" + request.getQueryString() : ""); filterChain.doFilter(request, response); - long endTime = System.currentTimeMillis(); + long endTime = System.nanoTime(); log.info("Incoming Response: [{}] {}, {}, {} ms", request.getMethod(), request.getRequestURI(), response.getStatus(), - endTime - startTime); + (endTime - startTime) / 1_000_000.0);backend/src/main/resources/logback-spring.xml (2)
4-8: Consider making log path configurable and adding MDC support
- The log path should be configurable through environment variables or application properties
- Consider adding MDC support for request tracking (e.g., correlation ID)
<property name="CONSOLE_LOG_PATTERN" - value="%d{yyyy-MM-dd HH:mm:ss}:%-4relative %highlight(%-5level) %magenta(${PID:-}) [%thread] %cyan(%logger{36}) - %msg%n" /> + value="%d{yyyy-MM-dd HH:mm:ss}:%-4relative %highlight(%-5level) %magenta(${PID:-}) [%thread] %cyan(%logger{36}) [%X{correlationId}] - %msg%n" /> <property name="FILE_LOG_PATTERN" - value="%d{yyyy-MM-dd HH:mm:ss}:%-4relative %-5level ${PID:-} [%thread] %logger{36} - %msg%n" /> + value="%d{yyyy-MM-dd HH:mm:ss}:%-4relative %-5level ${PID:-} [%thread] %logger{36} [%X{correlationId}] - %msg%n" /> - <property name="LOG_PATH" value="/var/logs/myapp"/> + <property name="LOG_PATH" value="${LOG_PATH:-/var/logs/myapp}"/>
26-47: Consider using AsyncAppender for better performanceIn production, consider wrapping the RollingFileAppender with an AsyncAppender to prevent logging from blocking the main thread.
<springProfile name="prod"> + <appender name="ASYNC" class="ch.qos.logback.classic.AsyncAppender"> + <appender-ref ref="FILE" /> + <queueSize>512</queueSize> + <discardingThreshold>0</discardingThreshold> + <includeCallerData>false</includeCallerData> + <neverBlock>true</neverBlock> + </appender> <root level="info"> - <appender-ref ref="FILE" /> + <appender-ref ref="ASYNC" /> </root>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/main/java/endolphin/backend/global/config/FilterConfig.java(1 hunks)backend/src/main/java/endolphin/backend/global/logging/LogFilter.java(1 hunks)backend/src/main/java/endolphin/backend/global/security/SecurityConfig.java(0 hunks)backend/src/main/resources/logback-spring.xml(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/main/java/endolphin/backend/global/security/SecurityConfig.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (3)
backend/src/main/java/endolphin/backend/global/config/FilterConfig.java (1)
22-28: Consider restricting JWT authentication to API endpointsThe JWT authentication filter is currently applied to all requests ("/*"). Consider restricting it to API endpoints if authentication is only needed for API calls.
Could you confirm if JWT authentication should be applied to all endpoints or just API endpoints?
backend/src/main/java/endolphin/backend/global/logging/LogFilter.java (1)
30-33: LGTM!The path filtering logic is clear and correctly implemented.
backend/src/main/resources/logback-spring.xml (1)
10-24: LGTM!The non-prod configuration with console logging and debug levels is appropriate for development environments.
| @Override | ||
| protected boolean shouldNotFilter(HttpServletRequest request) { | ||
| return !request.getRequestURI().startsWith("/api/v1"); | ||
| } |
There was a problem hiding this comment.
p2; Config에 url pattern으로 추가하는게 좋아 보입니다.
Docstrings generation was requested by @kwon204. * #97 (comment) The following files were modified: * `backend/src/main/java/endolphin/backend/global/config/FilterConfig.java` * `backend/src/main/java/endolphin/backend/global/logging/LogFilter.java`
|
Note Generated docstrings for this pull request at #102 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/src/main/java/endolphin/backend/global/logging/LogFilter.java (2)
29-33: Consider improving time measurement precision.The current implementation might lose precision when converting nanoseconds to milliseconds. Consider using TimeUnit for more accurate conversion:
- (endTime - startTime) / 1_000_000.0); + TimeUnit.NANOSECONDS.toMillis(endTime - startTime));Don't forget to add the import:
import java.util.concurrent.TimeUnit;
16-35: Consider adding request body logging for debugging.For debugging purposes, it might be helpful to log request bodies. Consider adding conditional request body logging (e.g., only in non-production environments or for specific content types).
+ private boolean shouldLogBody(HttpServletRequest request) { + String contentType = request.getContentType(); + return contentType != null && + (contentType.contains("application/json") || + contentType.contains("application/xml")); + } + @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { long startTime = System.nanoTime(); String correlationId = UUID.randomUUID().toString(); MDC.put("correlationId", correlationId); + + if (shouldLogBody(request)) { + String requestBody = request.getReader().lines() + .collect(Collectors.joining(System.lineSeparator())); + log.debug("Request Body: {}", requestBody); + }Don't forget to add the import:
import java.util.stream.Collectors;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/main/java/endolphin/backend/global/config/FilterConfig.java(1 hunks)backend/src/main/java/endolphin/backend/global/logging/LogFilter.java(1 hunks)backend/src/main/resources/logback-spring.xml(1 hunks)
🔇 Additional comments (5)
backend/src/main/java/endolphin/backend/global/config/FilterConfig.java (2)
13-19: LGTM! LogFilter registration looks good.The LogFilter is correctly registered with order 1 and appropriate URL pattern for API endpoints.
21-28: Consider restricting JwtAuthFilter to API endpoints.The JwtAuthFilter is currently applied to all endpoints ("/"). Consider restricting it to API endpoints ("/api/v1/") for consistency with LogFilter, unless there's a specific requirement to authenticate all endpoints.
backend/src/main/java/endolphin/backend/global/logging/LogFilter.java (1)
20-25: LGTM! Request logging setup looks good.Good use of correlation ID and MDC for request tracking.
backend/src/main/resources/logback-spring.xml (2)
40-46: LGTM! Async appender configuration looks good.Good configuration of the async appender with:
- Appropriate queue size
- No discarding of events
- Never blocking behavior
26-38: Ensure proper log file permissions in production.The RollingFileAppender will create log files in production. Ensure that:
- The application has write permissions to the log directory
- Log files have appropriate permissions for security
Add this to your deployment checklist:
| value="%d{yyyy-MM-dd HH:mm:ss}:%-4relative %highlight(%-5level) %magenta(${PID:-}) [%thread] %cyan(%logger{36}) %X{correlationId} - %msg%n" /> | ||
| <property name="FILE_LOG_PATTERN" | ||
| value="%d{yyyy-MM-dd HH:mm:ss}:%-4relative %-5level ${PID:-} [%thread] %logger{36} %X{correlationId} - %msg%n" /> | ||
| <property name="LOG_PATH" value="/var/logs/myapp"/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making log path configurable.
The log path is hardcoded. Consider making it configurable through Spring properties:
- <property name="LOG_PATH" value="/var/logs/myapp"/>
+ <springProperty scope="context" name="LOG_PATH" source="logging.file.path" defaultValue="/var/logs/myapp"/>Then add to your application.properties/yaml:
logging:
file:
path: /var/logs/myapp
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
New Features
Bug Fixes
Refactor