Add TimedAspect bean and metrics for FCM notifications#53
Merged
Conversation
Test Results72 tests 72 ✅ 1s ⏱️ Results for commit 5bd5d6a. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds Micrometer/Prometheus instrumentation around notification dispatch and FCM sending, and adds new tests around notification metrics exposure plus holiday notification generation. It extends the existing notification subsystem so operators can observe dispatch behavior and validate some of that behavior through unit, integration, and end-to-end tests.
Changes:
- Added
TimedAspect/AOP-based timing support and global metrics configuration usage for notification dispatch timing. - Instrumented
NotificationDispatchServiceandFcmServicewith new counters for dispatch outcomes, FCM send results, and token invalidation paths. - Added notification-focused tests: unit/integration/E2E metrics tests and a holiday-generation unit test suite.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/kotlin/com/moa/common/config/MetricsConfig.kt |
Enables AOP timing support and exposes the TimedAspect bean for Micrometer annotations. |
src/main/kotlin/com/moa/service/FcmService.kt |
Adds FCM send/result counters and token invalidation metrics during error handling. |
src/main/kotlin/com/moa/service/notification/NotificationDispatchService.kt |
Adds dispatch counters and a timed annotation around notification processing. |
src/test/kotlin/com/moa/service/notification/NotificationBatchServiceHolidayTest.kt |
Adds classicist-style unit coverage for public-holiday notification generation rules. |
src/test/kotlin/com/moa/service/notification/NotificationDispatchIntegrationTest.kt |
Verifies dispatch metrics and common tags inside a Spring integration context. |
src/test/kotlin/com/moa/service/notification/NotificationDispatchServiceMetricsTest.kt |
Adds unit tests for dispatch failure/pending metric increments. |
src/test/kotlin/com/moa/service/notification/PrometheusEndpointE2eTest.kt |
Verifies /actuator/prometheus exposes the new notification metrics over HTTP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .tag("reason", reason) | ||
| .register(meterRegistry) | ||
|
|
||
| @Timed(value = "moa.notification.dispatch", percentiles = [0.5, 0.95, 0.99]) |
Comment on lines
85
to
89
| MessagingErrorCode.INVALID_ARGUMENT -> { | ||
| log.warn("유효하지 않은 FCM 토큰 삭제: {}", token) | ||
| fcmTokenRepository.deleteByToken(token) | ||
| tokenInvalidated("invalid_argument").increment() | ||
| } |
Comment on lines
+76
to
+77
| assertThat(body).contains("moa_notification_dispatch_seconds") | ||
| assertThat(body).contains("quantile=\"0.95\"") |
| * | ||
| * 통합 테스트(`NotificationDispatchIntegrationTest`)와의 차이: | ||
| * - 통합: `MeterRegistry` 객체를 직접 주입받아 메트릭 값을 in-process로 단언. | ||
| * - E2E: 실제 HTTP 서버(Random Port)를 띄우고 `TestRestTemplate`으로 `/actuator/prometheus` |
Comment on lines
+83
to
+88
| // 4) common tag — 운영 PromQL의 `deploy_color`, `app_version` 라벨 분리에 필수 | ||
| assertThat(body).contains("application=\"moa\"") | ||
| assertThat(body).contains("deploy_color=") | ||
| assertThat(body).contains("app_version=") | ||
| } | ||
|
|
Comment on lines
+30
to
+39
| private fun pendingCounter(type: String): Counter = Counter.builder(METRIC_PENDING) | ||
| .description("발송 시도된 알림 수") | ||
| .tag("notification_type", type) | ||
| .register(meterRegistry) | ||
|
|
||
| private fun failedCounter(type: String, reason: String): Counter = Counter.builder(METRIC_FAILED) | ||
| .description("발송 실패한 알림 수") | ||
| .tag("notification_type", type) | ||
| .tag("reason", reason) | ||
| .register(meterRegistry) |
Comment on lines
+30
to
+33
| private fun pendingCounter(type: String): Counter = Counter.builder(METRIC_PENDING) | ||
| .description("발송 시도된 알림 수") | ||
| .tag("notification_type", type) | ||
| .register(meterRegistry) |
Comment on lines
+62
to
+63
| val tagKeys = pending?.id?.tags?.map { it.key }.orEmpty() | ||
| assertThat(tagKeys).contains("application", "deploy_color", "app_version", "notification_type") |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+125
to
+130
| @TestConfiguration | ||
| class NoopSchedulerConfig { | ||
| @Bean | ||
| @Primary | ||
| fun noopDispatchScheduler(): NotificationDispatchScheduler = | ||
| mockk(relaxed = true) |
Comment on lines
+35
to
+39
| private fun failedCounter(type: String, reason: String): Counter = Counter.builder(METRIC_FAILED) | ||
| .description("발송 실패한 알림 수") | ||
| .tag("notification_type", type) | ||
| .tag("reason", reason) | ||
| .register(meterRegistry) |
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.
This pull request introduces comprehensive metrics and monitoring enhancements to the notification and FCM (Firebase Cloud Messaging) services, as well as adds a classicist-style unit test for the notification batch service's holiday logic. The main changes are grouped into metrics/monitoring improvements and testing additions.
Metrics and Monitoring Enhancements:
Metrics and AOP Configuration:
TimedAspectbean inMetricsConfigto support method-level timing annotations for metrics collection. (src/main/kotlin/com/moa/common/config/MetricsConfig.kt) [1] [2]FCM Service Metrics:
FcmServiceto track successful and failed FCM message sends, as well as automatic token invalidations due to specific error codes. These counters are incremented appropriately during FCM operations and error handling. (src/main/kotlin/com/moa/service/FcmService.kt) [1] [2] [3]Notification Dispatch Metrics:
NotificationDispatchServiceto monitor the number of pending notifications by type and failed notifications by type and reason. These metrics are incremented during notification processing, including attempts, failures due to missing tokens, message build errors, and FCM send failures. Also, annotated the main dispatch method with@Timedto collect execution time metrics. (src/main/kotlin/com/moa/service/notification/NotificationDispatchService.kt) [1] [2] [3] [4] [5] [6]Testing Additions:
Classicist-style Unit Test:
NotificationBatchServiceHolidayTest, a classicist unit test suite that uses fake repositories for all out-of-process dependencies and verifies notification log generation logic for public holidays, including various edge cases and idempotency. (src/test/kotlin/com/moa/service/notification/NotificationBatchServiceHolidayTest.kt)