-
Notifications
You must be signed in to change notification settings - Fork 12
Pass user agent to auction telemetry #1103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| //! Auction telemetry row construction and sink abstraction. | ||
| //! | ||
| //! Core owns the privacy-preserving auction observation model and pure row | ||
| //! builder. Platform adapters provide the concrete sink implementation. | ||
| //! Core owns the auction observation model and pure row builder. Platform | ||
| //! adapters provide the concrete sink implementation. | ||
|
|
||
| use std::collections::HashSet; | ||
| use std::time::Instant; | ||
|
|
@@ -19,6 +19,9 @@ use crate::platform::RuntimeServices; | |
|
|
||
| const MAX_PAGE_PATH_BYTES: usize = 256; | ||
| const DYNAMIC_SEGMENT_REPLACEMENT: &str = ":id"; | ||
| #[cfg(test)] | ||
| const TEST_USER_AGENT: &str = | ||
| "FictionalBrowser/123.4 (FictionalOS 10.2; FictionalDevice) ExampleRenderer/567.8"; | ||
|
|
||
| /// Source path that initiated an auction candidate. | ||
| #[derive(Debug, Clone, Copy, Eq, PartialEq)] | ||
|
|
@@ -91,7 +94,7 @@ impl AbandonedProviderCall { | |
| } | ||
| } | ||
|
|
||
| /// Privacy-preserving context shared by all rows in one auction observation. | ||
| /// Context shared by all rows in one auction observation. | ||
| #[derive(Debug, Clone)] | ||
| pub struct AuctionObservationContext { | ||
| /// Fresh telemetry UUID, independent of EC and internal auction IDs. | ||
|
|
@@ -110,6 +113,8 @@ pub struct AuctionObservationContext { | |
| pub is_mobile: u8, | ||
| /// `0` = bot, `1` = browser, `2` = unknown. | ||
| pub is_known_browser: u8, | ||
| /// Complete User-Agent supplied by the auction request, when present. | ||
| pub user_agent: Option<String>, | ||
| /// Whether GDPR applies. | ||
| pub gdpr_applies: bool, | ||
| /// Whether any consent signal was present. | ||
|
|
@@ -134,11 +139,16 @@ impl AuctionObservationContext { | |
| .and_then(|page_url| url::Url::parse(page_url).ok()) | ||
| .map(|url| url.path().to_owned()) | ||
| .unwrap_or_else(|| "/".to_owned()); | ||
| let user_agent = request | ||
| .device | ||
| .as_ref() | ||
| .and_then(|device| device.user_agent.as_deref()); | ||
| Self::from_parts( | ||
| auction_source, | ||
| &request.publisher.domain, | ||
| &raw_path, | ||
| request.slots.len(), | ||
| user_agent, | ||
| ec_context, | ||
| ) | ||
| } | ||
|
|
@@ -150,6 +160,7 @@ impl AuctionObservationContext { | |
| publisher_domain: &str, | ||
| raw_page_path: &str, | ||
| slot_count: usize, | ||
| user_agent: Option<&str>, | ||
| ec_context: &EcContext, | ||
| ) -> Self { | ||
| let device = ec_context.device_signals(); | ||
|
|
@@ -172,6 +183,7 @@ impl AuctionObservationContext { | |
| Some(false) => 0, | ||
| None => 2, | ||
| }, | ||
| user_agent: user_agent.map(str::to_owned), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 wrench — Every other request-derived value in this module is bounded before it reaches a row: The failure mode isn't just storage bloat: the UA is duplicated into every row of the batch (summary + one per provider call + one per bid, up to Proposed fix (apply manually — it spans two hunks in the same file, and neither half compiles/lints alone, so it can't be a one-click suggestion): // Near the existing consts at the top of telemetry.rs:
const MAX_USER_AGENT_BYTES: usize = 1024;
// In AuctionObservationContext::from_parts, replacing this line:
user_agent: user_agent.map(|value| {
let mut bounded = value.to_owned();
truncate_to_char_boundary(&mut bounded, MAX_USER_AGENT_BYTES);
bounded
}),1024 bytes keeps every legitimate UA intact (real-world UAs, including frozen Client-Hints-era strings, sit well under 400 bytes) while bounding the adversarial case, and it reuses the module's existing |
||
| gdpr_applies: consent.gdpr_applies, | ||
| consent_present: !consent.is_empty(), | ||
| slot_count, | ||
|
|
@@ -196,6 +208,7 @@ impl AuctionObservationContext { | |
| region: Some("CA".to_owned()), | ||
| is_mobile: 0, | ||
| is_known_browser: 1, | ||
| user_agent: Some(TEST_USER_AGENT.to_owned()), | ||
| gdpr_applies: false, | ||
| consent_present: false, | ||
| slot_count, | ||
|
|
@@ -282,6 +295,8 @@ pub struct AuctionEventRow { | |
| pub is_mobile: u8, | ||
| /// `0` = bot, `1` = browser, `2` = unknown. | ||
| pub is_known_browser: u8, | ||
| /// Complete User-Agent supplied by the auction request, when present. | ||
| pub user_agent: Option<String>, | ||
| /// `0` or `1`. | ||
| pub gdpr_applies: u8, | ||
| /// `0` or `1`. | ||
|
|
@@ -341,6 +356,7 @@ impl AuctionEventRow { | |
| region: observation.region.clone(), | ||
| is_mobile: observation.is_mobile, | ||
| is_known_browser: observation.is_known_browser, | ||
| user_agent: observation.user_agent.clone(), | ||
| gdpr_applies: u8::from(observation.gdpr_applies), | ||
| consent_present: u8::from(observation.consent_present), | ||
| terminal_status: None, | ||
|
|
@@ -933,7 +949,7 @@ mod tests { | |
|
|
||
| use serde_json::json; | ||
|
|
||
| use crate::auction::types::{AdFormat, AdSlot, PublisherInfo, UserInfo}; | ||
| use crate::auction::types::{AdFormat, AdSlot, DeviceInfo, PublisherInfo, UserInfo}; | ||
|
|
||
| use super::*; | ||
|
|
||
|
|
@@ -1005,6 +1021,52 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn observation_sources_preserve_complete_user_agent() { | ||
| let ec_context = | ||
| EcContext::new_for_test(None, crate::consent::types::ConsentContext::default()); | ||
| let mut request = test_request("request-id"); | ||
| request.device = Some(DeviceInfo { | ||
| user_agent: Some(TEST_USER_AGENT.to_owned()), | ||
| ip: None, | ||
| geo: None, | ||
| }); | ||
|
|
||
| let from_request = AuctionObservationContext::from_auction_request( | ||
| AuctionSource::AuctionApi, | ||
| &request, | ||
| &ec_context, | ||
| ); | ||
| let from_parts = AuctionObservationContext::from_parts( | ||
| AuctionSource::InitialNavigation, | ||
| "test-publisher.example", | ||
| "/article", | ||
| 1, | ||
| Some(TEST_USER_AGENT), | ||
| &ec_context, | ||
| ); | ||
| let without_user_agent = AuctionObservationContext::from_auction_request( | ||
| AuctionSource::AuctionApi, | ||
| &test_request("request-without-user-agent"), | ||
| &ec_context, | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| from_request.user_agent.as_deref(), | ||
| Some(TEST_USER_AGENT), | ||
| "should preserve the complete user agent from an auction request" | ||
| ); | ||
| assert_eq!( | ||
| from_parts.user_agent.as_deref(), | ||
| Some(TEST_USER_AGENT), | ||
| "should preserve the complete user agent from publisher request parts" | ||
| ); | ||
| assert_eq!( | ||
| without_user_agent.user_agent, None, | ||
| "should omit a missing user agent" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn normalize_page_path_strips_query_and_redacts_dynamic_segments() { | ||
| assert_eq!( | ||
|
|
@@ -1065,6 +1127,11 @@ mod tests { | |
| ); | ||
|
|
||
| let rows = batch.rows(); | ||
| assert!( | ||
| rows.iter() | ||
| .all(|row| row.user_agent.as_deref() == Some(TEST_USER_AGENT)), | ||
| "should copy the complete user agent to every row kind" | ||
| ); | ||
| assert_eq!( | ||
| rows.iter() | ||
| .filter(|row| row.event_kind == "summary") | ||
|
|
@@ -1249,6 +1316,10 @@ mod tests { | |
| .expect("should serialize ndjson"); | ||
|
|
||
| assert!(body.ends_with('\n'), "should end each row with newline"); | ||
| assert!( | ||
| body.contains(TEST_USER_AGENT), | ||
| "should preserve the complete user agent in serialized rows" | ||
| ); | ||
| for line in body.lines() { | ||
| let parsed: serde_json::Value = serde_json::from_str(line).expect("should parse row"); | ||
| assert_eq!(parsed["event_kind"], "summary"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏ nitpick — The field doc says "supplied by the auction request", but two of the three sources are navigation requests: the initial-navigation and SPA paths pass the publisher-page request's header through
from_parts, and on the skipped/bot paths there is noAuctionRequestin scope at all.The same wording appears on
AuctionEventRow.user_agent(line 298 of this file) — please mirror the edit there manually.