[EDMT-452] 커스텀 메트릭 정합성 문제 해결#67
Conversation
…nd update related references
…upport additional parameter
…ionMetrics method
…lect new parameter structure
Walkthrough애플리케이션에 AspectJ 자동 프록시를 활성화하고, AI 학생기록 생성 흐름에서 레코드 타입 전달을 제거해 Facade/Aspect가 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Controller
participant F as StudentRecordAIFacade
participant S as StudentRecordService
participant P as AIPromptGenerator
participant T as TaskService
participant E as EventPublisher
C->>F: createTaskId(memberId, recordId, byteCount, prompt)
F->>S: getRecordDetail(memberId, recordId)
S-->>F: StudentRecord (type)
F->>P: createStreamingPrompt(type, byteCount, prompt)
P-->>F: requestPrompt
F->>T: createTask(requestPrompt)
T-->>F: taskId
F->>E: publishTaskCreated(taskId)
F-->>C: StudentRecordTaskResponse(taskId)
sequenceDiagram
autonumber
participant Biz as Annotated Method
participant AOP as StudentRecordMetricsAspect
participant S as StudentRecordService
participant R as RecordGenerationTracker
participant M as StudentRecordMetricsCounter
Note over Biz,AOP: @StudentRecordMetrics가 붙은 비즈니스 메서드 실행
Biz->>AOP: proceed(...)
Biz-->>AOP: return
AOP->>S: getRecordDetail(memberId, recordId)
S-->>AOP: StudentRecord (type)
AOP->>R: isFirstGeneration(recordId)
alt first-generation
AOP->>M: countFirstGeneration(type)
else regeneration
AOP->>M: countRegeneration(type)
end
AOP->>M: countRequest(type)
Note over AOP: 즉시 카운팅, 트랜잭션 동기화 제거
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/StudentRecordMetricsCounter.java (1)
46-53: 하드코딩된 바이트 임계값을 설정 가능하게 개선 고려완료 판단을 위한 최소 바이트 수(1000, 750)가 하드코딩되어 있습니다. 향후 요구사항 변경에 대응하기 위해 이 값들을 설정 가능한 상수나 프로퍼티로 관리하는 것을 고려해보세요.
+ private static final int SUBJECT_MIN_BYTES = 1000; + private static final int DEFAULT_MIN_BYTES = 750; + private boolean isCompleted(final StudentRecordType type, final String description) { if (description == null || description.trim().isEmpty()) { return false; } - int minBytes = (type == StudentRecordType.SUBJECT) ? 1000 : 750; + int minBytes = (type == StudentRecordType.SUBJECT) ? SUBJECT_MIN_BYTES : DEFAULT_MIN_BYTES; return description.getBytes(StandardCharsets.UTF_8).length >= minBytes; }edukit-core/src/main/java/com/edukit/core/studentrecord/aop/StudentRecordMetricsAspect.java (2)
36-37: null 체크 필요
studentRecordService.getRecordDetail()이 null을 반환하거나 예외를 발생시킬 가능성이 있습니다. null 체크나 예외 처리를 추가하는 것을 고려해보세요.StudentRecord studentRecord = studentRecordService.getRecordDetail(memberId, recordId); + if (studentRecord == null) { + log.warn("StudentRecord not found for memberId: {}, recordId: {}", memberId, recordId); + return; + } StudentRecordType recordType = studentRecord.getStudentRecordType();
56-57: 중복된 null 체크 필요여기서도
getRecordDetail()결과에 대한 null 체크가 필요합니다.StudentRecord studentRecord = studentRecordService.getRecordDetail(memberId, recordId); + if (studentRecord == null) { + log.warn("StudentRecord not found for memberId: {}, recordId: {}", memberId, recordId); + return joinPoint.proceed(); + } StudentRecordType recordType = studentRecord.getStudentRecordType();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
edukit-api/src/main/java/com/edukit/EdukitApiApplication.java(1 hunks)edukit-api/src/main/java/com/edukit/studentrecord/controller/StudentRecordAIController.java(1 hunks)edukit-api/src/main/java/com/edukit/studentrecord/facade/StudentRecordAIFacade.java(2 hunks)edukit-api/src/main/java/com/edukit/studentrecord/facade/StudentRecordFacade.java(0 hunks)edukit-core/src/main/java/com/edukit/core/studentrecord/aop/StudentRecordMetricsAspect.java(3 hunks)edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java(1 hunks)edukit-core/src/main/java/com/edukit/core/studentrecord/service/StudentRecordMetricsCounter.java(1 hunks)
💤 Files with no reviewable changes (1)
- edukit-api/src/main/java/com/edukit/studentrecord/facade/StudentRecordFacade.java
🧰 Additional context used
🧬 Code graph analysis (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/aop/StudentRecordMetricsAspect.java (1)
Slf4j(17-83)
🔇 Additional comments (13)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/StudentRecordMetricsCounter.java (1)
11-11: 클래스명 변경 확인
StudentRecordMetricsService에서StudentRecordMetricsCounter로 클래스명이 변경되었습니다. 이 변경사항이 올바르게 적용되었음을 확인합니다.edukit-api/src/main/java/com/edukit/EdukitApiApplication.java (1)
7-9: AspectJ 자동 프록시 활성화 확인
@EnableAspectJAutoProxy어노테이션이 추가되어 AOP 기반 메트릭스 수집 기능이 정상 작동할 것으로 확인됩니다.edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (2)
8-12: 클래스명 및 어노테이션 변경 확인
GenerationTrackingService에서RecordGenerationTracker로 클래스명이 변경되고,@Service에서@Component로 어노테이션이 변경되었습니다. Spring 컨테이너에서 정상적으로 빈으로 관리될 것으로 확인됩니다.
16-30: 동시성 처리 적절함
compute메서드를 사용하여 원자적으로 업데이트하고 있어 동시성 처리가 적절합니다.edukit-core/src/main/java/com/edukit/core/studentrecord/aop/StudentRecordMetricsAspect.java (4)
23-25: 의존성 변경 확인필드 타입이 올바르게 업데이트되었습니다:
StudentRecordMetricsService→StudentRecordMetricsCounterGenerationTrackingService→RecordGenerationTracker- 새로운
StudentRecordService추가변경사항이 적절하게 적용되었습니다.
27-27: 포인트컷 표현식 명시화
@AfterReturning어노테이션에 명시적으로pointcut속성을 지정하여 가독성이 향상되었습니다.
39-44: 메트릭 수집 실패 처리 적절함메트릭 수집 실패 시 로그만 남기고 비즈니스 로직을 계속 진행하는 것은 적절한 처리입니다.
60-72: AI 생성 메트릭 수집 로직 적절함첫 생성과 재생성을 구분하여 메트릭을 수집하는 로직이 명확하고 적절합니다.
edukit-api/src/main/java/com/edukit/studentrecord/controller/StudentRecordAIController.java (2)
33-33: 매개변수 포맷팅 개선
memberId와recordId를 한 줄에 배치하여 가독성이 향상되었습니다.
36-38: Facade 메서드 호출 간소화
createTaskId메서드 호출이 간소화되어 record type을 전달하지 않게 되었습니다. 이는 Facade에서 내부적으로 타입을 조회하도록 변경된 설계와 일치합니다.edukit-api/src/main/java/com/edukit/studentrecord/facade/StudentRecordAIFacade.java (3)
32-37: 메서드 시그니처 변경이 적절하며 타입 안전성이 향상되었습니다.
recordType파라미터를 제거하고 런타임에StudentRecord에서 직접 타입을 조회하도록 변경한 것은 좋은 개선입니다. 이는 데이터 일관성을 보장하고 클라이언트에서 잘못된 타입을 전달할 가능성을 제거합니다.
31-31: AOP 메트릭 수집이 올바르게 적용되었습니다.
@AIGenerationMetrics어노테이션이 메서드에 적절히 적용되어 있어 성능 모니터링이 가능합니다.
6-6: 권한 검증 확인 — StudentRecordService.getRecordDetail이 소유권을 검증합니다.getRecordDetail(memberId, recordId)는 getRecordDetailById(recordId) 호출 후 validatePermission(existingDetail.getStudent(), memberId)를 호출하며, validatePermission은 student.getMember().getId()와 memberId를 비교해 불일치 시 StudentRecordErrorCode.PERMISSION_DENIED 예외를 던집니다. 위치: edukit-core/src/main/java/com/edukit/core/studentrecord/service/StudentRecordService.java (getRecordDetail / validatePermission).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (2)
23-26: null 반환에 대한 더 구체적인 예외 처리를 고려하세요.Redis INCR 연산이 null을 반환하는 경우는 매우 예외적인 상황입니다. 현재는 warning 로그만 남기고 false를 반환하지만, 이런 상황에서는 더 적극적인 대응이 필요할 수 있습니다.
메트릭 수집의 정확성을 위해 예외를 던지거나 재시도 로직을 고려해보세요.
if (newCount == null) { - log.warn("Redis INCR returned null for key: {}", key); - return false; + log.error("Redis INCR returned null for key: {}, this should not happen", key); + throw new IllegalStateException("Redis INCR operation failed for key: " + key); }
28-33: TTL 설정 실패에 대한 예외 처리가 누락되었습니다.
redisTemplate.expire(key, DEFAULT_TTL)호출이 실패할 경우에 대한 처리가 없습니다. expire 연산 실패 시 키가 영구적으로 남아있을 수 있어 메모리 누수로 이어질 수 있습니다.if (newCount == 1L) { // 최초 생성 시에만 TTL 설정 (비정상 흐름 방치 대비) - redisTemplate.expire(key, DEFAULT_TTL); + Boolean expireResult = redisTemplate.expire(key, DEFAULT_TTL); + if (Boolean.FALSE.equals(expireResult)) { + log.warn("Failed to set TTL for key: {}", key); + } log.debug("RecordId: {}, Generation count set to 1 (first)", recordId); return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/aop/StudentRecordMetricsAspect.java (1)
Slf4j(17-83)
🔇 Additional comments (3)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (3)
1-17: 클래스 구조와 의존성이 잘 설계되었습니다.Redis를 사용한 생성 횟수 추적 로직이 명확하고, Lombok을 활용한 의존성 주입 구조도 적절합니다. 상수 정의와 TTL 설정도 합리적입니다.
39-41: 키 생성 로직이 명확하고 적절합니다.키 생성 방식이 일관되고 예측 가능하며, 네이밍 컨벤션도 적절합니다.
12-12: 이전 리뷰 코멘트와 관련된 메모리 누수 문제가 해결되었습니다.Redis 기반 구현으로 변경되면서 TTL을 통해 자동 만료되므로, 이전에 지적된
ConcurrentHashMap의 무제한 증가 문제가 해결되었습니다.
…c increment and expiration in Redis
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (1)
39-46: Redis 실행 예외 방어 및 TTL 클램프(소프트 가드)메트릭 경로에서 Redis 장애가 잦다면 여기서도 예외를 흡수하고 안전하게 false로 폴백하는 편이 안정적입니다. 또한 TTL을 최소 1초로 클램프해 즉시만료를 방지하세요.
적용 제안(diff):
- Long newCount = redisTemplate.execute( - INCR_EXPIRE_SCRIPT, - Collections.singletonList(key), - String.valueOf(ttlSeconds) - ); + Long newCount; + try { + newCount = redisTemplate.execute( + INCR_EXPIRE_SCRIPT, + Collections.singletonList(key), + String.valueOf(Math.max(1L, ttlSeconds)) + ); + } catch (Exception e) { + log.warn("Redis script execution failed for key: {}", key, e); + return false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (1)
edukit-core/src/main/java/com/edukit/core/studentrecord/aop/StudentRecordMetricsAspect.java (1)
Slf4j(17-83)
🔇 Additional comments (4)
edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java (4)
27-35: Lua 스크립트/DefaultRedisScript 구성 적절
INCR+EXPIRE를 Lua로 원자화하고ResultType을 명시한 점 좋습니다. 캐시/TTL 안정성 측면에서 이전 레이스 이슈가 해소됩니다.
39-59: 카운트 증가 시점 확인(성공 시점 기준인지 재검토 권장)본 메서드는 호출 즉시 카운트를 증가시킵니다. 제공된 Aspect 코드에서는
proceed()이전에 호출되어 비즈니스 실패/롤백 시에도 ‘첫 생성’으로 카운트가 소모될 수 있습니다. 의도(요청 기준 vs. 성공 기준)를 확인해 주세요. 필요 시 Aspect에서 성공 이후로 이동하거나 보상 로직을 고려하세요.
61-63: 키 스코프 확인 요청키가
sr:gen:{recordId}:count형태입니다.recordId가 전역 유일임을 전제합니다. 멀티 테넌시/멤버 스코프가 필요하면 키에 테넌트/멤버 식별자를 포함하는 방안을 검토해 주세요.
16-18: TTL 프로퍼티 기본값 및 유효성 추가 필요설정 누락 시 즉시 만료(0s) 또는 예외가 발생할 수 있으니 안전한 기본값(예: 300초)과 유효성(>=1) 추가하세요.
파일: edukit-core/src/main/java/com/edukit/core/studentrecord/service/RecordGenerationTracker.java
적용 제안(diff):
- @Value("${record.generation.ttl}") + @Value("${record.generation.ttl:300}") private long ttlSeconds;검증: rg 실행 결과 "No files were searched"로 레포 내 설정 존재 여부를 확인하지 못했습니다. 설정이 있는지 수동 확인하거나 아래 명령으로 재검증 후 반영하세요:
rg -n --hidden --no-ignore -C2 'record\.generation\.ttl' || true
… set Redis expiration based on TTL argument
wldks1008
changed the title
1 participant
📣 Jira Ticket
EDMT-452
Summary by CodeRabbit