api7 / api7/aisix

obs sink: byte-aware chunking for SLS PutLogs (blocks content capture)

Open
#529 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

cross-repo documentation enhancement P1 real chain test
Dominant language
Rust
Stars
157
Forks
32
Avg merge
1h 25m
Merged PRs (30d)
145

Description

Surfaced by the independent audit of #528 (finding M1).

Problem

The sink framework's byte-ceiling is advertised but enforced nowhere:

  • SinkCapabilities.max_batch_bytes is set by sinks but read by no code. SinkPipeline batches purely by record count (max_batch, default 100) + flush interval.
  • The docs contradict each other on who enforces it:
    • capabilities.rs (max_batch_bytes): "The pipeline never hands the sink a batch larger than this."
    • pipeline.rs module doc: "the sink owns chunking a batch down to its own per-request byte limit inside append_batch."
    • Reality: neither happens.

AliyunSlsSink::append_batch encodes all records into a single LogGroup → one PutLogs. SLS caps the request body (single LogGroup, low single-digit MB). On today's metadata-only path this is fine (≈100 records × ~0.5–2 KB ≪ limit). But the next milestone — full prompt/response content capture (SinkContent) — makes a 100-record batch easily exceed the limit, and SLS then returns 400 PostBodyInvalid, which classifies as SinkError::Permanent → the whole batch is dropped, never retried. A log sink silently dropping under load is exactly the failure the sink's error-classification is meant to avoid.

Fix (do this before content capture ships)

  1. Implement byte-aware chunking in AliyunSlsSink::append_batch (per the pipeline.rs design — the sink self-chunks, since only the sink knows the encoded size): pack records into multiple PutLogs requests, each kept under a conservative raw-size ceiling (~3 MB), always sending at least one record per request. Fast-path the common case (whole batch fits → one encode + one POST); only split when oversize.
  2. Reconcile the contradictory capability docs (capabilities.rs vs pipeline.rs) to a single source of truth: the sink self-limits.
  3. Add a unit test that drives a synthetic >ceiling batch (e.g. one large SinkContent) and asserts it fans out across multiple signed PutLogs requests.
  4. Restore the SLS capability to batch_unit: Both + max_batch_bytes: Some(...) once (1) lands.

Interim (in #528)

The SLS capability was downgraded to the honest current state — batch_unit: Records, max_batch_bytes: None (parity with OtlpSink) — so it no longer advertises an unenforced ceiling. This issue blocks the content-capture milestone.

Refs: #528, api7/AISIX-Cloud#687

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with AliyunSlsSink::append_batch and the SinkPipeline, then read capabilities.rs and the pipeline.rs module documentation to resolve the chunking contract. Add a synthetic oversized SinkContent unit test that verifies multiple signed PutLogs requests, and confirm the SLS capability advertises batch_unit: Both with a byte ceiling once self-chunking is implemented.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
observability
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.