refactor(ads-client): change dynamic traits to generic traits for min rust version compatibility

Almaju · Almaju · commit a9944b1fce5c · 2025-12-01T15:45:17.000-05:00
diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs
@@ -11,10 +11,11 @@ use crate::client::ad_response::{
     pop_request_hash_from_url, AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile,
 };
 use crate::client::config::AdsClientConfig;
-use crate::client::telemetry::{AdsTelemetry, ClientOperationEvent};
+use crate::client::telemetry::ClientOperationEvent;
 use crate::error::{RecordClickError, RecordImpressionError, ReportAdError, RequestAdsError};
-use crate::http_cache::{HttpCache, RequestCachePolicy};
+use crate::http_cache::{CacheOutcome, HttpCache, HttpCacheBuilderError, RequestCachePolicy};
 use crate::mars::MARSClient;
+use crate::telemetry::Telemetry;
 use ad_request::{AdPlacementRequest, AdRequest};
 use context_id::{ContextIDComponent, DefaultContextIdCallback};
 use url::Url;
@@ -30,14 +31,34 @@ pub mod telemetry;
 const DEFAULT_TTL_SECONDS: u64 = 300;
 const DEFAULT_MAX_CACHE_SIZE_MIB: u64 = 10;
 
-pub struct AdsClient {
-    client: MARSClient,
+pub struct AdsClient<T>
+where
+    T: Telemetry<CacheOutcome>
+        + Telemetry<ClientOperationEvent>
+        + Telemetry<HttpCacheBuilderError>
+        + Telemetry<RecordClickError>
+        + Telemetry<RecordImpressionError>
+        + Telemetry<ReportAdError>
+        + Telemetry<RequestAdsError>
+        + Telemetry<serde_json::Error>,
+{
+    client: MARSClient<T>,
     context_id_component: ContextIDComponent,
-    telemetry: Arc<dyn AdsTelemetry>,
+    telemetry: Arc<T>,
 }
 
-impl AdsClient {
-    pub fn new(client_config: AdsClientConfig) -> Self {
+impl<T> AdsClient<T>
+where
+    T: Telemetry<CacheOutcome>
+        + Telemetry<serde_json::Error>
+        + Telemetry<ClientOperationEvent>
+        + Telemetry<HttpCacheBuilderError>
+        + Telemetry<RecordClickError>
+        + Telemetry<RecordImpressionError>
+        + Telemetry<ReportAdError>
+        + Telemetry<RequestAdsError>,
+{
+    pub fn new(client_config: AdsClientConfig<T>) -> Self {
         let context_id = Uuid::new_v4().to_string();
         let context_id_component = ContextIDComponent::new(
             &context_id,
@@ -97,35 +118,19 @@ impl AdsClient {
         client
     }
 
-    #[cfg(test)]
-    pub fn new_with_mars_client(client: MARSClient) -> Self {
-        use crate::client::telemetry::PrintAdsTelemetry;
-
-        let context_id_component = ContextIDComponent::new(
-            &uuid::Uuid::new_v4().to_string(),
-            0,
-            false,
-            Box::new(DefaultContextIdCallback),
-        );
-        Self {
-            context_id_component,
-            client,
-            telemetry: Arc::new(PrintAdsTelemetry),
-        }
-    }
-
-    fn request_ads<T>(
+    fn request_ads<A>(
         &self,
         ad_placement_requests: Vec<AdPlacementRequest>,
         options: Option<RequestCachePolicy>,
-    ) -> Result<AdResponse<T>, RequestAdsError>
+    ) -> Result<AdResponse<A>, RequestAdsError>
     where
-        T: AdResponseValue,
+        A: AdResponseValue,
     {
         let context_id = self.get_context_id()?;
         let ad_request = AdRequest::build(context_id, ad_placement_requests)?;
         let cache_policy = options.unwrap_or_default();
-        let (mut response, request_hash) = self.client.fetch_ads(&ad_request, &cache_policy)?;
+        let (mut response, request_hash) =
+            self.client.fetch_ads::<A>(&ad_request, &cache_policy)?;
         response.add_request_hash_to_callbacks(&request_hash);
         Ok(response)
     }
@@ -237,18 +242,30 @@ impl AdsClient {
 mod tests {
     use crate::{
         client::config::Environment,
-        client::telemetry::PrintAdsTelemetry,
         test_utils::{
             get_example_happy_image_response, get_example_happy_spoc_response,
-            get_example_happy_uatile_response, make_happy_placement_requests,
+            get_example_happy_uatile_response, make_happy_placement_requests, PrintAdsTelemetry,
         },
     };
 
     use super::*;
 
+    fn new_with_mars_client(client: MARSClient<PrintAdsTelemetry>) -> AdsClient<PrintAdsTelemetry> {
+        let context_id_component = ContextIDComponent::new(
+            &uuid::Uuid::new_v4().to_string(),
+            0,
+            false,
+            Box::new(DefaultContextIdCallback),
+        );
+        AdsClient {
+            context_id_component,
+            client,
+            telemetry: Arc::new(PrintAdsTelemetry),
+        }
+    }
+
     #[test]
     fn test_get_context_id() {
-        use crate::client::telemetry::PrintAdsTelemetry;
         use std::sync::Arc;
         let config = AdsClientConfig {
             environment: Environment::Test,
@@ -262,7 +279,6 @@ mod tests {
 
     #[test]
     fn test_cycle_context_id() {
-        use crate::client::telemetry::PrintAdsTelemetry;
         use std::sync::Arc;
         let config = AdsClientConfig {
             environment: Environment::Test,
@@ -288,8 +304,9 @@ mod tests {
             .with_body(serde_json::to_string(&expected_response.data).unwrap())
             .create();
 
-        let mars_client = MARSClient::new(Environment::Test, None, Arc::new(PrintAdsTelemetry));
-        let ads_client = AdsClient::new_with_mars_client(mars_client);
+        let telemetry = Arc::new(PrintAdsTelemetry);
+        let mars_client = MARSClient::new(Environment::Test, None, telemetry.clone());
+        let ads_client = new_with_mars_client(mars_client);
 
         let ad_placement_requests = make_happy_placement_requests();
 
@@ -309,8 +326,9 @@ mod tests {
             .with_body(serde_json::to_string(&expected_response.data).unwrap())
             .create();
 
-        let mars_client = MARSClient::new(Environment::Test, None, Arc::new(PrintAdsTelemetry));
-        let ads_client = AdsClient::new_with_mars_client(mars_client);
+        let telemetry = Arc::new(PrintAdsTelemetry);
+        let mars_client = MARSClient::new(Environment::Test, None, telemetry.clone());
+        let ads_client = new_with_mars_client(mars_client);
 
         let ad_placement_requests = make_happy_placement_requests();
 
@@ -330,8 +348,9 @@ mod tests {
             .with_body(serde_json::to_string(&expected_response.data).unwrap())
             .create();
 
-        let mars_client = MARSClient::new(Environment::Test, None, Arc::new(PrintAdsTelemetry));
-        let ads_client = AdsClient::new_with_mars_client(mars_client);
+        let telemetry = Arc::new(PrintAdsTelemetry);
+        let mars_client = MARSClient::new(Environment::Test, None, telemetry.clone());
+        let ads_client = new_with_mars_client(mars_client);
 
         let ad_placement_requests = make_happy_placement_requests();
 
@@ -346,9 +365,9 @@ mod tests {
         let cache = HttpCache::builder("test_record_click_invalidates_cache")
             .build()
             .unwrap();
-        let mars_client =
-            MARSClient::new(Environment::Test, Some(cache), Arc::new(PrintAdsTelemetry));
-        let ads_client = AdsClient::new_with_mars_client(mars_client);
+        let telemetry = Arc::new(PrintAdsTelemetry);
+        let mars_client = MARSClient::new(Environment::Test, Some(cache), telemetry.clone());
+        let ads_client = new_with_mars_client(mars_client);
 
         let response = get_example_happy_image_response();
 
diff --git a/components/ads-client/src/client/ad_response.rs b/components/ads-client/src/client/ad_response.rs
@@ -11,23 +11,23 @@ use std::collections::HashMap;
 use url::Url;
 
 #[derive(Debug, PartialEq, Serialize)]
-pub struct AdResponse<T: AdResponseValue> {
-    pub data: HashMap<String, Vec<T>>,
+pub struct AdResponse<A: AdResponseValue> {
+    pub data: HashMap<String, Vec<A>>,
 }
 
-impl<T: AdResponseValue> AdResponse<T> {
-    pub fn parse(
+impl<A: AdResponseValue> AdResponse<A> {
+    pub fn parse<T: Telemetry<serde_json::Error> + ?Sized>(
         data: serde_json::Value,
-        telemetry: &dyn Telemetry<serde_json::Error>,
-    ) -> Result<AdResponse<T>, serde_json::Error> {
-        let raw: HashMap<String, serde_json::Value> = ::serde_json::from_value(data)?;
+        telemetry: &T,
+    ) -> Result<AdResponse<A>, serde_json::Error> {
+        let raw: HashMap<String, serde_json::Value> = serde_json::from_value(data)?;
         let mut result = HashMap::new();
 
         for (key, value) in raw {
             if let serde_json::Value::Array(arr) = value {
-                let mut ads: Vec<T> = vec![];
+                let mut ads: Vec<A> = vec![];
                 for item in arr {
-                    match serde_json::from_value::<T>(item.clone()) {
+                    match serde_json::from_value::<A>(item.clone()) {
                         Ok(ad) => ads.push(ad),
                         Err(e) => {
                             telemetry.record(&e);
@@ -60,7 +60,7 @@ impl<T: AdResponseValue> AdResponse<T> {
         }
     }
 
-    pub fn take_first(self) -> HashMap<String, T> {
+    pub fn take_first(self) -> HashMap<String, A> {
         self.data
             .into_iter()
             .filter_map(|(k, mut v)| {
diff --git a/components/ads-client/src/client/config.rs b/components/ads-client/src/client/config.rs
@@ -8,7 +8,10 @@ use std::sync::Arc;
 use once_cell::sync::Lazy;
 use url::Url;
 
-use super::telemetry::AdsTelemetry;
+use crate::client::telemetry::ClientOperationEvent;
+use crate::error::{RecordClickError, RecordImpressionError, ReportAdError, RequestAdsError};
+use crate::http_cache::{CacheOutcome, HttpCacheBuilderError};
+use crate::telemetry::Telemetry;
 
 static MARS_API_ENDPOINT_PROD: Lazy<Url> =
     Lazy::new(|| Url::parse("https://ads.mozilla.org/v1/").expect("hardcoded URL must be valid"));
@@ -17,10 +20,20 @@ static MARS_API_ENDPOINT_PROD: Lazy<Url> =
 static MARS_API_ENDPOINT_STAGING: Lazy<Url> =
     Lazy::new(|| Url::parse("https://ads.allizom.org/v1/").expect("hardcoded URL must be valid"));
 
-pub struct AdsClientConfig {
+pub struct AdsClientConfig<T>
+where
+    T: Telemetry<CacheOutcome>
+        + Telemetry<ClientOperationEvent>
+        + Telemetry<HttpCacheBuilderError>
+        + Telemetry<RecordClickError>
+        + Telemetry<RecordImpressionError>
+        + Telemetry<ReportAdError>
+        + Telemetry<RequestAdsError>
+        + Telemetry<serde_json::Error>,
+{
     pub environment: Environment,
     pub cache_config: Option<AdsCacheConfig>,
-    pub telemetry: Arc<dyn AdsTelemetry>,
+    pub telemetry: Arc<T>,
 }
 
 #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
diff --git a/components/ads-client/src/client/telemetry.rs b/components/ads-client/src/client/telemetry.rs
@@ -4,21 +4,6 @@ use std::fmt::Debug;
 * 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/.
 */
-use crate::error::{RecordClickError, RecordImpressionError, ReportAdError, RequestAdsError};
-use crate::http_cache::HttpCacheBuilderError;
-use crate::mars::MARSTelemetry;
-use crate::telemetry::Telemetry;
-
-pub trait AdsTelemetry:
-    MARSTelemetry
-    + Telemetry<ClientOperationEvent>
-    + Telemetry<HttpCacheBuilderError>
-    + Telemetry<RecordClickError>
-    + Telemetry<RecordImpressionError>
-    + Telemetry<ReportAdError>
-    + Telemetry<RequestAdsError>
-{
-}
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum ClientOperationEvent {
@@ -28,13 +13,3 @@ pub enum ClientOperationEvent {
     ReportAd,
     RequestAds,
 }
-
-pub struct PrintAdsTelemetry;
-
-impl<A: Debug> Telemetry<A> for PrintAdsTelemetry {
-    fn record(&self, event: &A) {
-        println!("record: {:?}", event);
-    }
-}
-
-impl AdsTelemetry for PrintAdsTelemetry {}
diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs
@@ -3,7 +3,7 @@
 * file, You can obtain one at http://mozilla.org/MPL/2.0/.
 */
 
-mod telemetry;
+pub mod telemetry;
 
 use std::sync::Arc;
 
@@ -12,9 +12,8 @@ use crate::client::ad_response::{
     AdCallbacks, AdImage, AdSpoc, AdTile, SpocFrequencyCaps, SpocRanking,
 };
 use crate::client::config::{AdsCacheConfig, AdsClientConfig, Environment};
-use crate::client::telemetry::{AdsTelemetry, PrintAdsTelemetry};
 use crate::error::ComponentError;
-use crate::ffi::telemetry::MozAdsTelemetryWrapper;
+use crate::ffi::telemetry::{MozAdsTelemetryWrapper, NoopMozAdsTelemetry};
 use crate::http_cache::{CacheMode, RequestCachePolicy};
 use error_support::{ErrorHandling, GetErrorHandling};
 use url::Url;
@@ -96,12 +95,16 @@ pub struct MozAdsClientConfig {
     pub telemetry: Option<Arc<dyn MozAdsTelemetry>>,
 }
 
-impl From<MozAdsClientConfig> for AdsClientConfig {
+impl From<MozAdsClientConfig> for AdsClientConfig<MozAdsTelemetryWrapper> {
     fn from(config: MozAdsClientConfig) -> Self {
-        let telemetry: Arc<dyn AdsTelemetry> = config
+        let telemetry = config
             .telemetry
-            .map(|t| Arc::new(MozAdsTelemetryWrapper { inner: t }) as Arc<dyn AdsTelemetry>)
-            .unwrap_or_else(|| Arc::new(PrintAdsTelemetry));
+            .map(|t| Arc::new(MozAdsTelemetryWrapper { inner: t }))
+            .unwrap_or_else(|| {
+                Arc::new(MozAdsTelemetryWrapper {
+                    inner: Arc::new(NoopMozAdsTelemetry),
+                })
+            });
         Self {
             environment: config.environment.into(),
             cache_config: config.cache_config.map(Into::into),
diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs
@@ -5,7 +5,7 @@
 
 use std::sync::Arc;
 
-use crate::client::telemetry::{AdsTelemetry, ClientOperationEvent};
+use crate::client::telemetry::ClientOperationEvent;
 use crate::error::{RecordClickError, RecordImpressionError, ReportAdError, RequestAdsError};
 use crate::http_cache::{CacheOutcome, HttpCacheBuilderError};
 use crate::telemetry::Telemetry;
@@ -19,6 +19,16 @@ pub trait MozAdsTelemetry: Send + Sync {
     fn record_http_cache_outcome(&self, label: &str);
 }
 
+pub struct NoopMozAdsTelemetry;
+
+impl MozAdsTelemetry for NoopMozAdsTelemetry {
+    fn record_build_cache_error(&self, _label: &str, _value: &str) {}
+    fn record_client_error(&self, _label: &str, _value: &str) {}
+    fn record_client_operation_total(&self, _label: &str) {}
+    fn record_deserialization_error(&self, _label: &str, _value: &str) {}
+    fn record_http_cache_outcome(&self, _label: &str) {}
+}
+
 pub struct MozAdsTelemetryWrapper {
     pub inner: Arc<dyn MozAdsTelemetry>,
 }
@@ -103,5 +113,3 @@ impl Telemetry<ClientOperationEvent> for MozAdsTelemetryWrapper {
         self.inner.record_client_operation_total(label);
     }
 }
-
-impl AdsTelemetry for MozAdsTelemetryWrapper {}
diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs
diff --git a/components/ads-client/src/mars.rs b/components/ads-client/src/mars.rs
diff --git a/components/ads-client/src/test_utils.rs b/components/ads-client/src/test_utils.rs
