Skip to content

feat(capture): implement v1 BatchResponse and sink result merging#59757

Merged
eli-r-ph merged 3 commits into
masterfrom
eli.r/capture-v1-response-1
May 26, 2026
Merged

feat(capture): implement v1 BatchResponse and sink result merging#59757
eli-r-ph merged 3 commits into
masterfrom
eli.r/capture-v1-response-1

Conversation

@eli-r-ph
Copy link
Copy Markdown
Contributor

@eli-r-ph eli-r-ph commented May 23, 2026

Problem

The V1 capture analytics API needs to return structured per-event responses so SDKs know exactly which events succeeded, were dropped, rate-limited, or should be retried.

Changes

Implements the 200 OK response path for POST /i/v1/general/events:

  • response.rsBatchResponse and BatchEntryStatus types with IntoResponse impl; per-event Ok/Drop/Limited/Retry statuses keyed by UUID with optional cause and detail fields
  • process.rsmerge_sink_results correlates publish outcomes from the sink back into WrappedEvent metadata; process_batch now calls the v1 sink router, merges results, and builds the typed response
  • handler.rs — return type updated to Result<Response, Error>
  • router.rsv1_sink_router: Option<Arc<...>> field added to app state (not yet wired in setup.rs — gated behind CAPTURE_V1_SINKS env var)
  • test_utils.rsMockSinkResult and fixture helpers for response/merge unit tests
  • types.rs — partition key parity tests, serialize_into edge case tests

Inline unit and integration tests cover: response serialization (JSON field ordering, optional fields), merge logic (all outcome variants, partial failures, multi-destination), and the ServiceUnavailable guard when no sink router is configured.

How did you test this code?

Locally and in CI

This is PR 1 of a 3-PR stack:

  1. This PR — core feature (response types, merge logic, production wiring)
  2. chore(capture): move static response headers to v1 middleware #59967 — move static response headers to middleware (~10 lines)
  3. chore(capture): expand v1 test coverage with pipeline and e2e suites #59968 — expanded test coverage with pipeline integration and Docker e2e suites (~1200 lines)

Publish to changelog?

N/A

🤖 Agent context

Eli planned, Claude coded, Eli reviewed

@eli-r-ph eli-r-ph self-assigned this May 23, 2026
@eli-r-ph eli-r-ph force-pushed the eli.r/capture-v1-response-1 branch 3 times, most recently from e6e3745 to 5fac4c4 Compare May 25, 2026 21:48
@eli-r-ph eli-r-ph changed the title feat(capture): implement v1 response handling feat(capture): implement v1 BatchResponse and sink result merging May 25, 2026
@eli-r-ph eli-r-ph requested a review from a team May 25, 2026 21:57
@eli-r-ph eli-r-ph marked this pull request as ready for review May 25, 2026 21:57
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 25, 2026

Comments Outside Diff (2)

  1. rust/capture/src/v1/analytics/process.rs, line 398-432 (link)

    P2 Duplicated event_refs building pattern

    The block that collects event_refs from events is copy-pasted verbatim in four integration tests (integration_happy_path_all_ok, integration_mixed_pre_drop_and_publish, integration_all_events_pre_dropped_empty_publish, integration_overflow_destination_published_ok) and is structurally identical to the same block in production process_batch. This violates OnceAndOnlyOnce — a helper like fn publishable_refs(events: &[WrappedEvent]) -> Vec<&(dyn SinkEvent + Send + Sync)> in test_utils.rs would remove all four duplicates.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: rust/capture/src/v1/analytics/process.rs
    Line: 398-432
    
    Comment:
    **Duplicated `event_refs` building pattern**
    
    The block that collects `event_refs` from `events` is copy-pasted verbatim in four integration tests (`integration_happy_path_all_ok`, `integration_mixed_pre_drop_and_publish`, `integration_all_events_pre_dropped_empty_publish`, `integration_overflow_destination_published_ok`) and is structurally identical to the same block in production `process_batch`. This violates OnceAndOnlyOnce — a helper like `fn publishable_refs(events: &[WrappedEvent]) -> Vec<&(dyn SinkEvent + Send + Sync)>` in `test_utils.rs` would remove all four duplicates.
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. rust/capture/src/v1/analytics/process.rs, line 181-233 (link)

    P2 Opportunity to parameterize single-outcome merge tests

    The four tests merge_all_success_leaves_results_intact, merge_retriable_error_flips_ok_to_retry, merge_timeout_flips_ok_to_retry, and merge_fatal_error_flips_ok_to_drop are structurally identical: one event + one sink result → assert event.result and event.details. Per the project's preference for parameterised tests, these could be collapsed into a single table-driven test (e.g. using an array of (mock_result_fn, expected_result, expected_details) tuples), keeping each case as a distinct row rather than four separate functions.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: rust/capture/src/v1/analytics/process.rs
    Line: 181-233
    
    Comment:
    **Opportunity to parameterize single-outcome merge tests**
    
    The four tests `merge_all_success_leaves_results_intact`, `merge_retriable_error_flips_ok_to_retry`, `merge_timeout_flips_ok_to_retry`, and `merge_fatal_error_flips_ok_to_drop` are structurally identical: one event + one sink result → assert `event.result` and `event.details`. Per the project's preference for parameterised tests, these could be collapsed into a single table-driven test (e.g. using an array of `(mock_result_fn, expected_result, expected_details)` tuples), keeping each case as a distinct row rather than four separate functions.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
rust/capture/src/v1/analytics/process.rs:398-432
**Duplicated `event_refs` building pattern**

The block that collects `event_refs` from `events` is copy-pasted verbatim in four integration tests (`integration_happy_path_all_ok`, `integration_mixed_pre_drop_and_publish`, `integration_all_events_pre_dropped_empty_publish`, `integration_overflow_destination_published_ok`) and is structurally identical to the same block in production `process_batch`. This violates OnceAndOnlyOnce — a helper like `fn publishable_refs(events: &[WrappedEvent]) -> Vec<&(dyn SinkEvent + Send + Sync)>` in `test_utils.rs` would remove all four duplicates.

### Issue 2 of 2
rust/capture/src/v1/analytics/process.rs:181-233
**Opportunity to parameterize single-outcome merge tests**

The four tests `merge_all_success_leaves_results_intact`, `merge_retriable_error_flips_ok_to_retry`, `merge_timeout_flips_ok_to_retry`, and `merge_fatal_error_flips_ok_to_drop` are structurally identical: one event + one sink result → assert `event.result` and `event.details`. Per the project's preference for parameterised tests, these could be collapsed into a single table-driven test (e.g. using an array of `(mock_result_fn, expected_result, expected_details)` tuples), keeping each case as a distinct row rather than four separate functions.

Reviews (1): Last reviewed commit: "feat(capture): implement v1 BatchRespons..." | Re-trigger Greptile

Comment thread rust/capture/src/v1/analytics/process.rs Outdated
Comment thread rust/capture/src/v1/analytics/response.rs
Copy link
Copy Markdown
Contributor

@jose-sequeira jose-sequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just small comments

eli-r-ph added 3 commits May 26, 2026 14:55
Implement the 200 OK response path for the V1 capture analytics API:

- Add BatchResponse and BatchEntryStatus types with per-event
  Ok/Drop/Limited/Retry statuses keyed by UUID
- Implement merge_sink_results to correlate publish outcomes back to
  WrappedEvent metadata after Kafka produce
- Wire process_batch to call the v1 sink router, merge results, and
  build the typed response
- Add v1_sink_router field to router::State (Option, not yet wired in
  production setup.rs)
- Include inline unit and integration tests for response serialization,
  merge logic, and sink result correlation
@eli-r-ph eli-r-ph force-pushed the eli.r/capture-v1-response-1 branch from f4b7219 to fc606d6 Compare May 26, 2026 21:55
@eli-r-ph eli-r-ph merged commit a49854d into master May 26, 2026
167 checks passed
@eli-r-ph eli-r-ph deleted the eli.r/capture-v1-response-1 branch May 26, 2026 22:12
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented May 26, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-05-26 22:46 UTC Run
prod-us ✅ Deployed 2026-05-26 22:56 UTC Run
prod-eu ✅ Deployed 2026-05-26 23:00 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants