Improve Prebid auction diagnostics#671
Conversation
SummaryRicher Blocking🔧 wrench
Non-blocking🤔 thinking
♻️ refactor
🌱 seedling
📌 out of scope
⛏ nitpick
CI Status
|
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The diagnostic improvements are well-scoped and the prior round's findings are cleanly addressed (current-context truncation, debug-gated body_preview, PREBID_INTEGRATION_ID constant, regression tests for the 500/1000-char caps and attached-detail suppression). CI is green and the new tests pin the right boundaries.
The remaining concerns are about consistency of that boundary: a few error paths slip past the debug gate that the 4xx path now establishes. Inline comments below cover the three specific call sites; cross-cutting items are listed here.
Blocking
🔧 wrench
- Parse-error message bypasses debug gate:
parse_responsereturns serde-error detail + body byte length unconditionally, while the 4xx branch hides body content behindconfig.debug(crates/trusted-server-core/src/integrations/prebid.rs:1141). warn!emits up to 1000 chars of upstream body without debug gate: previously trace-only; this is a logging behavior change worth gating in line with the metadata-path treatment (crates/trusted-server-core/src/integrations/prebid.rs:1112).- Launch-failure metadata exposes internal
messagestrings: e.g. signing-key kid viaRequestSigner::from_services().provider_error_messageis sound; the issue is at the call site choosing to emitcurrent_context()verbatim forerror_type=launch_failed(crates/trusted-server-core/src/auction/orchestrator.rs:385).
Non-blocking
🌱 seedling
- Network-layer
select()failures still drop providers fromprovider_responses(crates/trusted-server-core/src/auction/orchestrator.rs:481-486). The PR adds metadata forlaunch_failed,parse_response, andunsupported_response_body, but transport-level errors emerging fromselect()Err — and providers still pending after the deadline (backend_to_providerleftovers) — produce no entry./auctionclients can't distinguish "no bid" from "transport error" for those. Pre-existing, but the PR's stated intent is unified observability so it's a natural follow-up.
📌 out of scope
- JS bundle still defaults to bundling the Rubicon adapter (
crates/js/lib/build-all.mjs:32). Withclient_side_bidders = []now the default intrusted-server.toml, the bundled Rubicon Prebid.js adapter is dead weight unless a publisher overrides the list. Either defaultTSJS_PREBID_ADAPTERS=''to match, or havebuild-all.mjsderive the adapter list from the merged TOML. Worth a follow-up issue.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
- integration / browser tests: PASS
- CodeQL / format-typescript / format-docs: PASS
| let response_json: Json = serde_json::from_slice(&body_bytes).map_err(|e| { | ||
| Report::new(TrustedServerError::Prebid { | ||
| message: format!( | ||
| "Failed to parse Prebid response JSON (status {status}, {} bytes): {e}", |
There was a problem hiding this comment.
🔧 wrench — Parse-error path bypasses the debug-gate that the 4xx path enforces.
The non-success branch above carefully gates body_preview behind config.debug so untrusted upstream content doesn't reach /auction consumers. But on a 200 with malformed JSON, the error message itself is "Failed to parse Prebid response JSON (status {N}, {len} bytes): {serde_err}", which then flows through provider_error_message (capped at 500 chars but not debug-gated) into provider_details[].metadata.message. serde_json::Error::Display is normally just "expected value at line 1 column 1", so the realistic leak is the body byte length + parser category — but the asymmetry undermines the boundary the PR set up for the 4xx path, and serde messages can grow if upstream JSON gets exotic.
Suggested fix — keep the rich detail in the operator log, but use a static public message:
let response_json: Json = match serde_json::from_slice(&body_bytes) {
Ok(json) => json,
Err(e) => {
log::warn!(
"Prebid: failed to parse response JSON (status {status}, {} bytes): {e}",
body_bytes.len()
);
return Err(Report::new(TrustedServerError::Prebid {
message: "Failed to parse Prebid response JSON".to_string(),
}));
}
};That keeps full diagnostics for operators (via warn!) while ensuring /auction clients see a flat, non-leaky message regardless of debug mode.
| if body_preview.is_empty() { | ||
| log::warn!("Prebid returned non-success status: {status} with empty body"); | ||
| } else { | ||
| log::warn!("Prebid returned non-success status: {status} body: {body_preview}"); |
There was a problem hiding this comment.
🔧 wrench — warn! now emits up to 1000 chars of upstream body unconditionally.
Pre-PR, the body preview was logged at trace only. Now it lands in the standard warn path on every non-success Prebid response, regardless of config.debug. Upstream Prebid 4xx/5xx bodies routinely contain bidder-config echoes, internal request IDs, account hints, and occasionally PII fragments — same concern that motivated debug-gating body_preview in metadata, just shifted to logs.
The PR description acknowledges this ("warn log path still records the capped preview for operators regardless of debug") which is a defensible operator-visibility choice, but production logs are persisted, indexed, and often shared more broadly than /auction response bodies. They deserve at least the same gate.
Suggested fix — log the status at warn always, but gate the body behind debug:
if !status.is_success() {
let body_preview = prebid_body_preview(&body_bytes);
log::warn!("Prebid returned non-success status: {status} ({} bytes)", body_bytes.len());
if self.config.debug && !body_preview.is_empty() {
log::debug!("Prebid non-success body: {body_preview}");
}
// ...
}This preserves the operational signal (status + size) while keeping the upstream payload behind the same gate as the metadata path.
| provider.provider_name(), | ||
| e | ||
| ); | ||
| responses.push(provider_error_response( |
There was a problem hiding this comment.
🔧 wrench — Launch-failure messages can surface internal identifiers to /auction clients.
provider_error_message returns error.current_context().to_string() (capped at 500 chars). That's a clean fix for the prior format!("{:?}") issue — but it still exposes whatever string each provider chose to put into its message field, and those weren't designed as user-facing.
Concrete example: when request_bids fails because RequestSigner::from_services() couldn't load the active key, the error message is "Configuration error: failed to get signing key for kid: <kid>" (request_signing/signing.rs:140-148). With this change, that kid (an internal Secret Store key identifier) lands in the public provider_details[].metadata.message. Kids aren't strictly secret, but they're internal state that didn't previously surface — and the same pattern means anyone adding a new change_context(...) site upstream of request_bids is now shipping that detail to /auction callers by default.
Suggested fix — give error_type=launch_failed a static public message, and keep the rich context in the existing warn! log on line 380:
Err(e) => {
let response_time_ms = start_time.elapsed().as_millis() as u64;
log::warn!(
"Provider '{}' failed to launch request: {:?}",
provider.provider_name(),
e
);
let mut error_response = AuctionResponse::error(provider.provider_name(), response_time_ms)
.with_metadata("error_type", serde_json::json!("launch_failed"));
// Optionally surface error_type only here; full detail stays in logs.
responses.push(error_response);
}Or — if you want a per-error-type policy — split provider_error_response into a public path (static description per error_type) and an operator path (rich Report formatting), so future call sites can't accidentally widen the public surface.
2 participants
Summary
/auctionprovider metadata, with capped body previews included only when Prebid debug mode is enabled.204 No Contentor empty200) asnobidinstead of providererror.client_side_bidderssample config so it is not enabled by default.Changes
crates/trusted-server-core/src/integrations/prebid.rsdebug, treats empty successful responses as no-bid, usesPREBID_INTEGRATION_IDfor provider response IDs, and adds tests for truncation/non-UTF-8 preview behavior.crates/trusted-server-core/src/auction/orchestrator.rstrusted-server.tomlclient_side_biddersfrom["rubicon"]to[].Compatibility notes
provider_detailsmay include launch-failure diagnostics before awaited provider responses. Bid selection is order-insensitive, but consumers should not rely onprovider_detailsbeing ordered by response completion.Closes
N/A — no issue filed.
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test -p trusted-server-core prebid -- --nocapture;cargo test -p trusted-server-core auction -- --nocapture; targeted review-feedback tests forprovider_error,prebid_body_preview, andparse_response_non_successChecklist
unwrap()in production code — useexpect("should ...")logmacros per CLAUDE.md (template saystracing, but this repo standard islog)