Skip to content

feat(ads-client): roundtrip cap key for spoc impression callbacks (AC-125) #7442

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.

Sign up for GitHub

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

Open
jonesetc wants to merge 2 commits into
base: main
Choose a base branch
from
+143 −4
Open

feat(ads-client): roundtrip cap key for spoc impression callbacks (AC-125) #7442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12ad45c
Add roundtripping of cap_key in sposored content impression callback …
jonesetc Jun 22, 2026
e50e0d5
address feedback
jonesetc Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions components/ads-client/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,29 @@ where
// if let Some(request_hash) = pop_request_hash_from_url(&mut impression_url) {
// let _ = self.client.invalidate_cache_by_hash(&request_hash);
// }

// TODO: Add count call with _cap_key for impression capping logic
let impression_url = if let Some((_, _cap_key)) = impression_url
.query_pairs()
.find(|(key, _)| key == "cap_key")
{
let mut new_url = impression_url.clone();
new_url
.query_pairs_mut()
.clear()
.extend_pairs(
impression_url
.query_pairs()
.collect::<Vec<_>>()
.iter()
.filter(|(key, _)| key != "cap_key"),
)
.finish();
new_url
} else {
impression_url
};

self.client
.record_impression(impression_url, ohttp)
.inspect_err(|e| {
Expand Down Expand Up @@ -392,6 +415,33 @@ mod tests {
m.assert();
}

#[test]
fn test_record_impression_removes_cap_key() {
viaduct_dev::init_backend_dev();
let mars_client = MARSClient::new(Environment::Test, None, MozAdsTelemetryWrapper::noop());
let ads_client = new_with_mars_client(mars_client);

let base_url = mockito::server_url();
let path_and_query = "/impression?kept=example";
let callback_url = Url::parse(&format!("{}{}", base_url, path_and_query)).unwrap();

let mock = mockito::mock("GET", path_and_query)
.with_status(200)
.create();

ads_client.record_impression(callback_url, false).unwrap();

mock.assert();

let callback_url_with_cap_key =
Url::parse(&format!("{}{}&cap_key=test", base_url, path_and_query)).unwrap();
ads_client
.record_impression(callback_url_with_cap_key, false)
.unwrap();

mock.expect(2).assert();
}

#[test]
#[ignore = "Cache invalidation temporarily disabled - will be re-enabled behind Nimbus experiment"]
fn test_record_click_invalidates_cache() {
Expand Down
97 changes: 93 additions & 4 deletions components/ads-client/src/mars/ad_response.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,17 @@ impl<A: AdResponseValue> AdResponse<A> {
let hash_str = request_hash.to_string();
for (placement_id, ads) in self.data.iter_mut() {
for (position, ad) in ads.iter_mut().enumerate() {
let cap_key = ad.cap_key();
let callbacks = ad.callbacks_mut();
callbacks
.click
.query_pairs_mut()
.append_pair("request_hash", &hash_str);
callbacks
.impression
.query_pairs_mut()
.append_pair("request_hash", &hash_str);
let mut impression_callback_query = callbacks.impression.query_pairs_mut();
impression_callback_query.append_pair("request_hash", &hash_str);
if let Some(cap_key) = cap_key {
impression_callback_query.append_pair("cap_key", &cap_key);
}
if let Some(report_url) = callbacks.report.as_mut() {
report_url
.query_pairs_mut()
Expand Down Expand Up @@ -161,6 +163,9 @@ pub struct AdCallbacks {

pub trait AdResponseValue: DeserializeOwned {
fn callbacks_mut(&mut self) -> &mut AdCallbacks;
fn cap_key(&self) -> Option<String> {
None
}
}

impl AdResponseValue for AdImage {
Expand All @@ -173,6 +178,10 @@ impl AdResponseValue for AdSpoc {
fn callbacks_mut(&mut self) -> &mut AdCallbacks {
&mut self.callbacks
}

fn cap_key(&self) -> Option<String> {
Some(self.caps.cap_key.clone())
}

@Almaju Almaju Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can do fn impression_enriching_params(&self) -> [(&str, &str); 1] to have zero allocation (no Vec, no String)

@jonesetc jonesetc Jun 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the &str (or any reference returned) in there is that this result is used during the mutable borrow of callbacks_mut, so those would have to be cloned anyway. I think returning something like Option<IntoIterator<(String, String)>> should be possible and avoid allocation in the non-spoc case and just a single small array in the spoc case.

@jonesetc jonesetc Jun 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went ahead and simplified it instead of worrying about iterators or refs. Now there is just a method that returns an optional cap_key, by default None, and spoc returns a string. The string would need to be owned eventually like i mentioned because of the mutable borrow for callbacks. so I figured just returning the string made more sense than making the caller do something like:

let cap_key = ad.cap_key().map(String::from)

}

impl AdResponseValue for AdTile {
Expand Down Expand Up @@ -514,6 +523,86 @@ mod tests {
assert!(report_1.contains("position=1"));
}

#[test]
fn test_enrich_callbacks_spoc_impressions_have_cap_key() {
let mut response = AdResponse {
data: HashMap::from([(
"tile1".into(),
vec![
AdSpoc {
block_key: "block_key1".into(),
callbacks: AdCallbacks {
click: Url::parse("https://example.com/click1").unwrap(),
impression: Url::parse("https://example.com/impression1").unwrap(),
report: Some(Url::parse("https://example.com/report1").unwrap()),
},
caps: SpocFrequencyCaps {
cap_key: "cap_key1".into(),
day: 100,
},
domain: "1.example.com".into(),
excerpt: "excerpt1".into(),
format: "format1".into(),
image_url: "https://example.com/image1.png".into(),
url: "https://example.com/ad1".into(),
ranking: SpocRanking {
priority: 1,
personalization_models: None,
item_score: 1.0,
},
sponsor: "sponsor1".into(),
sponsored_by_override: None,
title: "title1".into(),
},
AdSpoc {
block_key: "block_key2".into(),
callbacks: AdCallbacks {
click: Url::parse("https://example.com/click2").unwrap(),
impression: Url::parse("https://example.com/impression2").unwrap(),
report: Some(Url::parse("https://example.com/report2").unwrap()),
},
caps: SpocFrequencyCaps {
cap_key: "cap_key2".into(),
day: 200,
},
domain: "2.example.com".into(),
excerpt: "excerpt2".into(),
format: "format2".into(),
image_url: "https://example.com/image2.png".into(),
url: "https://example.com/ad2".into(),
ranking: SpocRanking {
priority: 2,
personalization_models: None,
item_score: 2.0,
},
sponsor: "sponsor2".into(),
sponsored_by_override: None,
title: "title2".into(),
},
],
)]),
};

let request_hash = RequestHash::from("abc123def456");
response.enrich_callbacks(&request_hash);

let ads = &response.data["tile1"];

assert!(ads[0]
.callbacks
.impression
.query()
.unwrap_or("")
.contains("cap_key=cap_key1"));

assert!(ads[1]
.callbacks
.impression
.query()
.unwrap_or("")
.contains("cap_key=cap_key2"));
}

#[test]
fn test_enrich_callbacks_skips_ads_without_report_url() {
let mut response = AdResponse {
Expand Down
Loading