Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
428 changes: 428 additions & 0 deletions components/ads-client/integration-tests/tests/mars_async.rs

Large diffs are not rendered by default.

99 changes: 99 additions & 0 deletions components/ads-client/src/ads_cache.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
use crate::mars::ad_response::{AdImage, AdSpoc, AdTile};
use std::{collections::HashMap, time::Duration};

// TODO: This is an intentionally naive in-memory cache implementation of the ads cache.
// It functions as a skeleton to store ads fetched in the background, and has a naive expiration mechanism.
// The subsequent vertical slice will replace this in its entirety with the http_cache sqlite database instead, with TTLs, persistent storage, etc.
const DEFAULT_TTL: Duration = Duration::from_secs(300);

#[derive(Debug)]
pub struct AdsCache {
image_ads: HashMap<String, (u64, AdImage)>,
spoc_ads: HashMap<String, (u64, Vec<AdSpoc>)>,
tile_ads: HashMap<String, (u64, AdTile)>,
}

impl Default for AdsCache {
fn default() -> Self {
Self::new()
}
}

impl AdsCache {
pub fn new() -> Self {
AdsCache {
image_ads: HashMap::new(),
spoc_ads: HashMap::new(),
tile_ads: HashMap::new(),
}
}

pub fn cache_ads<T: AdsCacheable>(
&mut self,
ads: HashMap<String, T::StorageType>,
timestamp: u64,
) {
T::cache_ads(ads, self, timestamp);
}

pub fn get_cached_ads<'a, T: AdsCacheable>(
&'a self,
placement: &str,
) -> Option<&'a T::StorageType> {
T::fetch_cached_ads(self, placement)
}
}

pub trait AdsCacheable: Sized {
// The cached ad(s) to store (eg: this may be a single ad, or an array of ads)
type StorageType;

fn cache_ads(ads: HashMap<String, Self::StorageType>, ads_cache: &mut AdsCache, timestamp: u64);
fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Self::StorageType>;
}

impl AdsCacheable for AdImage {
type StorageType = AdImage;
fn cache_ads(ads: HashMap<String, AdImage>, ads_cache: &mut AdsCache, timestamp: u64) {
ads_cache
.image_ads
.extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad))));
ads_cache
.image_ads
.retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs());

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 this is backwards:

Beyond fixing the operand order, I'd suggest we stop doing the age check as raw arithmetic on u64 at three call sites and give it a name instead, something like:

struct CacheEntry<T>{
    inserted_at: Instant,
    value: T,
}

impl<T> CacheEntry<T>{
    fn is_expired(&self, ttl: Duration) -> bool {
        self.inserted_at.elapsed() >= ttl
    }
}

}

fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a AdImage> {
ads_cache.image_ads.get(id).map(|(_, ads)| ads)
}
}

impl AdsCacheable for AdSpoc {
type StorageType = Vec<AdSpoc>;
fn cache_ads(ads: HashMap<String, Vec<AdSpoc>>, ads_cache: &mut AdsCache, timestamp: u64) {
ads_cache
.spoc_ads
.extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad))));
ads_cache
.spoc_ads
.retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs());
}
fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec<AdSpoc>> {
ads_cache.spoc_ads.get(id).map(|(_, ads)| ads)
}
}

impl AdsCacheable for AdTile {
type StorageType = AdTile;
fn cache_ads(ads: HashMap<String, AdTile>, ads_cache: &mut AdsCache, timestamp: u64) {
ads_cache
.tile_ads
.extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad))));
ads_cache
.tile_ads
.retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs());
}
fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a AdTile> {
ads_cache.tile_ads.get(id).map(|(_, ads)| ads)
}
}

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.

A few thoughts here:

  1. Do we need the StorageType associated type / three separate maps? It looks like this could just be placement_id -> Vec<Ad> uniformly. image/tile would simply be a Vec of length 0 or 1, and the unwrap-to-single-ad already happens at the call site (take_first() in client.rs). That removes the trait indirection and the three near-identical impls.
  2. Naming: is "cache" the right word? We already have http_cache as the actual cache (lookup-before-fetch, sqlite-backed, TTL/ETag). This structure is only ever written by the background worker after a prefetch resolves, and read via the query_* methods -> that reads more like a materialized view (CQRS-style) than a cache to me, and having two different "cache" concepts in the same crate risks confusion down the line. Would AdsStore be clearer?
  3. Could we use a PlacementId newtype instead of a raw String while we're touching this to make it clearer what the key is?

@thesuzerain thesuzerain Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

  1. My initial version of it looked something like this but I ended up veering away from it because it felt a bit odd to be storing them as Vec<...> and then running them immediately. Happy to do it however though.

  2. I'm happy to rename it to AdsStore. I think the term cache is still appropriate here but I agree with the naming confusion- it is intended to be bundled into the same sqlite database eventually as well.

48 changes: 45 additions & 3 deletions components/ads-client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

use std::collections::HashMap;
use std::time::Duration;

use crate::ads_cache::{AdsCache, AdsCacheable};
use crate::http_cache::{ByteSize, CachePolicy, HttpCache};
use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags};
use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile};
Expand All @@ -15,6 +13,8 @@ use crate::telemetry::Telemetry;
use config::AdsClientConfig;
use context_id::{ContextIDComponent, DefaultContextIdCallback};
use error::RequestAdsError;
use std::collections::HashMap;
use std::time::Duration;
use url::Url;
use uuid::Uuid;

Expand Down Expand Up @@ -42,6 +42,7 @@ where
client: MARSClient<T>,
context_id_provider: Box<dyn ContextIdProvider>,
telemetry: T,
ads_cache: AdsCache,
}

impl<T> AdsClient<T>
Expand Down Expand Up @@ -91,6 +92,7 @@ where
client,
context_id_provider,
telemetry: telemetry.clone(),
ads_cache: AdsCache::new(),
}
}

Expand All @@ -110,6 +112,15 @@ where
Ok(())
}

pub fn cache_ads<A: AdsCacheable>(&mut self, ads: HashMap<String, A::StorageType>) {
let now = chrono::Utc::now().timestamp().unsigned_abs();
self.ads_cache.cache_ads::<A>(ads, now);
}

pub fn get_cached_ads<A: AdsCacheable>(&self, placement_id: &str) -> Option<&A::StorageType> {
self.ads_cache.get_cached_ads::<A>(placement_id)
}

pub fn get_context_id(&self) -> context_id::ApiResult<String> {
self.context_id_provider.context_id()
}
Expand Down Expand Up @@ -264,6 +275,7 @@ where
}
}

// Event fires in both sync and background strategies.
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum ClientOperationEvent {
New,
Expand All @@ -273,6 +285,35 @@ pub enum ClientOperationEvent {
RequestAds,
}

// Event fires when dispatch is fired, not when the event resolves.
pub enum CommandDispatchedOperationEvent {
RecordClick,
RecordImpression,
ReportAd,
RequestAds,
}

// Event fires when the corresponding background event resolves.
pub enum CommandProcessedOperationEvent {
RecordClick,
RecordImpression,
ReportAd,
RequestAds,
}

// Event fires when the corresponding background event fails to resolve.
pub enum CommandFailedOperationEvent {
RecordClick,
RecordImpression,
ReportAd,
RequestAds,
}

Comment on lines +289 to +311

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.

These four enums all share the same four variants. Could we collapse them into a single ClientEvent enum plus a separate Phase (Dispatched/Processed/Failed)? That would also address the naming, CommandDispatchedOperationEvent is quite long for something scoped to this module.

pub enum WorkerMetaEvent {
Start,
Stop,
}

#[cfg(test)]
mod tests {
use std::{assert_eq, assert_ne, sync::Arc};
Expand Down Expand Up @@ -301,6 +342,7 @@ mod tests {
Box::new(DefaultContextIdCallback),
)),
telemetry,
ads_cache: AdsCache::new(),
}
}

Expand Down
33 changes: 32 additions & 1 deletion components/ads-client/src/client/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError};
use crate::{
mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError},
worker::command,
};
use std::sync::mpsc::{RecvTimeoutError, TrySendError};

#[derive(Debug, thiserror::Error)]
pub enum ComponentError {
Expand All @@ -18,6 +22,9 @@ pub enum ComponentError {

#[error("Error requesting ads: {0}")]
RequestAds(#[from] RequestAdsError),

#[error("Error requesting ads from worker: {0}")]
BackgroundWorker(#[from] BackgroundWorkerError),

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.

Nit: could we keep the variants alphabetically ordered here? BackgroundWorker would go first, ahead of the Record*/Report*/RequestAds variants. Same applies to BackgroundWorkerError below (PongFailure, WorkerClosed, WorkerFull, WorkerTimedOut).

}

#[derive(Debug, thiserror::Error)]
Expand All @@ -28,3 +35,27 @@ pub enum RequestAdsError {
#[error("Error requesting ads from MARS: {0}")]
FetchAds(#[from] FetchAdsError),
}

#[derive(Debug, thiserror::Error)]
pub enum BackgroundWorkerError {
#[error("Error requesting new ads from the background worker: worker full")]
WorkerFull,

#[error("Error requesting new ads from the background worker: worker closed")]
WorkerClosed,

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.

If this happens, it looks like the client stays broken for the rest of its lifetime, nothing here attempts to restart the worker thread. Since this is an internal implementation detail, I'd prefer we recover from it (respawn) rather than propagate the error to the caller.


#[error("Worker timed out waiting for response: {0}")]
WorkerTimedOut(#[from] RecvTimeoutError),

#[error("Error sending pong back from background worker")]
PongFailure(Box<TrySendError<()>>),
}

impl From<TrySendError<command::DispatchCommand>> for BackgroundWorkerError {
fn from(value: TrySendError<command::DispatchCommand>) -> Self {
match value {
TrySendError::Disconnected(_) => BackgroundWorkerError::WorkerClosed,
TrySendError::Full(_) => BackgroundWorkerError::WorkerFull,
}
}
}
25 changes: 12 additions & 13 deletions components/ads-client/src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
pub mod error;
pub mod telemetry;

use std::sync::Arc;

use crate::client::config::{AdsCacheConfig, AdsClientConfig};
use crate::client::{AdsClient, ContextIdProvider};
use crate::ffi::telemetry::MozAdsTelemetryWrapper;
Expand All @@ -20,14 +18,14 @@ use crate::mars::ad_response::{
};
use crate::mars::Environment;
use crate::mars::ReportReason;
use crate::AdsClientUrl;
use crate::MozAdsClient;
use crate::{worker, AdsClientUrl};
use parking_lot::Mutex;
use std::collections::HashMap;
use std::sync::Arc;

pub use error::{AdsClientApiResult, MozAdsClientApiError};
pub use telemetry::MozAdsTelemetry;

// TODO: Temporary workaround for HNT requirements — do not use for new integrations.
// Context ID management should remain internal to the ads client and this interface should be removed.
#[uniffi::export(with_foreign)]
Expand Down Expand Up @@ -55,7 +53,7 @@ impl From<MozAdsContextIdProviderWrapper> for Box<dyn ContextIdProvider> {
}
}

#[derive(Default, uniffi::Record)]
#[derive(Default, uniffi::Record, Clone)]
pub struct MozAdsRequestOptions {
pub cache_policy: Option<MozAdsCachePolicy>,
#[uniffi(default)]
Expand Down Expand Up @@ -124,6 +122,11 @@ impl MozAdsClientBuilder {

pub fn build(&self) -> MozAdsClient {
let inner = self.0.lock();
let telemetry = inner
.telemetry
.clone()
.map(MozAdsTelemetryWrapper::new)
.unwrap_or_else(MozAdsTelemetryWrapper::noop);
let client_config = AdsClientConfig {
cache_config: inner.cache_config.clone().map(Into::into),
context_id_provider: inner
Expand All @@ -132,16 +135,12 @@ impl MozAdsClientBuilder {
.map(MozAdsContextIdProviderWrapper::new)
.map(Into::into),
environment: inner.environment.unwrap_or_default().into(),
telemetry: inner
.telemetry
.clone()
.map(MozAdsTelemetryWrapper::new)
.unwrap_or_else(MozAdsTelemetryWrapper::noop),
telemetry: telemetry.clone(),
};
let client = AdsClient::new(client_config);
MozAdsClient {
inner: Mutex::new(client),
}
let inner = Arc::new(Mutex::new(client));
let worker = worker::AdsClientWorkerWrapper::new(inner.clone(), telemetry);
MozAdsClient { inner, worker }
}

pub fn cache_config(self: Arc<Self>, cache_config: MozAdsCacheConfig) -> Arc<Self> {
Expand Down
54 changes: 53 additions & 1 deletion components/ads-client/src/ffi/telemetry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ use std::sync::Arc;
use parking_lot::RwLock;

use crate::client::error::RequestAdsError;
use crate::client::ClientOperationEvent;
use crate::client::{
ClientOperationEvent, CommandDispatchedOperationEvent, CommandFailedOperationEvent,
CommandProcessedOperationEvent, WorkerMetaEvent,
};
use crate::http_cache::{CacheOutcome, HttpCacheBuilderError};
use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError};
use crate::telemetry::Telemetry;
Expand Down Expand Up @@ -101,6 +104,55 @@ impl Telemetry for MozAdsTelemetryWrapper {
});
return;
}

if let Some(client_op) = event.downcast_ref::<CommandDispatchedOperationEvent>() {
inner.record_client_operation_total(match client_op {
CommandDispatchedOperationEvent::RecordClick => {
"cmd_dispatch_record_click".to_string()
}
CommandDispatchedOperationEvent::RecordImpression => {
"cmd_dispatch_record_impression".to_string()
}
CommandDispatchedOperationEvent::ReportAd => "cmd_dispatch_report_ad".to_string(),
CommandDispatchedOperationEvent::RequestAds => "cmd_dispatch_report_ad".to_string(),
});
return;
}

if let Some(client_op) = event.downcast_ref::<CommandProcessedOperationEvent>() {
inner.record_client_operation_total(match client_op {
CommandProcessedOperationEvent::RecordClick => {
"cmd_processed_record_click".to_string()
}
CommandProcessedOperationEvent::RecordImpression => {
"cmd_processed_record_impression".to_string()
}
CommandProcessedOperationEvent::ReportAd => "cmd_processed_report_ad".to_string(),
CommandProcessedOperationEvent::RequestAds => "cmd_processed_report_ad".to_string(),
});
return;
}

if let Some(client_op) = event.downcast_ref::<CommandFailedOperationEvent>() {
inner.record_client_operation_total(match client_op {
CommandFailedOperationEvent::RecordClick => "cmd_failed_record_click".to_string(),
CommandFailedOperationEvent::RecordImpression => {
"cmd_failed_record_impression".to_string()
}
CommandFailedOperationEvent::ReportAd => "cmd_failed_report_ad".to_string(),
CommandFailedOperationEvent::RequestAds => "cmd_failed_report_ad".to_string(),
});
return;
}

if let Some(client_op) = event.downcast_ref::<WorkerMetaEvent>() {
inner.record_client_operation_total(match client_op {
WorkerMetaEvent::Start => "worker_started".to_string(),
WorkerMetaEvent::Stop => "worker_ended".to_string(),
});
return;
}

if let Some(cache_builder_error) = event.downcast_ref::<HttpCacheBuilderError>() {
inner.record_build_cache_error(
match cache_builder_error {
Expand Down
Loading
Loading