feat: Flush telemetry events to remote ingest endpoint#283
Conversation
Add telemetry flush system that sends up to 25 events per CLI invocation to a configurable remote endpoint. Events are deleted after successful flush by default; set telemetryKeepFlushed: true in .swamp.yaml to rename to .flushed.json instead. Repo UUID (repoId) is auto-generated on init and lazy-migrated for existing repos. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review: Flush telemetry events to remote ingest endpoint
This PR adds a telemetry flush system that sends CLI invocation events to a remote endpoint. The implementation follows domain-driven design principles well and includes comprehensive test coverage.
Summary
✅ No blocking issues found. The code is well-structured, follows the project's conventions, and has good test coverage.
Code Quality Assessment
TypeScript Strict Mode Compliance: ✅
- No
anytypes used - All parameters and return types are properly typed
- Uses
readonlyappropriately for immutable fields
Named Exports: ✅
- All exports are named exports, no default exports
- Module barrel (
mod.ts) properly re-exports new types
Domain-Driven Design: ✅
TelemetrySenderis correctly modeled as a domain port (interface) insrc/domain/telemetry/HttpTelemetrySenderis correctly placed in infrastructure as an adapterTelemetryRepositoryinterface is properly extended withfindUnflushedandmarkFlushedmethodsTelemetryServiceorchestrates the flushing logic appropriately as a domain service- Follows the ports-and-adapters pattern correctly
Test Coverage: ✅
- 32 new/updated tests covering:
TelemetryService.flushTelemetry(3 tests)JsonTelemetryRepository.findUnflushed(4 tests)JsonTelemetryRepository.markFlushed(4 tests)HttpTelemetrySender.sendBatch(4 tests)
- Tests verify edge cases: empty results, network errors, non-202 responses, file not found
Security: ✅
- Uses 5-second timeout on HTTP requests to prevent hanging
- Fire-and-forget pattern prevents telemetry from blocking CLI execution
- Response body is properly consumed to prevent resource leaks
- No sensitive data appears to be transmitted (arguments are already redacted elsewhere)
Suggestions (non-blocking)
-
Documentation suggestion: Consider adding a note to the README or design docs about the telemetry endpoint configuration options (
telemetryEndpoint,telemetryKeepFlushedin.swamp.yaml). -
Minor inconsistency: The
deleteOlderThanmethod usesentry.name.endsWith(".json")which will match both.jsonand.flushed.jsonfiles - this is actually correct behavior for cleanup, but the test at line 439-479 (deleteOlderThan cleans up both flushed and unflushed files) documents this well. -
Consideration for future: The
HttpTelemetrySendersends a single event vs batch format based on array length. While this works correctly, documenting why this optimization exists (API compatibility?) might help future maintainers.
Files Reviewed
| File | Assessment |
|---|---|
src/cli/mod.ts |
✅ Clean integration with telemetry flush |
src/domain/telemetry/mod.ts |
✅ Proper exports added |
src/domain/telemetry/repositories.ts |
✅ Well-documented interface methods |
src/domain/telemetry/telemetry_sender.ts |
✅ Clean port interface |
src/domain/telemetry/telemetry_service.ts |
✅ Fire-and-forget with proper error handling |
src/domain/telemetry/telemetry_service_test.ts |
✅ Comprehensive tests |
src/infrastructure/persistence/json_telemetry_repository.ts |
✅ Robust file handling |
src/infrastructure/persistence/json_telemetry_repository_test.ts |
✅ Thorough test coverage |
src/infrastructure/persistence/repo_marker_repository.ts |
✅ New fields for telemetry config |
src/infrastructure/telemetry/http_telemetry_sender.ts |
✅ Clean adapter implementation |
src/infrastructure/telemetry/http_telemetry_sender_test.ts |
✅ Tests with real HTTP server |
LGTM! 🚀
There was a problem hiding this comment.
Code Review: Telemetry Flush Feature
Verdict: APPROVED ✅
This PR implements a well-designed telemetry flush system that follows good DDD principles and adheres to the project's code standards.
Code Quality ✅
- TypeScript strict mode: All types explicitly defined, no
anytypes - Named exports used throughout (no default exports)
- Code passes
deno checkanddeno lint
Domain-Driven Design ✅
The architecture correctly follows DDD patterns:
TelemetrySenderis a proper domain port (interface) insrc/domain/telemetry/HttpTelemetrySenderis an infrastructure adapter implementing the portTelemetryRepositoryextension withfindUnflushed/markFlushedis appropriateTelemetryService.flushTelemetry()is a proper domain service method orchestrating the operation
Test Coverage ✅
Comprehensive tests added:
telemetry_service_test.ts: 3 tests for flush success, failure, no-opjson_telemetry_repository_test.ts: 7 tests for findUnflushed, markFlushed, cleanuphttp_telemetry_sender_test.ts: 4 tests for single/batch sends, error handling
Security ✅
- Fire-and-forget pattern prevents telemetry from blocking CLI
- 5-second timeout on HTTP requests prevents hanging
- Error handling suppresses exceptions appropriately
repoIdUUID provides anonymity (no PII sent)- Telemetry endpoint is user-configurable
Suggestions (non-blocking)
-
Sequential markFlushed: The
doFlushmethod marks entries flushed sequentially. UsingPromise.allcould improve throughput for the 25-entry batch, but sequential is safer for file operations and this is fire-and-forget anyway. -
Pre-existing:
findByDateincludes flushed files: The existingfindByDatemethod doesn't exclude.flushed.jsonfiles, which could affectfindByDateRangeresults. This is pre-existing behavior, not introduced by this PR.
Overall this is clean, well-tested code that follows the project's architectural patterns.
Summary
Closes #282
/ingestendpoint (default:https://telemetry.swamp.club)telemetryKeepFlushed: truein.swamp.yamlto rename to.flushed.jsoninsteadrepoIdUUID in.swamp.yamlonswamp init(lazy-migrated for existing repos), sent asdistinct_idon every eventtelemetryEndpointin.swamp.yaml--log-level debug)TelemetrySenderdomain port withHttpTelemetrySenderinfrastructure adapter (5s timeout)Test Plan
findUnflushed,markFlushed(delete + keep modes),HttpTelemetrySender, andflushTelemetryservice methodtelemetry-app-production.up.railway.app) — events arriving with correctdistinct_idand properties🤖 Generated with Claude Code