feat(telemetry): implement async POST /api/v1/telemetry/ping endpoint#117
Open
feat(telemetry): implement async POST /api/v1/telemetry/ping endpoint#117
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Spring Web dependency; introduces a telemetry REST endpoint POST /api/v1/telemetry/ping that delegates to an async TelemetryService which persists telemetry records; enables Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as TelemetryController
participant Service as TelemetryService
participant Repository as TelemetryRepository
participant DB as Storage
Client->>Controller: POST /api/v1/telemetry/ping (TelemetryPingDTO)
Controller->>Controller: Log receipt (itemId)
Controller->>Service: processPing(pingDTO) (async)
Service->>Service: Build TelemetryRecord (copy fields + serverReceivedTimestamp)
Service->>Repository: save(record)
Repository->>DB: persist telemetry_record
DB-->>Repository: persistence confirmation
Repository-->>Service: saved record
Service->>Service: Log completion / errors
Controller-->>Client: HTTP 202 Accepted {"status":"success"}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
src/main/resources/application.properties (1)
6-6: Use environment-variable binding instead of literal placeholder secrets.Line 6 and Line 14 currently use sentinel strings that can accidentally ship as runtime values. Prefer direct env binding for safer deployment behavior.
♻️ Proposed config change
-spring.datasource.password=YOUR_PASSWORD_HERE +spring.datasource.password=${SPRING_DATASOURCE_PASSWORD} -spring.mongodb.uri=YOUR_MONGODB_URI_HERE +spring.mongodb.uri=${SPRING_MONGODB_URI}Also applies to: 14-14
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application.properties` at line 6, Replace the literal sentinel secret value used for spring.datasource.password with an environment-bound expression so the runtime password is sourced from an env var; specifically, update the property key spring.datasource.password to use a placeholder like ${DB_PASSWORD} (with optional default) and do the same for the other sentinel at line 14 — ensure you reference the exact property names (e.g., spring.datasource.password and the other property key found at line 14) and remove hard-coded "YOUR_PASSWORD_HERE" values so deployments read secrets from environment variables instead of shipping literal placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/p2ps/telemetry/controller/TelemetryController.java`:
- Around line 20-29: The controller currently returns 202 Accepted while calling
telemetryService.processPing synchronously inside receivePing, which is
misleading; either make processing asynchronous or change the response to
indicate completion—prefer the simpler fix: replace ResponseEntity.accepted()
with ResponseEntity.ok() (or ResponseEntity.status(HttpStatus.CREATED) if a new
resource is created) in receivePing so the HTTP status matches the synchronous
telemetryService.processPing behavior and update any tests/clients expecting 202
accordingly.
---
Nitpick comments:
In `@src/main/resources/application.properties`:
- Line 6: Replace the literal sentinel secret value used for
spring.datasource.password with an environment-bound expression so the runtime
password is sourced from an env var; specifically, update the property key
spring.datasource.password to use a placeholder like ${DB_PASSWORD} (with
optional default) and do the same for the other sentinel at line 14 — ensure you
reference the exact property names (e.g., spring.datasource.password and the
other property key found at line 14) and remove hard-coded "YOUR_PASSWORD_HERE"
values so deployments read secrets from environment variables instead of
shipping literal placeholders.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ae4842f-0ad0-4ab9-9ff7-57da5afaf2c0
📒 Files selected for processing (4)
build.gradle.ktssrc/main/java/p2ps/telemetry/controller/TelemetryController.javasrc/main/java/p2ps/telemetry/services/TelemetryService.javasrc/main/resources/application.properties
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/p2ps/P2PShoppingApplication.java (1)
8-9: Consider configuring a customTaskExecutorfor bounded thread pool.Using
@EnableAsyncwithout a customTaskExecutordefaults toSimpleAsyncTaskExecutor, which creates a new unbounded thread per async invocation. Under high telemetry ping load, this could exhaust system resources.Consider defining a
TaskExecutorbean with bounded pool size:`@Bean` public TaskExecutor taskExecutor() { ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); executor.setCorePoolSize(4); executor.setMaxPoolSize(10); executor.setQueueCapacity(100); executor.setThreadNamePrefix("telemetry-async-"); executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy()); executor.initialize(); return executor; }Additionally, note that exceptions thrown in
TelemetryService.processPingwill be handled by Spring's defaultAsyncUncaughtExceptionHandler(logged only) rather than propagated to the client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/p2ps/P2PShoppingApplication.java` around lines 8 - 9, The app enables async with `@EnableAsync` in P2PShoppingApplication but no TaskExecutor is configured, so Spring uses SimpleAsyncTaskExecutor (unbounded threads); add a TaskExecutor bean in P2PShoppingApplication that returns a ThreadPoolTaskExecutor with bounded core/max pool sizes, queueCapacity, threadNamePrefix and a rejectedExecutionHandler (e.g., CallerRunsPolicy) and call initialize() so async calls use a bounded pool; also be aware exceptions in TelemetryService.processPing invoked asynchronously will use AsyncUncaughtExceptionHandler, so consider providing a custom AsyncUncaughtExceptionHandler bean if you need better handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/p2ps/P2PShoppingApplication.java`:
- Line 6: The import org.springframework.scheduling.annotation.EnableScheduling
in P2PShoppingApplication is unused; remove the import line to clean up unused
dependencies (or if you intended to enable scheduled tasks, add the
`@EnableScheduling` annotation to the P2PShoppingApplication class). Adjust
imports accordingly so the compiler no longer flags EnableScheduling as unused.
---
Nitpick comments:
In `@src/main/java/p2ps/P2PShoppingApplication.java`:
- Around line 8-9: The app enables async with `@EnableAsync` in
P2PShoppingApplication but no TaskExecutor is configured, so Spring uses
SimpleAsyncTaskExecutor (unbounded threads); add a TaskExecutor bean in
P2PShoppingApplication that returns a ThreadPoolTaskExecutor with bounded
core/max pool sizes, queueCapacity, threadNamePrefix and a
rejectedExecutionHandler (e.g., CallerRunsPolicy) and call initialize() so async
calls use a bounded pool; also be aware exceptions in
TelemetryService.processPing invoked asynchronously will use
AsyncUncaughtExceptionHandler, so consider providing a custom
AsyncUncaughtExceptionHandler bean if you need better handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 772afc3a-2ad9-4560-b6c2-b82d954b9047
📒 Files selected for processing (2)
src/main/java/p2ps/P2PShoppingApplication.javasrc/main/java/p2ps/telemetry/services/TelemetryService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/p2ps/telemetry/services/TelemetryService.java
Author
|
SonarQube failed because of applicationproperties where the password and the mongoDB URI are replaced with placeholders. Ran it and got online access + responses from mongoDB |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/p2ps/telemetry/services/TelemetryService.java (2)
32-37: Silently swallowed exceptions may hide persistent failures.The
catch (Exception e)logs the error but doesn't propagate or track failures. For fire-and-forget telemetry this may be acceptable, but consider:
- Adding metrics/alerting for save failures so operational issues don't go unnoticed.
- If persistence is critical, implementing a retry mechanism or dead-letter queue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/p2ps/telemetry/services/TelemetryService.java` around lines 32 - 37, In TelemetryService where telemetryRepository.save(record) is wrapped in a try/catch (and currently only logs via log.error), update the catch block to both emit an operational metric/counter (e.g., telemetrySaveFailures.increment or use your metrics client) and implement a failure handling path: either retry the save with a bounded backoff/retry (limited attempts) or push the failing record to a dead-letter queue/enqueue for later processing, and if persistence is critical rethrow or return a failure status upstream instead of silently swallowing; reference telemetryRepository.save(record), log.error(...) and pingDTO.getItemId() when adding contextual logging/metric labels.
1-1: Package naming convention violation (low priority).SonarCloud flags
p2psas containing a digit which violates the standard Java package naming convention. Since this is a project-wide naming decision, consider addressing it in a separate refactor if desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/p2ps/telemetry/services/TelemetryService.java` at line 1, SonarCloud flags the package name "p2ps.telemetry.services" for containing a digit; to fix this rename the package across the project (for example to "p2psproject.telemetry.services" or another digit-free identifier), update the package declaration in TelemetryService.java and all other source files that use "p2ps" (adjust imports and fully-qualified references), move/rename the directory structure to match the new package, update build/config (Maven/Gradle settings, module-info if present) and run a full compile/test to ensure no remaining references to "p2ps" remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/p2ps/telemetry/services/TelemetryService.java`:
- Line 22: The local variable named record in TelemetryService (created via
TelemetryRecord record = new TelemetryRecord();) uses the restricted identifier
"record"; rename it (e.g., telemetryRecord or entry) and update all subsequent
uses in the method to that new name to avoid future Java reserved-word conflicts
and silence the SonarCloud warning; ensure you change the variable declaration
and every reference (constructor call, setters/getters, method calls) within
TelemetryService accordingly.
- Around line 18-19: Add a bounded ThreadPoolTaskExecutor bean and wire it to
the `@Async` processing to avoid unbounded threads: create an AsyncConfig class
with a telemetryExecutor bean (ThreadPoolTaskExecutor with sensible core/max
pool sizes, queueCapacity, threadNamePrefix and a CallerRunsPolicy) and annotate
the config with `@EnableAsync`; then update TelemetryService.processPing to use
that executor by changing `@Async` to `@Async`("telemetryExecutor") (or configure
the executor as the default AsyncConfigurer) so processPing uses the bounded
telemetryExecutor instead of Spring's SimpleAsyncTaskExecutor.
---
Nitpick comments:
In `@src/main/java/p2ps/telemetry/services/TelemetryService.java`:
- Around line 32-37: In TelemetryService where telemetryRepository.save(record)
is wrapped in a try/catch (and currently only logs via log.error), update the
catch block to both emit an operational metric/counter (e.g.,
telemetrySaveFailures.increment or use your metrics client) and implement a
failure handling path: either retry the save with a bounded backoff/retry
(limited attempts) or push the failing record to a dead-letter queue/enqueue for
later processing, and if persistence is critical rethrow or return a failure
status upstream instead of silently swallowing; reference
telemetryRepository.save(record), log.error(...) and pingDTO.getItemId() when
adding contextual logging/metric labels.
- Line 1: SonarCloud flags the package name "p2ps.telemetry.services" for
containing a digit; to fix this rename the package across the project (for
example to "p2psproject.telemetry.services" or another digit-free identifier),
update the package declaration in TelemetryService.java and all other source
files that use "p2ps" (adjust imports and fully-qualified references),
move/rename the directory structure to match the new package, update
build/config (Maven/Gradle settings, module-info if present) and run a full
compile/test to ensure no remaining references to "p2ps" remain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 944ad61f-f9e4-4d0b-add5-f6a34833b47d
📒 Files selected for processing (2)
src/main/java/p2ps/telemetry/services/TelemetryService.javasrc/main/resources/application.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/application.properties
…ping/server into feature/ping-controller
|
iuliaaa20
approved these changes
Mar 30, 2026
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
3 participants
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.
Summary by CodeRabbit
New Features
Chores
Infrastructure