Skip to content

[AC-144] Initial implementation of impression capping in MAC #7465

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

Draft
jonesetc wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[AC-144] Initial implementation of impression capping in MAC #7465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions components/ads-client/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ context_id = { path = "../context_id" }
error-support = { path = "../support/error" }
parking_lot = "0.12"
rusqlite = { version = "0.37.0", features = [
"array",
"functions",
"bundled",
"serde_json",
Expand Down
8 changes: 8 additions & 0 deletions ...ents/ads-client/android/src/main/java/mozilla/appservices/adsclient/AdsClientTelemetry.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,12 @@ class AdsClientTelemetry : MozAdsTelemetry {
override fun recordHttpCacheOutcome(label: String, value: String) {
AdsClient.httpCacheOutcome[label].set(value)
}

override fun recordBuildImpressionLogError(label: String, value: String) {
AdsClient.impressionLogError[label].set(value)
}

override fun recordImpressionLogOutcome(label: String, value: String) {
AdsClient.impressionLogOutcome[label].set(value)
}
}
125 changes: 89 additions & 36 deletions components/ads-client/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ use std::collections::HashMap;
use std::time::Duration;

use crate::http_cache::{ByteSize, CachePolicy, HttpCache};
use crate::impression_log::{ImpressionCappingPolicy, ImpressionLog};
use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags};
use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile};
use crate::mars::ad_response::{
pop_query_param_from_url, AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile,
};
use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError};
use crate::mars::{MARSClient, ReportReason};
use crate::telemetry::Telemetry;
Expand Down Expand Up @@ -85,7 +88,20 @@ where
}
});

let client = MARSClient::new(environment, http_cache, telemetry.clone());
let impression_log =
client_config
.impression_log_config
.and_then(|impression_log_config| {
match ImpressionLog::builder(impression_log_config.db_path).build() {
Ok(cache) => Some(cache),
Err(e) => {
telemetry.record(&e);
None
}
}
});

let client = MARSClient::new(environment, http_cache, impression_log, telemetry.clone());
telemetry.record(&ClientOperationEvent::New);
Self {
client,
Expand Down Expand Up @@ -121,7 +137,7 @@ where

pub fn record_impression(
&self,
impression_url: Url,
mut impression_url: Url,
ohttp: bool,
) -> Result<(), RecordImpressionError> {
// TODO: Re-enable cache invalidation behind a Nimbus experiment.
Expand All @@ -131,30 +147,9 @@ where
// 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
};

let cap_key = pop_query_param_from_url(&mut impression_url, "cap_key");
self.client
.record_impression(impression_url, ohttp)
.record_impression(impression_url, ohttp, cap_key.as_deref())
.inspect_err(|e| {
self.telemetry.record(e);
})
Expand Down Expand Up @@ -185,10 +180,17 @@ where
ad_placement_requests: Vec<AdPlacementRequest>,
flags: AdRequestFlags,
options: Option<CachePolicy>,
impression_capping_policy: Option<ImpressionCappingPolicy>,
ohttp: bool,
) -> Result<HashMap<String, AdImage>, RequestAdsError> {
let response = self
.request_ads::<AdImage>(ad_placement_requests, flags, options, ohttp)
.request_ads::<AdImage>(
ad_placement_requests,
flags,
options,
impression_capping_policy,
ohttp,
)
.inspect_err(|e| {
self.telemetry.record(e);
})?;
Expand All @@ -201,9 +203,16 @@ where
ad_placement_requests: Vec<AdPlacementRequest>,
flags: AdRequestFlags,
options: Option<CachePolicy>,
impression_capping_policy: Option<ImpressionCappingPolicy>,
ohttp: bool,
) -> Result<HashMap<String, Vec<AdSpoc>>, RequestAdsError> {
let result = self.request_ads::<AdSpoc>(ad_placement_requests, flags, options, ohttp);
let result = self.request_ads::<AdSpoc>(
ad_placement_requests,
flags,
options,
impression_capping_policy,
ohttp,
);
result
.inspect_err(|e| {
self.telemetry.record(e);
Expand All @@ -219,9 +228,16 @@ where
ad_placement_requests: Vec<AdPlacementRequest>,
flags: AdRequestFlags,
options: Option<CachePolicy>,
impression_capping_policy: Option<ImpressionCappingPolicy>,
ohttp: bool,
) -> Result<HashMap<String, AdTile>, RequestAdsError> {
let result = self.request_ads::<AdTile>(ad_placement_requests, flags, options, ohttp);
let result = self.request_ads::<AdTile>(
ad_placement_requests,
flags,
options,
impression_capping_policy,
ohttp,
);
result
.inspect_err(|e| {
self.telemetry.record(e);
Expand All @@ -237,16 +253,23 @@ where
placements: Vec<AdPlacementRequest>,
flags: AdRequestFlags,
options: Option<CachePolicy>,
impression_capping_policy: Option<ImpressionCappingPolicy>,
ohttp: bool,
) -> Result<AdResponse<A>, RequestAdsError>
where
A: AdResponseValue,
{
let context_id = self.get_context_id()?;
let cache_policy = options.unwrap_or_default();
let (mut response, request_hash) =
self.client
.fetch_ads::<A>(context_id, flags, placements, cache_policy, ohttp)?;
let impression_capping_policy = impression_capping_policy.unwrap_or_default();
let (mut response, request_hash) = self.client.fetch_ads::<A>(
context_id,
flags,
placements,
cache_policy,
impression_capping_policy,
ohttp,
)?;
response.enrich_callbacks(&request_hash);
Ok(response)
}
Expand Down Expand Up @@ -295,6 +318,7 @@ mod tests {
cache_config: None,
context_id_provider: None,
environment: Environment::Test,
impression_log_config: None,
telemetry: MozAdsTelemetryWrapper::noop(),
};
let client = AdsClient::new(config);
Expand All @@ -313,13 +337,19 @@ mod tests {
.with_body(serde_json::to_string(&expected_response.data).unwrap())
.create();

let mars_client = MARSClient::new(Environment::Test, None, MozAdsTelemetryWrapper::noop());
let mars_client = MARSClient::new(
Environment::Test,
None,
None,
MozAdsTelemetryWrapper::noop(),
);
let ads_client = new_with_mars_client(mars_client);

let result = ads_client.request_image_ads(
make_happy_placement_requests(),
AdRequestFlags::default(),
None,
None,
false,
);
assert!(result.is_ok());
Expand All @@ -337,13 +367,19 @@ mod tests {
.with_body(serde_json::to_string(&expected_response.data).unwrap())
.create();

let mars_client = MARSClient::new(Environment::Test, None, MozAdsTelemetryWrapper::noop());
let mars_client = MARSClient::new(
Environment::Test,
None,
None,
MozAdsTelemetryWrapper::noop(),
);
let ads_client = new_with_mars_client(mars_client);

let result = ads_client.request_spoc_ads(
make_happy_placement_requests(),
AdRequestFlags::default(),
None,
None,
false,
);
assert!(result.is_ok());
Expand All @@ -361,13 +397,19 @@ mod tests {
.with_body(serde_json::to_string(&expected_response.data).unwrap())
.create();

let mars_client = MARSClient::new(Environment::Test, None, MozAdsTelemetryWrapper::noop());
let mars_client = MARSClient::new(
Environment::Test,
None,
None,
MozAdsTelemetryWrapper::noop(),
);
let ads_client = new_with_mars_client(mars_client);

let result = ads_client.request_tile_ads(
make_happy_placement_requests(),
AdRequestFlags::default(),
None,
None,
false,
);
assert!(result.is_ok());
Expand Down Expand Up @@ -399,6 +441,7 @@ mod tests {
cache_config: None,
context_id_provider: Some(Box::new(FixedContextId)),
environment: Environment::Test,
impression_log_config: None,
telemetry: MozAdsTelemetryWrapper::noop(),
};
let client = AdsClient::new(config);
Expand All @@ -409,6 +452,7 @@ mod tests {
make_happy_placement_requests(),
AdRequestFlags::default(),
None,
None,
false,
);
assert!(result.is_ok());
Expand All @@ -418,7 +462,12 @@ mod tests {
#[test]
fn test_record_impression_removes_cap_key() {
viaduct_dev::init_backend_dev();
let mars_client = MARSClient::new(Environment::Test, None, MozAdsTelemetryWrapper::noop());
let mars_client = MARSClient::new(
Environment::Test,
None,
None,
MozAdsTelemetryWrapper::noop(),
);
let ads_client = new_with_mars_client(mars_client);

let base_url = mockito::server_url();
Expand Down Expand Up @@ -452,6 +501,7 @@ mod tests {
let mars_client = MARSClient::new(
Environment::Test,
Some(cache),
None,
MozAdsTelemetryWrapper::noop(),
);
let ads_client = new_with_mars_client(mars_client);
Expand All @@ -470,6 +520,7 @@ mod tests {
make_happy_placement_requests(),
AdRequestFlags::default(),
None,
None,
false,
)
.unwrap();
Expand All @@ -484,6 +535,7 @@ mod tests {
make_happy_placement_requests(),
AdRequestFlags::default(),
None,
None,
false,
)
.unwrap();
Expand All @@ -495,6 +547,7 @@ mod tests {
make_happy_placement_requests(),
AdRequestFlags::default(),
Some(CachePolicy::default()),
None,
false,
)
.unwrap();
Expand Down
6 changes: 6 additions & 0 deletions components/ads-client/src/client/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ where
pub cache_config: Option<AdsCacheConfig>,
pub context_id_provider: Option<Box<dyn super::ContextIdProvider>>,
pub environment: Environment,
pub impression_log_config: Option<ImpressionLogConfig>,
pub telemetry: T,
}

Expand All @@ -22,3 +23,8 @@ pub struct AdsCacheConfig {
pub default_cache_ttl_seconds: Option<u64>,
pub max_size_mib: Option<u64>,
}

#[derive(Clone, Debug)]
pub struct ImpressionLogConfig {
pub db_path: String,
}
35 changes: 35 additions & 0 deletions components/ads-client/src/clock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

pub trait Clock: Send + Sync + 'static {
fn now_epoch_seconds(&self) -> i64;
#[cfg(test)]
fn advance(&self, secs: i64);
}

#[cfg(test)]
pub struct TestClock {
now: std::sync::atomic::AtomicI64,
}

#[cfg(test)]
impl TestClock {
pub fn new(start: i64) -> Self {
Self {
now: std::sync::atomic::AtomicI64::new(start),
}
}
}

#[cfg(test)]
impl Clock for TestClock {
fn now_epoch_seconds(&self) -> i64 {
self.now.load(std::sync::atomic::Ordering::Relaxed)
}

fn advance(&self, secs: i64) {
self.now
.fetch_add(secs, std::sync::atomic::Ordering::Relaxed);
}
}
Loading
Loading