fix(telemetry): stop clipping the per-attempt error message at 256 chars#782
Merged
Conversation
The `error_message` an operator reads on the /logs page is truncated before it is ever stored: `attempt_error_message` caps at 256 chars, a bound copied from `sanitize_tag` — which exists to bound short provider tags (`branded_provider`, `pk_label`), not error text. An `UpstreamStatus` message is already bounded by the bridge at `MAX_UPSTREAM_ERROR_MESSAGE_BYTES` (1 KiB), so the 256-char cap was a second, tighter truncation applied on top of a bound that had already made the string safe to store. Real upstream errors exceed it: Azure's content-management-policy message is ~260 chars on its own, so what reached telemetry ended mid-URL at "...read our documentation: https://go.m" — dropping exactly the part that says what to do about it. Raise the cap to 2048 chars, above the bridge's 1 KiB byte budget, so the bridge's bound is the only one that fires and the operator sees the whole message the bridge kept. The cap stays as a backstop for variants carrying an unbounded string (e.g. `Config`). Fixes api7/AISIX-Cloud#1065
📝 WalkthroughWalkthroughPer-attempt error telemetry now permits up to 2048 characters after control-character removal. Tests cover upstream messages, bridge-bounded messages, unbounded errors, and newline or tab sanitization. ChangesAttempt error telemetry
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/attempt.rs`:
- Around line 203-225: Update the long-message regression test and its comment
to use provider-neutral wording and a generated message longer than 256
characters, while preserving the assertions that the full body and tail remain
intact. Rename long_upstream_message_is_not_clipped if needed to remove
provider-specific terminology, and eliminate the Azure/product names and
documentation URL from the fixture.
🪄 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: b64cbc72-e5e5-439d-a6b3-6482e54231f6
📒 Files selected for processing (1)
crates/aisix-proxy/src/attempt.rs
The regression fixture quoted an upstream vendor's refusal prose verbatim, doc URL and all, inside a provider-neutral crate. Only its LENGTH was load-bearing — the brand was decorative, which is what AGENTS.md §7 rules out for shipped artifacts. Keep the shape that makes the fixture worth having (prose ending in a URL, so what must survive is the END of a realistically long message) and write it generically. Still fails on the old 256-char cap.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
The
error_messagean operator reads on the /logs page is truncated before it is ever stored.attempt_error_messagecaps the string at 256 chars — a bound copied fromsanitize_tag, which exists to bound short provider tags (branded_provider,pk_label), not error text.That cap sits on top of a bound that had already made the string safe to store: an
UpstreamStatusmessage is bounded by the bridge atMAX_UPSTREAM_ERROR_MESSAGE_BYTES(1 KiB) incapture_upstream_error_http. So the 256-char cap was a second, tighter truncation with no budget of its own to defend.Real upstream errors exceed it. A content-filter refusal — the shape that provoked the issue — runs ~260 chars on its own, so what reached telemetry ended mid-URL:
— dropping exactly the part that tells the operator what to do about it.
Implementation
Raise the cap to 2048 chars (
MAX_ATTEMPT_ERROR_MESSAGE_CHARS), above the bridge's 1 KiB byte budget, so the bridge's bound is the only limit that fires and the operator sees the whole message the bridge kept. Chars-vs-bytes matters here: a 1 KiB ASCII message is 1024 chars, so a 1024-char cap would still have clipped the prefixupstream returned HTTP 400:worth of tail.The cap stays as a backstop —
BridgeErrorvariants that carry an unbounded string (e.g.Config) still can't write unbounded telemetry.Baseline. Mainstream gateways bound this field an order of magnitude looser than we did: the closest comparable applies a 2048-char DB-storage cap to the stored error message (env-tunable), and truncates prompts on the same budget. 2048 matches that. None treat a short-tag-sized bound as adequate for error text.
Behavior change
Failed-attempt
error_messagetelemetry can now carry up to ~1 KiB (whatever the bridge kept) instead of 256 chars. Only failed attempts populate the field, and cp-api stores it in atextcolumn with no cap of its own; per-orgusage_retention_dayspruning is unchanged.Tests
crates/aisix-proxy/src/attempt.rs— the two regression tests fail on the old cap and pass on the new one; verified by reverting the constant locally:long_upstream_message_is_not_clipped— a long refusal survives to its trailing doc URL. The fixture is prose ending in a URL rather than generated filler, because what has to survive is the END of a realistically long message.cap_clears_the_bridge_message_bound— a message already at the bridge's byte bound reaches telemetry whole.pathological_message_still_hits_the_backstop— an unboundedConfigstring is still capped.control_chars_are_stripped— unchanged sanitisation.The paired dashboard fix (the detail panel renders this field with a
truncateclass, clipping it to one line regardless of length) ships separately in api7/AISIX-Cloud#1068.Fixes api7/AISIX-Cloud#1065