Repository navigation
feat(telemetry): wire desktop Registration Client metrics to TUSD ingestion backend - #818
yakshithkd23 wants to merge 3 commits into
Conversation
WalkthroughEnables Spring scheduling in AppConfig and adds a new TelemetryUploadService that periodically reads a local metrics.log file and uploads it to a configured TUS server using resumable uploads, deleting the file on success. New application properties configure the TUS server URL, chunk size, and log path. ChangesTelemetry Upload via TUS
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant TelemetryUploadService
participant TusClient
participant TUSServer
Scheduler->>TelemetryUploadService: scheduledUpload() every 30s
TelemetryUploadService->>TelemetryUploadService: locate metrics.log
TelemetryUploadService->>TusClient: configure uploadCreationURL + TusURLMemoryStore
TelemetryUploadService->>TusClient: resumeOrCreateUpload(tusUpload)
loop upload chunks
TusClient->>TUSServer: PATCH chunk
TUSServer-->>TusClient: offset ack
TelemetryUploadService->>TusClient: uploadChunk()
end
TusClient->>TUSServer: finish upload
TelemetryUploadService->>TelemetryUploadService: delete metrics.log (if upload succeeded)
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/config/TelemetryUploadService.java`:
- Around line 26-28: There are now two scheduled telemetry upload paths using
the same TUS config and metrics file, so consolidate the logic into one
scheduler and remove/disable the duplicate. Update TelemetryUploadService and
the existing scheduled exporter in MetricsConfig to keep a single upload flow
for metrics.log, then ensure only one job performs upload and cleanup to avoid
races, double-sends, and conflicting deletions.
- Around line 56-57: The upload flow in
TelemetryUploadService.uploadFileToTusServer is operating on the live
metrics.log that LoggingJsonMeterRegistry is still writing to, which risks
losing or splitting entries and deleting data produced during upload. Change the
upload path to use a rotated or snapshotted copy of the log instead of the
active writer target, and update the cleanup logic in the upload flow so it only
removes the temporary snapshot after a successful transfer.
- Around line 41-45: The scheduled upload in
TelemetryUploadService.scheduledUpload is still using the temporary test cadence
instead of the production interval. Update the `@Scheduled` fixedDelay value back
to 15 minutes (900000 ms), or gate the shorter delay behind a test-only profile
so production behavior remains unchanged.
- Around line 64-72: The resumable upload setup in TelemetryUploadService
currently uses a fresh TusURLMemoryStore, so retry runs cannot recover the
previous upload URL. Update the upload flow in the method that creates the
TusClient to reuse the existing MetricsURLMemoryStore instead of constructing a
new in-memory store, so resumeOrCreateUpload can continue the same upload across
scheduled retries.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7482eff3-bca9-4586-b84b-8ffa9b7285cc
📒 Files selected for processing (4)
registration/registration-services/pom.xmlregistration/registration-services/src/main/java/io/mosip/registration/config/AppConfig.javaregistration/registration-services/src/main/java/io/mosip/registration/config/TelemetryUploadService.javaregistration/registration-services/src/main/resources/props/mosip-application.properties
a8be324 to
cc450b9
Compare
…estion backend Signed-off-by: Yakshith K D <yakshithkd97@gmail.com>
cc450b9 to
1acb035
Compare
…estion backend - Add TelemetryUploadService to upload metrics.log via TUS protocol - Enable @EnableScheduling in AppConfig for scheduled uploads - Add TUS server URL, chunk size, and metrics log path config Signed-off-by: Yakshith K D <yakshithkd97@gmail.com>
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
`@registration/registration-services/src/main/java/io/mosip/registration/config/TelemetryUploadService.java`:
- Line 27: Remove the redundant `@EnableScheduling` annotation from
TelemetryUploadService, since scheduling is already enabled in AppConfig; keep
the `@Service` focused on service behavior and leave the scheduling enablement
only on the configuration class.
- Around line 56-63: The upload path in
TelemetryUploadService.uploadFileToTusServer currently sends metrics.log as-is,
but that file can contain sensitive `@MetricTag` values produced by
MetricsConfig.tagFactory and written verbatim by LoggingJsonMeterRegistry. Add a
sanitization step before the upload begins that whitelists allowed tags or
redacts sensitive ones (such as userid) from the file contents, and make sure
the cleaned file is what gets egressed instead of the raw metrics log.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc9329f7-71e1-44e5-be6a-641b40df3073
📒 Files selected for processing (3)
registration/registration-services/src/main/java/io/mosip/registration/config/AppConfig.javaregistration/registration-services/src/main/java/io/mosip/registration/config/TelemetryUploadService.javaregistration/registration-services/src/main/resources/props/mosip-application.properties
MetricsConfig.java already contains the complete TUS upload scheduler (exportMetrics, @scheduled every 15 min, TusExecutor with retry, MetricsURLMemoryStore for resumable uploads). TelemetryUploadService.java was a duplicate that would race with MetricsConfig.exportMetrics() on the same file. Removed to keep a single upload path. Previous commit adds tus-java-client dependency and TUS config keys to activate the existing MetricsConfig.exportMetrics() scheduler. Signed-off-by: Yakshith K D <yakshithkd97@gmail.com>
yakshithkd23
left a comment
There was a problem hiding this comment.
Wired desktop registration client metrics to Tusd ingestion backend
|
Hi Team, I have finalized the changes for the Telemetry TUSD upload integration. To summarize the progression and current state of this PR:
All end-to-end verifications, including scheduled execution and successful TUSD ingestion, have been validated. I believe this approach meets the requirements for a cloud-native telemetry pipeline while keeping the codebase maintainable. Please let me know if any further adjustments are required. I am ready for final review and merge. |
|
Hi team, quick update on the latest changes: I have finalized the telemetry integration. I performed a review of the develop branch and confirmed that the TUSD upload logic and necessary metrics configuration were already present in MetricsConfig.java. To avoid code duplication and maintain a clean architecture, I have kept the existing implementation rather than creating a redundant service. Summary of changes: AppConfig.java: Added @EnableScheduling to trigger the automated upload task. mosip-application.properties: Added the required TUSD server configuration keys (url, chunksize, log.path). I have validated that the existing MetricsConfig logic correctly handles the JSON schema and TUS protocol as per MOSIP requirements. Everything is functioning as expected. Please review these final changes. Thanks! |
Summary
This PR implements the client-side telemetry upload layer for the Desktop Registration Client as part of issue mosip/android-registration-client#719.
The MOSIP Registration Client already collects JVM and application metrics using the existing
LoggingJsonMeterRegistry(Micrometer). This PR provides the missing link: a scheduled upload service that transmits these pre-collected metrics to the backend ingestion pipeline implemented in mosip/tusd-server#14.Historical Context & Evolution
During the initial development and review process against the
masterbranch, the implementation began as a standaloneTelemetryUploadService.java. Following architectural feedback to improve maintainability, we refactored the design:Initial Attempt: Created a dedicated
TelemetryUploadServiceto handle the TUS-based transport logic.Refinement: During the transition to the
developbranch, we identified that merging the scheduling and transport logic into the existingMetricsConfig.javawould better align with the project's architecture and avoid unnecessary service fragmentation.Final Approach: Consolidated the upload logic into
MetricsConfig.java. This ensures that the telemetry transport is handled as a natural extension of the metric configuration provided by the MOSIP framework.Problem
While the MOSIP
LoggingJsonMeterRegistrywas already correctly collecting and writing telemetry metrics tometrics.log, there was no mechanism to upload these logs to the server. Consequently, telemetry data remained trapped on local disks, preventing centralized monitoring of JVM health, resource utilization, and application performance.Solution
Integrated the upload transport logic into
MetricsConfig.java:Scheduling: Enabled Spring
@EnableSchedulinginAppConfig.java.Upload Logic: Implemented the task within
MetricsConfigto utilize the existing telemetry setup.TUS Integration: Integrated
tus-java-client:0.4.3to handle reliable, resumable chunked uploads of the existingmetrics.logfile to the mosip-tusd server.Resilience: The implementation respects the existing log lifecycle—ensuring the file is only deleted after a successful TUS upload, with automatic retries on failure.
Architecture
Files Changed
File | Change -- | -- MetricsConfig.java | Refactored: Added scheduled upload logic + TUS integration AppConfig.java | Enabled @EnableScheduling mosip-application.properties | Added telemetry/TUS configuration properties