feat: initialize suggestion providers at start up [CONSVC-1247] (#193)

Moves initialization of suggestion providers to startup and shares the initialized data between threads. Closes #127
mozilla-services · Nov 9, 2021 · e1c2561 · e1c2561
1 parent 3df72ed
commit e1c2561
Show file tree

Hide file tree

Showing 12 changed files with 141 additions and 165 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/merino-adm/src/remote_settings/client.rs b/merino-adm/src/remote_settings/client.rs
@@ -88,6 +88,7 @@ impl RemoteSettingsClient {
             let records_res = self
                 .reqwest_client
                 .get(url.clone())
+                .timeout(std::time::Duration::from_secs(3))
                 .send()
                 .await
                 .and_then(Response::error_for_status)

diff --git a/merino-adm/src/remote_settings/mod.rs b/merino-adm/src/remote_settings/mod.rs
@@ -50,7 +50,6 @@ impl RemoteSettingsSuggester {
         let suggestions = Arc::new(DedupedMap::new());
 
         Self::sync(&mut remote_settings_client, &*suggestions).await?;
-
         {
             let task_suggestions = Arc::clone(&suggestions);
             let task_interval = config.resync_interval;
@@ -106,7 +105,6 @@ impl RemoteSettingsSuggester {
             r#type = "adm.remote-settings.sync-start",
             "Syncing quicksuggest records from Remote Settings"
         );
-
         remote_settings_client.sync().await?;
 
         // Download and process all the attachments concurrently

diff --git a/merino-integration-tests/src/suggest.rs b/merino-integration-tests/src/suggest.rs
@@ -3,7 +3,6 @@
 
 use crate::{merino_test_macro, utils::test_tools::TestReqwestClient, TestingTools};
 use anyhow::Result;
-use httpmock::{Method::GET, MockServer};
 use merino_settings::providers::{
     FixedConfig, KeywordFilterConfig, MultiplexerConfig, RemoteSettingsConfig,
     SuggestionProviderConfig,
@@ -130,16 +129,9 @@ async fn test_expected_variant_fields(
         "adm".to_string(),
         SuggestionProviderConfig::RemoteSettings(RemoteSettingsConfig::default())
     );
+    settings.remote_settings.test_changes = Some(vec![])
 })]
-async fn suggest_adm_rs_works_empty(
-    TestingTools {
-        test_client,
-        remote_settings_mock,
-        ..
-    }: TestingTools,
-) -> Result<()> {
-    setup_remote_settings_collection(&remote_settings_mock, &[]).await;
-
+async fn suggest_adm_rs_works_empty(TestingTools { test_client, .. }: TestingTools) -> Result<()> {
     let response = test_client.get("/api/v1/suggest?q=apple").send().await?;
 
     // Check that the status is 200 OK, and that the body is JSON. The
@@ -157,16 +149,11 @@ async fn suggest_adm_rs_works_empty(
         "adm".to_string(),
         SuggestionProviderConfig::RemoteSettings(RemoteSettingsConfig::default())
     );
+    settings.remote_settings.test_changes = Some(vec!["apple".to_string(), "banana".to_string()]);
 })]
 async fn suggest_adm_rs_works_content(
-    TestingTools {
-        test_client,
-        remote_settings_mock,
-        ..
-    }: TestingTools,
+    TestingTools { test_client, .. }: TestingTools,
 ) -> Result<()> {
-    setup_remote_settings_collection(&remote_settings_mock, &["apple", "banana"]).await;
-
     let response = test_client.get("/api/v1/suggest?q=apple").send().await?;
 
     assert_eq!(response.status(), StatusCode::OK);
@@ -180,63 +167,6 @@ async fn suggest_adm_rs_works_content(
     Ok(())
 }
 
-async fn setup_remote_settings_collection(server: &MockServer, suggestions: &[&str]) {
-    let mut changes = suggestions
-        .iter()
-        .map(|s| {
-            assert_ne!(*s, "icon");
-            json!({
-                "id": s,
-                "type": "data",
-                "last_modified": 0,
-                "attachment": {
-                    "location": format!("/attachment/data-{}.json", s),
-                    "hash": s,
-                }
-            })
-        })
-        .collect::<Vec<_>>();
-    changes.push(json!({
-        "id": "icon-1",
-        "type": "icon",
-        "last_modified": 0,
-        "attachment": {
-            "location": "/attachment/icon-1.png",
-            "hash": "icon"
-        }
-    }));
-
-    server
-        .mock_async(|when, then| {
-            when.method(GET)
-                .path("/v1/buckets/main/collections/quicksuggest/changeset");
-            then.status(200).json_body(json!({
-                "changes": changes,
-            }));
-        })
-        .await;
-
-    for (idx, s) in suggestions.iter().enumerate() {
-        server
-            .mock_async(|when, then| {
-                when.method(GET)
-                    .path(format!("/attachment/data-{}.json", s));
-                then.status(200).json_body(json!([{
-                    "id": idx,
-                    "url": format!("https://example.com/#url/{}", s),
-                    "click_url": format!("https://example.com/#click/{}", s),
-                    "impression_url": format!("https://example.com/#impression/{}", s),
-                    "iab_category": "5 - Education",
-                    "icon": "1",
-                    "advertiser": "fake",
-                    "title": format!("Suggestion {}", s),
-                    "keywords": [s],
-                }]));
-            })
-            .await;
-    }
-}
-
 #[merino_test_macro(|settings| {
     // Wiki fruit is only enabled when debug is true.
     settings.debug = true;

diff --git a/merino-integration-tests/src/utils/test_tools.rs b/merino-integration-tests/src/utils/test_tools.rs
@@ -1,7 +1,7 @@
 //! Tools for running tests
 
 use crate::utils::{logging::LogWatcher, metrics::MetricsWatcher, redis::get_temp_db};
-use httpmock::MockServer;
+use httpmock::{Method::GET, MockServer};
 use merino_settings::Settings;
 use reqwest::{redirect, Client, ClientBuilder, RequestBuilder};
 use serde_json::json;
@@ -82,6 +82,14 @@ where
             }));
     });
     settings.remote_settings.server = remote_settings_mock.base_url();
+    settings_changer(&mut settings);
+    if let Some(changes) = &settings.remote_settings.test_changes {
+        setup_remote_settings_collection(
+            &remote_settings_mock,
+            &changes.iter().map(String::as_str).collect::<Vec<_>>(),
+        )
+        .await;
+    }
 
     // Set up Redis
     let _redis_connection_guard = match get_temp_db(&settings.redis.url).await {
@@ -96,8 +104,6 @@ where
         }
     };
 
-    settings_changer(&mut settings);
-
     // Setup metrics
     assert_eq!(
         settings.metrics.sink_host, "0.0.0.0",
@@ -114,8 +120,11 @@ where
     let address = listener.local_addr().unwrap().to_string();
     let redis_client =
         redis::Client::open(settings.redis.url.clone()).expect("Couldn't access redis server");
-    let server =
-        merino_web::run(listener, metrics_client, settings).expect("Failed to start server");
+    let providers = merino_web::providers::SuggestionProviderRef::init(&settings, &metrics_client)
+        .await
+        .expect("Could not create providers");
+    let server = merino_web::run(listener, metrics_client, settings, providers)
+        .expect("Failed to start server");
     let server_handle = tokio::spawn(server.with_current_subscriber());
     let test_client = TestReqwestClient::new(address);
 
@@ -197,3 +206,61 @@ impl TestReqwestClient {
         self.client.get(url)
     }
 }
+
+/// Create the remote settings collection and endpoint from the provided suggestions
+pub async fn setup_remote_settings_collection(server: &MockServer, suggestions: &[&str]) {
+    let mut changes = suggestions
+        .iter()
+        .map(|s| {
+            assert_ne!(*s, "icon");
+            json!({
+                "id": s,
+                "type": "data",
+                "last_modified": 0,
+                "attachment": {
+                    "location": format!("/attachment/data-{}.json", s),
+                    "hash": s,
+                }
+            })
+        })
+        .collect::<Vec<_>>();
+    changes.push(json!({
+        "id": "icon-1",
+        "type": "icon",
+        "last_modified": 0,
+        "attachment": {
+            "location": "/attachment/icon-1.png",
+            "hash": "icon"
+        }
+    }));
+
+    server
+        .mock_async(|when, then| {
+            when.method(GET)
+                .path("/v1/buckets/main/collections/quicksuggest/changeset");
+            then.status(200).json_body(json!({
+                "changes": changes,
+            }));
+        })
+        .await;
+
+    for (idx, s) in suggestions.iter().enumerate() {
+        server
+            .mock_async(|when, then| {
+                when.method(GET)
+                    .path(format!("/attachment/data-{}.json", s));
+                then.status(200).json_body(json!([{
+                    "id": idx,
+                    "url": format!("https://example.com/#url/{}", s),
+                    "click_url": format!("https://example.com/#click/{}", s),
+                    "impression_url": format!("https://example.com/#impression/{}", s),
+                    "iab_category": "5 - Education",
+                    "icon": "1",
+                    "advertiser": "fake",
+                    "title": format!("Suggestion {}", s),
+                    "keywords": [s],
+                }]));
+            })
+            .await;
+    }
+}
diff --git a/merino-settings/src/lib.rs b/merino-settings/src/lib.rs
@@ -177,6 +177,14 @@ pub struct RemoteSettingsGlobalSettings {
     /// - `http://127.0.0.1`
     /// - `https://firefox.settings.services.mozilla.com`
     pub server: String,
+
+    /// Only used for integration tests.
+    /// This field populates the mock returned remote settings collection
+    /// from inside the `merino_test_macro!(|settings| {...}) call.
+    // note `#[cfg(test)]` will cause a compile error as the
+    // `merino_test` proc_macro will not be able to find this
+    // field.
+    pub test_changes: Option<Vec<String>>,
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize)]

diff --git a/merino-web/Cargo.toml b/merino-web/Cargo.toml
@@ -5,12 +5,14 @@ edition = "2018"
 
 [dependencies]
 actix-cors = "0.6.0-beta.1"
+actix-rt = "2.3"
 actix-web = "=4.0.0-beta.8"
 actix-web-location = { version = "0.2", features = ["maxmind", "actix-web-v4", "cadence"] }
 anyhow = "1.0.40"
 async-recursion = "0.3"
 backtrace = "0.3"
 cadence = "0.26"
+futures = "0.3"
 futures-util = "0.3"
 lazy_static = "1.4.0"
 merino-adm = { path = "../merino-adm" }

diff --git a/merino-web/src/endpoints/providers.rs b/merino-web/src/endpoints/providers.rs
@@ -8,8 +8,6 @@ use actix_web::{
     web::{self, Data},
     HttpResponse,
 };
-use cadence::StatsdClient;
-use merino_settings::Settings;
 use merino_suggest::IdMultiProviderDetails;
 use serde::Serialize;
 
@@ -22,21 +20,8 @@ pub fn configure(config: &mut web::ServiceConfig) {
 #[get("")]
 async fn list_providers(
     provider: Data<SuggestionProviderRef>,
-    settings: Data<Settings>,
-    metrics_client: Data<StatsdClient>,
 ) -> Result<HttpResponse, HandlerError> {
-    let provider = provider
-        .get_or_try_init(settings.as_ref(), metrics_client.as_ref())
-        .await
-        .map_err(|error| {
-            tracing::error!(
-                ?error,
-                r#type = "web.suggest.setup-error",
-                "suggester error"
-            );
-            HandlerError::internal()
-        })?;
-
+    let provider = &provider.0;
     let providers = provider
         .list_providers()
         .into_iter()