-
Notifications
You must be signed in to change notification settings - Fork 278
[AC-154] implement fire and forget ads client async #7531
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
base: main
Are you sure you want to change the base?
Changes from all commits
dcad72b
d36cebb
860e022
f3f9ea0
ed31114
cfe9e68
2539df5
fb9e54f
1b784f7
bdf8e43
ddb1f4f
754dd50
af15b83
03afff8
a80bec8
2aa194e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| 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()); | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
| } | ||
|
Contributor
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. A few thoughts here:
Collaborator
Author
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.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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}; | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -42,6 +42,7 @@ where | |
| client: MARSClient<T>, | ||
| context_id_provider: Box<dyn ContextIdProvider>, | ||
| telemetry: T, | ||
| ads_cache: AdsCache, | ||
| } | ||
|
|
||
| impl<T> AdsClient<T> | ||
|
|
@@ -91,6 +92,7 @@ where | |
| client, | ||
| context_id_provider, | ||
| telemetry: telemetry.clone(), | ||
| ads_cache: AdsCache::new(), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -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() | ||
| } | ||
|
|
@@ -264,6 +275,7 @@ where | |
| } | ||
| } | ||
|
|
||
| // Event fires in both sync and background strategies. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum ClientOperationEvent { | ||
| New, | ||
|
|
@@ -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
Contributor
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. These four enums all share the same four variants. Could we collapse them into a single |
||
| pub enum WorkerMetaEvent { | ||
| Start, | ||
| Stop, | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use std::{assert_eq, assert_ne, sync::Arc}; | ||
|
|
@@ -301,6 +342,7 @@ mod tests { | |
| Box::new(DefaultContextIdCallback), | ||
| )), | ||
| telemetry, | ||
| ads_cache: AdsCache::new(), | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
|
@@ -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), | ||
|
Contributor
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. Nit: could we keep the variants alphabetically ordered here? |
||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
|
|
@@ -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, | ||
|
Contributor
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. 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, | ||
| } | ||
| } | ||
| } | ||
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.
I think this is backwards:
t=1000: we cache an image ad for placement"a"→ stored as("a", (1000, ad)).t=1100: we prefetch an image ad for placement"b"→cache_adsis called withtimestamp=1100, sox=1000for"a"'s existing entry andx=1100for"b"'s new one.retainevaluates"b":1100 - 1100 = 0 < 300→ kept, correct.retainevaluates"a":1000 - 1100→ this is a panic https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=aaa058d8f0bc8f002ce41e92ddb06bbaBeyond fixing the operand order, I'd suggest we stop doing the age check as raw arithmetic on
u64at three call sites and give it a name instead, something like: