Skip to content

fix: label market-data receive boundaries - #446

Merged
proerror77 merged 4 commits into
mainfrom
codex/receive-boundary-396
Jul 29, 2026
Merged

proerror77 merged 4 commits into
mainfrom
codex/receive-boundary-396

Conversation

@proerror77

@proerror77 proerror77 commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Change contract

Make every CEX market-data receive timestamp identify its actual capture boundary, and exclude adapter-publish and REST-snapshot-completion anchors from receive-latency cohorts.

Out of scope

Dependency / merge order

Stacked on PR #443. Merge #443 first, then this PR. This branch is based on exact #443 head 755ab9f.

Focused validation

  • cargo check --workspace --all-targets
  • cargo test for hft-core, hft-integration, hft-ports, hft-engine, Binance/Bybit/Bitget data adapters, hft-infra-metrics, hft-data, and hft-binance-md
  • cargo test -p hft-collector --bin binance-lob-archiver (63 passed)
  • receive_latency_cohort_excludes_non_receive_boundaries
  • non_receive_boundary_clears_prior_receive_latency_anchor
  • Two independent fixed-point reviews: no actionable P0-P3 findings

Workspace all-target check passes with only pre-existing unrelated warnings in Hyperliquid and runtime modules.

Rollout / rollback impact

No deployment in this PR; live remains disabled. Rollout changes metric taxonomy and cohort admission only. Roll back this PR as one unit after #443 if consumers require the old labels.

Closes #396

Summary by CodeRabbit

  • Improvements

    • Improved WebSocket latency measurement by capturing delivery timing when complete messages arrive.
    • More accurately separates WebSocket delivery, adapter publishing, and REST snapshot completion in latency reporting.
    • Refined end-to-end latency calculations to exclude events that are not part of receive-latency cohorts.
    • Updated Binance, Bitget, and Bybit market-data processing to use consistent message-level metrics.
  • Documentation

    • Clarified timestamp and latency metric descriptions, including their measurement boundaries.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change replaces frame-oriented WebSocket metrics with complete-message delivery metrics, adds explicit latency capture boundaries, gates receive-latency cohorts in the engine, and propagates timestamps through Binance snapshots, live parsing, collectors, and latency documentation.

Changes

WebSocket message timing

Layer / File(s) Summary
Message metrics and adapter interfaces
rust_hft/market-core/integration/..., rust_hft/data-pipelines/adapters/...
WsFrameMetrics is renamed to WsMessageMetrics, and WebSocket clients, handlers, and adapters propagate complete-message delivery timing.
Capture boundaries and engine cohorts
rust_hft/market-core/core/src/latency.rs, rust_hft/market-core/ports/src/events.rs, rust_hft/market-core/engine/...
LatencyCaptureBoundary distinguishes userspace delivery, adapter publish, and snapshot completion; engine ingestion and end-to-end samples are restricted to receive-latency cohorts.
Binance timestamp propagation and provenance
rust_hft/apps/binance-md/..., rust_hft/data-pipelines/adapters/adapter-binance/..., rust_hft/tools/collector/..., rust_hft/data-pipelines/core/binance_market_tape.rs
Live and snapshot paths preserve boundary timestamps, latency helpers identify userspace WebSocket delivery, and related timestamp and metric descriptions are updated.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WsClient
  participant BinanceAdapter
  participant EventIngester
  participant Engine
  WsClient->>BinanceAdapter: deliver message with WsMessageMetrics
  BinanceAdapter->>BinanceAdapter: create userspace delivery LatencyTracker
  BinanceAdapter->>EventIngester: publish tracked market event
  EventIngester->>Engine: attach capture boundary
  Engine->>Engine: record receive-cohort latency
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and matches the main change: receive-boundary labeling for market data.
Description check ✅ Passed The description covers the change contract, out-of-scope items, merge order, validation, and rollout notes.
Linked Issues check ✅ Passed The changes satisfy #396 by renaming receive provenance, separating publish/snapshot boundaries, and adding a focused cohort test.
Out of Scope Changes check ✅ Passed No clear out-of-scope code changes are present beyond the stated timestamp provenance and cohort-admission updates.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/receive-boundary-396

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5c24cc7cf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rust_hft/data-pipelines/adapters/adapter-binance/src/lib.rs Outdated
Comment thread rust_hft/data-pipelines/core/src/binance_market_tape.rs Outdated
Base automatically changed from codex/timestamp-provenance-395 to main July 29, 2026 11:58
Sonic Shih added 3 commits July 29, 2026 20:44
…y-396

# Conflicts:
#	rust_hft/data-pipelines/adapters/adapter-binance/src/lib.rs
#	rust_hft/market-core/engine/src/dataflow/ingestion_ultra.rs
…y-396

# Conflicts:
#	rust_hft/market-core/engine/src/lib.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
rust_hft/market-core/engine/src/lib.rs (1)

1450-1462: 🎯 Functional Correctness | 🔵 Trivial

Mixed-boundary adapters can starve the Fill end-to-end DoD latency metric.

recent_market_event_timestamp is now None whenever the most-recently strategy-processed pending event's capture_boundary isn't a receive-latency cohort (e.g. Bitget, still tagged AdapterPublish). In a deployment mixing Bitget with Binance/Bybit, this anchor can be frequently cleared by Bitget events interleaved via the round-robin consumer loop, making the handle_execution_event Fill "端到端DoD延遲" (infra_metrics::MetricsRegistry::global().record_end_to_end_latency) sparse or absent even when cohort-tagged market data is flowing normally. This is the intended tradeoff of excluding non-receive boundaries from the cohort, but worth confirming it matches the operational expectations for multi-venue deployments (dashboards/alerts relying on this specific gauge may need to account for reduced sample density).

🤖 Prompt for 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.

In `@rust_hft/market-core/engine/src/lib.rs` around lines 1450 - 1462, Update
run_strategies_sync and the receive_latency_anchor handling so
non-receive-latency events such as AdapterPublish do not clear an existing
recent_market_event_timestamp; only replace the anchor when the pending event
belongs to a receive-latency cohort. Preserve the existing exclusion of
non-cohort events from latency measurement while keeping the latest valid cohort
anchor available for handle_execution_event and record_end_to_end_latency.
🤖 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.

Nitpick comments:
In `@rust_hft/market-core/engine/src/lib.rs`:
- Around line 1450-1462: Update run_strategies_sync and the
receive_latency_anchor handling so non-receive-latency events such as
AdapterPublish do not clear an existing recent_market_event_timestamp; only
replace the anchor when the pending event belongs to a receive-latency cohort.
Preserve the existing exclusion of non-cohort events from latency measurement
while keeping the latest valid cohort anchor available for
handle_execution_event and record_end_to_end_latency.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3984bdd1-de8e-4b44-936e-940059b02bba

📥 Commits

Reviewing files that changed from the base of the PR and between 3aae5dd and badb2cc.

📒 Files selected for processing (24)
  • rust_hft/apps/binance-md/src/main.rs
  • rust_hft/data-pipelines/adapters/adapter-binance/src/lib.rs
  • rust_hft/data-pipelines/adapters/adapter-binance/src/websocket.rs
  • rust_hft/data-pipelines/adapters/adapter-bitget/src/bitget_stream.rs
  • rust_hft/data-pipelines/adapters/adapter-bitget/src/zero_copy_stream.rs
  • rust_hft/data-pipelines/adapters/adapter-bybit/src/lib.rs
  • rust_hft/data-pipelines/core/src/binance_market_tape.rs
  • rust_hft/infra-services/core/metrics/src/lib.rs
  • rust_hft/market-core/core/src/latency.rs
  • rust_hft/market-core/core/src/lib.rs
  • rust_hft/market-core/core/src/unified_timestamp.rs
  • rust_hft/market-core/engine/src/binance_md/latency.rs
  • rust_hft/market-core/engine/src/binance_md/parser.rs
  • rust_hft/market-core/engine/src/binance_md/replay.rs
  • rust_hft/market-core/engine/src/dataflow/ingestion.rs
  • rust_hft/market-core/engine/src/dataflow/ingestion_ultra.rs
  • rust_hft/market-core/engine/src/lib.rs
  • rust_hft/market-core/engine/tests/live_hotpath_contract.rs
  • rust_hft/market-core/integration/src/latency.rs
  • rust_hft/market-core/integration/src/lib.rs
  • rust_hft/market-core/integration/src/ws.rs
  • rust_hft/market-core/ports/src/events.rs
  • rust_hft/market-core/ports/src/traits.rs
  • rust_hft/tools/collector/src/bin/binance-lob-archiver.rs

@proerror77
proerror77 merged commit a2e3039 into main Jul 29, 2026
39 checks passed
@proerror77
proerror77 deleted the codex/receive-boundary-396 branch July 29, 2026 13:38
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.

CEX market data: correct WebSocket receive-boundary provenance

1 participant