feat(guardrails): surface monitor-mode hits on usage events - #731
Conversation
enforcement_mode=monitor observations now ride the request's UsageEvent as guardrail_monitor_hits (would_block with reason / would_mask with per-detector counts; names only, never matched content) instead of tracing logs alone, closing the observe-then-enforce loop for AISIX-Cloud#562. - Guardrail trait: check_*_observed variants (defaults delegate; kinds untouched); SegmentsOutcome.monitor_hits - MonitorGuardrail emits the hits, probes the suppressed pii redactor for would-mask counts, and now delegates the segment pass so a monitored bedrock/lakera/presidio row is observed via one provider call with mask fidelity - chain folds keep hits collected before an enforcing peer's block - every proxy family threads a per-request accumulator into its usage emission (streaming on_complete paths included), mirroring the redacted_entity_counts plumbing
📝 WalkthroughWalkthroughThis PR introduces monitor-mode guardrail telemetry across the codebase. A new ChangesMonitor-mode guardrail telemetry
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyHandler
participant GuardrailChain
participant MonitorGuardrail
participant UsageEvent
Client->>ProxyHandler: request
ProxyHandler->>GuardrailChain: check_input_observed(req)
GuardrailChain->>MonitorGuardrail: check_input_observed(req)
MonitorGuardrail-->>GuardrailChain: (downgraded verdict, hits)
GuardrailChain-->>ProxyHandler: (verdict, monitor_hits)
ProxyHandler->>ProxyHandler: accumulate monitor_hits
ProxyHandler->>UsageEvent: emit_usage_event(guardrail_monitor_hits)
UsageEvent-->>Client: (response unaffected by monitor mode)
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/aisix-proxy/src/jobs.rs (1)
639-668: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMonitor hits are dropped on the guardrail-block / error path.
finishreceivesmonitor_hitsbut only forwards them viaemit_job_usage_eventon theOkbranch. Whenscan_input_blob/scan_output_blobreturnsProxyError::ContentFiltered(an enforcing guardrail blocked after a monitor-mode guardrail already fired), the accumulatedmonitor_hitsare discarded because theErrbranch callsemit_error_usage_event, which has no hits parameter.This diverges from the chat path, where the input-blocked/error telemetry explicitly carries
guardrail_monitor_hits: monitor_hits.clone(), and from the PR's stated goal of preserving collected hits even when a later enforcing guardrail blocks.monitor_hitsis already in scope here, so the gap is in the error emission path.#!/bin/bash # Confirm emit_error_usage_event has no monitor-hits parameter and compare with chat.rs parity. rg -nP -A20 'fn emit_error_usage_event' crates/aisix-proxy/src/usage_attr.rs rg -nP -B2 -A2 'guardrail_monitor_hits' crates/aisix-proxy/src/chat.rs | head -40🤖 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 `@crates/aisix-proxy/src/jobs.rs` around lines 639 - 668, The error path in finish drops monitor_hits when a guardrail-blocked request returns Err, because it only passes telemetry through emit_error_usage_event. Update the Err branch in jobs.rs so the accumulated monitor_hits are forwarded into the error usage emission, following the chat path’s guardrail_monitor_hits handling, and adjust emit_error_usage_event (and its call sites/signature in usage_attr.rs) to accept and record those hits.crates/aisix-proxy/src/passthrough.rs (1)
196-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate guardrail hits into the passthrough error event. The
Err(err)branch drops themonitor_hitsaccumulated bydispatch, soContentFilteredpassthrough failures lose guardrail telemetry; pass those hits intoemit_error_usage_eventinstead.🤖 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 `@crates/aisix-proxy/src/passthrough.rs` around lines 196 - 231, The Err(err) branch in passthrough::dispatch is dropping the accumulated monitor_hits, so guardrail-triggered passthrough failures are not reflected in the error usage event. Thread the monitor_hits value from dispatch into the error path and pass it through to crate::usage_attr::emit_error_usage_event alongside the existing status, err.kind(), and request metadata so ContentFiltered failures retain guardrail telemetry.
🧹 Nitpick comments (3)
crates/aisix-proxy/src/redact.rs (1)
1806-1858: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap:
moderate_body's monitor-hit accumulation is never exercised.
StubSegments::moderatealways returnsmonitor_hits: Vec::new()(line 1855), and everymoderate_bodycall in this test module passes a throwaway&mut Vec::new()(lines 1890, 1940, 1970, 1983, 2011) without inspecting it afterward. A regression that drops or double-appendsoutcome.monitor_hitsinmoderate_body(lines 218-222) would go undetected by this file's tests — the actual data-flow contract added by this PR is untested here.Consider extending
StubSegmentswith an optionalmonitor_hits: Vec<GuardrailMonitorHit>field returned frommoderate(), and adding an assertion inmoderate_body_masks_chat_slots_positionally(or a new dedicated test) thatmonitor_hits_outcontains the expected entries after the call.Also applies to: 1868-1988, 2006-2020
🤖 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 `@crates/aisix-proxy/src/redact.rs` around lines 1806 - 1858, The monitor-hit path in moderate_body is not covered because StubSegments::moderate always returns an empty monitor_hits vector and the tests pass throwaway output buffers without asserting on them. Extend StubSegments to optionally return predefined monitor_hits from moderate(), then update moderate_body_masks_chat_slots_positionally or add a dedicated test to assert that the caller-provided monitor_hits_out receives the expected GuardrailMonitorHit entries after moderate_body runs.crates/aisix-guardrails/src/chain.rs (2)
210-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect, but the four observed folds duplicate the four existing plain folds almost verbatim.
Logic is verified correct —
hits.extend(member_hits)runs before the match, so hits from members preceding (and including) a blocking member are preserved, matching the stated intent. This does double the amount of near-identical bypass/block-tracking boilerplate in the file (8 structurally-identical loops total now). A shared private helper parameterized over a per-member async check would cut the duplication, though the differingChatFormat/ChatResponsetypes make this a bit more than a trivial extraction in Rust's async-trait world.🤖 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 `@crates/aisix-guardrails/src/chain.rs` around lines 210 - 329, The four observed fold methods in Chain are correct but duplicate the existing plain fold logic almost verbatim, so refactor them to reduce repeated bypass/block tracking boilerplate. Extract a shared private helper in the Chain implementation that handles iterating over members, collecting hits, and preserving the first bypass reason, then have check_input_observed, check_output_observed, check_input_non_segment_observed, and check_output_non_segment_observed delegate to it with the appropriate per-member async check. Keep the existing behavior and identifiers like attribute_block, GuardrailVerdict, and GuardrailMonitorHit unchanged.
210-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing chain-level test for hit preservation across a later member's Block.
The doc comment states monitor hits from an earlier member must survive a later member's Block, but no test in this file exercises that multi-member scenario (only
build.rs's single-membermonitor_mode_observed_check_reports_hitstest exists). A test with one monitor-mode member (emits a hit) followed by an enforcing member that blocks would directly validate this chain's core new guarantee.🤖 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 `@crates/aisix-guardrails/src/chain.rs` around lines 210 - 241, The new observed-fold behavior in check_input_observed needs a chain-level regression test to prove earlier monitor hits survive a later Block. Add a test in chain.rs that builds a multi-member chain with one member returning monitor-mode hits and a later enforcing member returning GuardrailVerdict::Block, then assert the final verdict blocks and the collected Vec<GuardrailMonitorHit> still includes the earlier hit. Use the check_input_observed method and the existing GuardrailVerdict/GuardrailMonitorHit types to locate the right behavior.
🤖 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.
Outside diff comments:
In `@crates/aisix-proxy/src/jobs.rs`:
- Around line 639-668: The error path in finish drops monitor_hits when a
guardrail-blocked request returns Err, because it only passes telemetry through
emit_error_usage_event. Update the Err branch in jobs.rs so the accumulated
monitor_hits are forwarded into the error usage emission, following the chat
path’s guardrail_monitor_hits handling, and adjust emit_error_usage_event (and
its call sites/signature in usage_attr.rs) to accept and record those hits.
In `@crates/aisix-proxy/src/passthrough.rs`:
- Around line 196-231: The Err(err) branch in passthrough::dispatch is dropping
the accumulated monitor_hits, so guardrail-triggered passthrough failures are
not reflected in the error usage event. Thread the monitor_hits value from
dispatch into the error path and pass it through to
crate::usage_attr::emit_error_usage_event alongside the existing status,
err.kind(), and request metadata so ContentFiltered failures retain guardrail
telemetry.
---
Nitpick comments:
In `@crates/aisix-guardrails/src/chain.rs`:
- Around line 210-329: The four observed fold methods in Chain are correct but
duplicate the existing plain fold logic almost verbatim, so refactor them to
reduce repeated bypass/block tracking boilerplate. Extract a shared private
helper in the Chain implementation that handles iterating over members,
collecting hits, and preserving the first bypass reason, then have
check_input_observed, check_output_observed, check_input_non_segment_observed,
and check_output_non_segment_observed delegate to it with the appropriate
per-member async check. Keep the existing behavior and identifiers like
attribute_block, GuardrailVerdict, and GuardrailMonitorHit unchanged.
- Around line 210-241: The new observed-fold behavior in check_input_observed
needs a chain-level regression test to prove earlier monitor hits survive a
later Block. Add a test in chain.rs that builds a multi-member chain with one
member returning monitor-mode hits and a later enforcing member returning
GuardrailVerdict::Block, then assert the final verdict blocks and the collected
Vec<GuardrailMonitorHit> still includes the earlier hit. Use the
check_input_observed method and the existing
GuardrailVerdict/GuardrailMonitorHit types to locate the right behavior.
In `@crates/aisix-proxy/src/redact.rs`:
- Around line 1806-1858: The monitor-hit path in moderate_body is not covered
because StubSegments::moderate always returns an empty monitor_hits vector and
the tests pass throwaway output buffers without asserting on them. Extend
StubSegments to optionally return predefined monitor_hits from moderate(), then
update moderate_body_masks_chat_slots_positionally or add a dedicated test to
assert that the caller-provided monitor_hits_out receives the expected
GuardrailMonitorHit entries after moderate_body runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba10953f-7557-47e0-835b-ce6355a3fa8b
📒 Files selected for processing (25)
crates/aisix-core/src/lib.rscrates/aisix-core/src/models/guardrail.rscrates/aisix-core/src/models/mod.rscrates/aisix-guardrails/src/bedrock.rscrates/aisix-guardrails/src/build.rscrates/aisix-guardrails/src/chain.rscrates/aisix-guardrails/src/lakera.rscrates/aisix-guardrails/src/lib.rscrates/aisix-guardrails/src/presidio.rscrates/aisix-obs/src/usage.rscrates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/completions.rscrates/aisix-proxy/src/embeddings.rscrates/aisix-proxy/src/images.rscrates/aisix-proxy/src/jobs.rscrates/aisix-proxy/src/mcp.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/passthrough.rscrates/aisix-proxy/src/realtime.rscrates/aisix-proxy/src/redact.rscrates/aisix-proxy/src/rerank.rscrates/aisix-proxy/src/responses.rscrates/aisix-proxy/src/responses_bridge.rstests/e2e/src/cases/guardrail-monitor-telemetry-e2e.test.ts
1 participant
What
enforcement_mode: monitorobservations now land on the request's usage event instead of only in gateway logs. Each event carriesguardrail_monitor_hits: one entry per suppressed decision —would_block(with the operator-facing reason) orwould_mask(with per-detector counts) — plus the guardrail row name and hook. Names only, never matched content (#153).This closes the monitor-mode observability gap from api7/AISIX-Cloud#562: an operator can stage a policy in monitor mode, audit its would-have-fired rate per request in the dashboard, and only then flip it to
block. Previously monitor hits went to DP tracing logs only, so the "observe first, enforce later" loop had no dashboard-visible signal.How
aisix_core::models::GuardrailMonitorHit { guardrail_name, hook, action, reason, counts }, carried onUsageEvent.guardrail_monitor_hits(serde-skipped when empty; older CP images ignore the unknown field — additive wire change).Guardrailtrait gainscheck_{input,output}[_non_segment]_observedvariants returning(verdict, Vec<GuardrailMonitorHit>), with defaults that delegate to the plain checks — concrete kinds are untouched.SegmentsOutcomegainsmonitor_hits.MonitorGuardrailproduces the hits: a downgraded Block →would_block; the suppressed sync redactor (kind=pii) is probed on the hook's scan text forwould_maskcounts; and it now delegates the segment pass (moderates_segments), so a monitored bedrock/lakera/presidio row is observed through ONE provider call with mask/count fidelity instead of degrading to the blob path.GuardrailChainfolds observed results and keeps hits collected before an enforcing peer's Block short-circuit.redacted_entity_countsplumbing): chat (incl. streaming on_complete + ensemble judge), completions, messages (both stream builders), responses (+bridge), embeddings, rerank, images, audio, jobs, mcp, realtime, passthrough. Failed-attempt / pre-guardrail error events intentionally carry no hits (terminal event does), mirroring how redaction counts already behave.Tests
would_mask(with counts) andwould_block(reason, no value leak); enforcing rows produce no hits; a monitor hit survives an enforcing peer's block short-circuit; existing 208-test suite green.guardrail-monitor-telemetry-e2e): real binary + etcd + mock upstream + realaliyun_slsexporter against a mock SLS endpoint — a monitor-mode pii guardrail lets an email prompt reach the upstream unmasked while the exported usage event carries thewould_maskhit, a block-action detector returns 200 while the event carrieswould_block, and the matched values never appear in the exported events.Fixes api7/AISIX-Cloud#562 (link added post-merge for the Development field; the issue is closed manually once the full stage-0-2 scope lands). The CP ingestion + logs filter lands as the paired AISIX-Cloud PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes