Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

feat: initialize suggestion providers at start up #193

Merged
merged 9 commits into from
Nov 9, 2021
Merged
Show file tree
Hide file tree
Changes from 6 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
21 changes: 11 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions merino-adm/src/remote_settings/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions merino-adm/src/remote_settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,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;
Expand All @@ -73,7 +72,6 @@ impl RemoteSettingsSuggester {
}
});
}

Ok(Box::new(Self { suggestions }))
}

Expand All @@ -98,7 +96,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
Expand Down
80 changes: 6 additions & 74 deletions merino-integration-tests/src/suggest.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
//! Tests Merino's ability to make basic suggestions.
// Note, setting the following line will cause a compile error. This may be due to the
// fact that the proc_macro may not be able to locate items.
jrconlin marked this conversation as resolved.
Show resolved Hide resolved
#![cfg(test)]

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,
Expand Down Expand Up @@ -130,16 +131,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
Expand All @@ -157,16 +151,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);
Expand All @@ -180,63 +169,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;
Expand Down
77 changes: 72 additions & 5 deletions merino-integration-tests/src/utils/test_tools.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -96,8 +104,6 @@ where
}
};

settings_changer(&mut settings);

// Setup metrics
assert_eq!(
settings.metrics.sink_host, "0.0.0.0",
Expand All @@ -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);

Expand Down Expand Up @@ -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;
}
}
8 changes: 8 additions & 0 deletions merino-settings/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
2 changes: 2 additions & 0 deletions merino-web/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
Loading