Uh oh!
There was an error while loading. Please reload this page.
feat(obs): content capture for embeddings/rerank/images/audio (LiteLLM parity) - #708
Merged
Conversation
…M parity) Completes the AISIX-Cloud#947 family: the four remaining handlers fanned observability events out with content=None, so content_mode=full exporters received metadata only for their traffic. Scope follows LiteLLM's standard logging payload: - embeddings — prompt = post-redaction request JSON; response = the full embeddings JSON, vectors included (LiteLLM logs EmbeddingResponse verbatim), truncated at the capture cap - rerank — prompt = post-redaction request JSON (query + documents); response = the relayed rerank JSON verbatim - images — prompt = post-redaction request JSON; response = the image JSON (url or b64_json; LiteLLM does not strip response b64 either) - audio speech — prompt = post-redaction request JSON (the input text); the binary audio response is not captured (LiteLLM logs no TTS response) - audio transcription/translation — the audio bytes are never captured: the file field is represented by its sha256 (exactly LiteLLM's file_checksum convention) alongside the verbatim text form fields (post-redaction prompt included); response = the post-redaction transcript Blocked (422) responses capture nothing, matching the chat surface. Fixes#700
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds optional content capture to audio, embeddings, images, and rerank proxy handlers, and forwards captured request/response content into telemetry fan-out instead of always emitting ChangesContent Capture Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AudioHandler
participant Upstream
participant Telemetry
Client->>AudioHandler: transcription, translation, or speech request
AudioHandler->>AudioHandler: redact input and compute content_capture_cap
AudioHandler->>AudioHandler: build captured_prompt from multipart or JSON body
AudioHandler->>Upstream: forward request
Upstream-->>AudioHandler: transcript, translation, or speech response
AudioHandler->>AudioHandler: build captured_content and attach to success
AudioHandler->>Telemetry: emit_usage_event(captured_content)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/e2e/src/cases/sls-content-capture-nontext-e2e.test.ts (1)
266-274: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winResponse-side markers missing from metadata_only negative check.
EMBED_RESPONSE_MARK,RERANK_RESPONSE_MARK, andIMAGES_RESPONSE_URLare asserted present inFULL_LOGSTORE(Lines 206, 220, 232) but are not included in this negative-token list, so the test doesn't verify that response content is excluded from the metadata-only export.🧪 Proposed addition to the negative-token list
for (const token of [ EMBED_PROMPT_TOKEN, + String(EMBED_RESPONSE_MARK), RERANK_PROMPT_TOKEN, RERANK_DOC_TOKEN, + String(RERANK_RESPONSE_MARK), IMAGES_PROMPT_TOKEN, + IMAGES_RESPONSE_URL, SPEECH_PROMPT_TOKEN, TRANSCRIPT_PROMPT_TOKEN, TRANSCRIPT_RESPONSE_TOKEN, ]) {🤖 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 `@tests/e2e/src/cases/sls-content-capture-nontext-e2e.test.ts` around lines 266 - 274, The metadata_only negative-token check in the nontext E2E test is missing response-side markers, so it does not fully verify response content is excluded from the export. Update the token loop in the `sls-content-capture-nontext-e2e` test to include `EMBED_RESPONSE_MARK`, `RERANK_RESPONSE_MARK`, and `IMAGES_RESPONSE_URL` alongside the existing prompt/transcript tokens. Keep the change within the existing negative assertion block so the metadata-only case covers both request and response markers.
🤖 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.
Inline comments:
In `@crates/aisix-proxy/src/audio.rs`:
- Around line 516-518: The full-content export path in audio.rs is leaking
user-controlled upload filenames via the obj.insert call that writes
{name}_filename. Update the logic around the file_name handling so filenames are
either omitted or passed through the same PII redaction flow used elsewhere,
while still keeping the SHA-256-only representation of the file payload. Use the
existing audio export and redaction helpers in this code path to locate the
change.
- Around line 503-521: The captured_prompt builder is losing repeated multipart
fields because serde_json::Map::insert overwrites earlier entries, so preserve
all values instead of collapsing by name. Update the logic inside the
captured_prompt mapping in audio.rs to accumulate repeated fields per key in the
captured content structure (including prompt and other duplicated names), while
keeping the existing sha256/file-name handling for non-text or file fields. Use
the captured_prompt block and the fields iteration as the main place to fix
this.
In `@tests/e2e/src/cases/sls-content-capture-nontext-e2e.test.ts`:
- Around line 264-276: The negative content checks on the metadata-only logstore
are racing the async export, so the assertion can pass before the event arrives.
In the sls-content-capture-nontext e2e test, update the META_LOGSTORE
verification to wait for a benign always-present marker from the same request,
using the same request flow that already waits on FULL_LOGSTORE, before checking
that EMBED_PROMPT_TOKEN, RERANK_PROMPT_TOKEN, RERANK_DOC_TOKEN,
IMAGES_PROMPT_TOKEN, SPEECH_PROMPT_TOKEN, TRANSCRIPT_PROMPT_TOKEN, and
TRANSCRIPT_RESPONSE_TOKEN are absent. This makes the assertion in the test case
around decodedTextFor and waitForToken order-independent and reliable.
---
Nitpick comments:
In `@tests/e2e/src/cases/sls-content-capture-nontext-e2e.test.ts`:
- Around line 266-274: The metadata_only negative-token check in the nontext E2E
test is missing response-side markers, so it does not fully verify response
content is excluded from the export. Update the token loop in the
`sls-content-capture-nontext-e2e` test to include `EMBED_RESPONSE_MARK`,
`RERANK_RESPONSE_MARK`, and `IMAGES_RESPONSE_URL` alongside the existing
prompt/transcript tokens. Keep the change within the existing negative assertion
block so the metadata-only case covers both request and response markers.
🪄 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: 7db54474-bca2-477f-a94c-bbe980f17f21
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/aisix-proxy/Cargo.tomlcrates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/embeddings.rscrates/aisix-proxy/src/images.rscrates/aisix-proxy/src/rerank.rstests/e2e/src/cases/sls-content-capture-nontext-e2e.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…e-race meta assertion CodeRabbit on #708: (1) serde_json::Map::insert overwrote repeated multipart field values in the captured prompt while all repeats are forwarded — append with a newline instead; (2) the upload filename is user-controlled text that skips the redaction path — drop it from the capture, the sha256 alone represents the file (LiteLLM parity anyway); (3) the e2e meta-logstore negative assertions now wait for the last request's metadata to land first, so they can't pass trivially against a lagging store.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
Audit finding #700 (parent: api7/AISIX-Cloud#950), completing the AISIX-Cloud#947 family: embeddings / rerank / images / audio fanned observability events out with
content: None, so acontent_mode = fullexporter received metadata only for their traffic.Scope decision
Per the user's call on #700, the capture scope follows LiteLLM's standard logging payload (verified against LiteLLM HEAD 88e03e54):
EmbeddingResponseverbatim), truncated atcontent_max_bytesurlorb64_json— LiteLLM does not strip response b64 either)inputtext)filefield is represented by its sha256 (LiteLLM'sfile_checksumconvention) alongside the verbatim text form fields (post-redactionpromptincluded)Blocked (422) responses capture nothing, matching the chat surface. All captures are post-#932-masking (builds on #703), so masked PII stays masked in exported content.
Tests
New DP E2E
sls-content-capture-nontext-e2e.test.ts(real binary + etcd + mock SLS): all five scenarios assert thefulllogstore carries the planted request tokens AND the response content (vector value / rerank score / image URL / transcript + file sha256, never the raw audio bytes), whilemetadata_onlyreceives none. Verified fail-before/pass-after (crate changes stashed → test fails).Fixes#700
Summary by CodeRabbit