Bug 1928287 - Cache Merino geolocation for a brief period so we don't ping it on every keystroke. r=daisuke, a=dmeehan

Depends on D227301

Differential Revision: https://phabricator.services.mozilla.com/D227439

browser/components/urlbar/QuickSuggest.sys.mjs

@@ -39,6 +39,13 @@ const FEATURES = {
   YelpSuggestions: "resource:///modules/urlbar/private/YelpSuggestions.sys.mjs",
 };
 
+// How long we'll cache Merino's geolocation response. This is intentionally a
+// small period of time, and keep in mind that this manual caching layer here is
+// on top of any HTTP caching done by Firefox according to the response's cache
+// headers. The point here is to make sure that, regardless of HTTP caching, we
+// don't ping Merino geolocation on each keystroke since that would be wasteful.
+const GEOLOCATION_CACHE_PERIOD_MS = 120000; // 2 minutes
+
 const TIMESTAMP_TEMPLATE = "%YYYYMMDDHH%";
 const TIMESTAMP_LENGTH = 10;
 const TIMESTAMP_REGEXP = /^\d{10}$/;
@@ -270,7 +277,9 @@ class _QuickSuggest {
 
   /**
    * Fetches the client's geolocation from Merino. Merino gets the geolocation
-   * by looking up the client's IP address in its MaxMind database.
+   * by looking up the client's IP address in its MaxMind database. We cache
+   * responses for a brief period of time so that fetches during a urlbar
+   * session don't ping Merino over and over.
    *
    * @returns {object}
    *   An object with the following properties (see Merino source for latest):
@@ -295,20 +304,30 @@ class _QuickSuggest {
    *       Accuracy radius in km.
    */
   async geolocation() {
-    if (!this.#merino) {
-      this.#merino = new lazy.MerinoClient("QuickSuggest");
+    if (
+      !this.#cachedGeolocation?.geolocation ||
+      this.#cachedGeolocation.dateMs + GEOLOCATION_CACHE_PERIOD_MS < Date.now()
+    ) {
+      if (!this.#merino) {
+        this.#merino = new lazy.MerinoClient("QuickSuggest");
+      }
+
+      this.logger.debug("Fetching geolocation from Merino");
+      let results = await this.#merino.fetch({
+        providers: ["geolocation"],
+        query: "",
+      });
+
+      this.logger.debug(
+        "Got geolocation from Merino: " + JSON.stringify(results)
+      );
+      this.#cachedGeolocation = {
+        geolocation: results?.[0]?.custom_details?.geolocation || null,
+        dateMs: Date.now(),
+      };
     }
 
-    this.logger.debug("Fetching geolocation from Merino");
-    let results = await this.#merino.fetch({
-      providers: ["geolocation"],
-      query: "",
-    });
-
-    this.logger.debug(
-      "Got geolocation from Merino: " + JSON.stringify(results)
-    );
-    return results.length ? results[0].custom_details.geolocation : null;
+    return this.#cachedGeolocation.geolocation;
   }
 
   /**
@@ -550,6 +569,10 @@ class _QuickSuggest {
     }
   }
 
+  _test_clearCachedGeolocation() {
+    this.#cachedGeolocation = { geolocation: null, dateMs: 0 };
+  }
+
   // Maps from Suggest feature class names to feature instances.
   #features = {};
 
@@ -570,6 +593,13 @@ class _QuickSuggest {
 
   // `MerinoClient`
   #merino;
+
+  #cachedGeolocation = {
+    // The cached geolocation object from Merino.
+    geolocation: null,
+    // The date the geolocation was cached as reported by `Date.now()`.
+    dateMs: 0,
+  };
 }
 
 export const QuickSuggest = new _QuickSuggest();

browser/components/urlbar/tests/quicksuggest/unit/test_weather.js

@@ -717,17 +717,115 @@ async function doCityTest({ desc, query, geolocation, expected }) {
     expected.weatherParams.q ??= "";
   }
 
-  let callCountsByProvider = {};
+  QuickSuggest._test_clearCachedGeolocation();
+  let callsByProvider = await doSearch({
+    query,
+    geolocation,
+    suggestionCity: expected?.suggestionCity,
+  });
 
+  // Check the Merino calls.
+  Assert.equal(
+    callsByProvider.geolocation?.length || 0,
+    expected?.geolocationCalled ? 1 : 0,
+    "geolocation provider should have been called the correct number of times"
+  );
+  Assert.equal(
+    callsByProvider.accuweather?.length || 0,
+    expected ? 1 : 0,
+    "accuweather provider should have been called the correct number of times"
+  );
+  if (expected) {
+    for (let [key, value] of Object.entries(expected.weatherParams)) {
+      Assert.strictEqual(
+        callsByProvider.accuweather[0].get(key),
+        value,
+        "Weather param should be correct: " + key
+      );
+    }
+  }
+}
+
+// We should cache the geolocation returned by Merino.
+add_task(async function cachedGeolocation() {
+  let query = "waterloo";
+  let geolocation = {
+    location: {
+      latitude: 41.0,
+      longitude: -93.0,
+    },
+  };
+
+  QuickSuggest._test_clearCachedGeolocation();
+
+  // Do a search. We should call Merino and cache the response.
+  info("Doing first search");
+  let callsByProvider = await doSearch({
+    query,
+    geolocation,
+    suggestionCity: "Waterloo",
+  });
+  info("First search callsByProvider: " + JSON.stringify(callsByProvider));
+  Assert.equal(
+    callsByProvider.geolocation.length,
+    1,
+    "geolocation provider should have been called on first search"
+  );
+
+  // Do a second search. We should use the cached geolocaton, so we shouldn't
+  // call Merino.
+  info("Doing second search");
+  callsByProvider = await doSearch({
+    query,
+    geolocation,
+    suggestionCity: "Waterloo",
+  });
+  info("Second search callsByProvider: " + JSON.stringify(callsByProvider));
+  Assert.ok(
+    !callsByProvider.geolocation,
+    "geolocation provider should not have been called on second search"
+  );
+
+  // Set the date forward 3 minutes, which is longer than the cache period.
+  let futureMs = Date.now() + 3 * 60 * 1000;
+  let sandbox = sinon.createSandbox();
+  let dateNowStub = sandbox.stub(
+    Cu.getGlobalForObject(QuickSuggest).Date,
+    "now"
+  );
+  dateNowStub.returns(futureMs);
+
+  // Do a third search. The previous response is cached but it's stale, so we
+  // should call Merino again.
+  info("Doing third search");
+  callsByProvider = await doSearch({
+    query,
+    geolocation,
+    suggestionCity: "Waterloo",
+  });
+  info("Third search callsByProvider: " + JSON.stringify(callsByProvider));
+  Assert.equal(
+    callsByProvider.geolocation.length,
+    1,
+    "geolocation provider should have been called on third search"
+  );
+
+  sandbox.restore();
+  QuickSuggest._test_clearCachedGeolocation();
+});
+
+async function doSearch({ query, geolocation, suggestionCity }) {
+  let callsByProvider = {};
+
+  // Set up the Merino request handler.
   MerinoTestUtils.server.requestHandler = req => {
     let params = new URLSearchParams(req.queryString);
     let provider = params.get("providers");
+    callsByProvider[provider] ||= [];
+    callsByProvider[provider].push(params);
 
-    callCountsByProvider[provider] ||= 0;
-    callCountsByProvider[provider]++;
-
+    // Handle geolocation requests.
     if (provider == "geolocation") {
-      // Return the geolocation response.
       return {
         body: {
           request_id: "request_id",
@@ -742,41 +840,20 @@ async function doCityTest({ desc, query, geolocation, expected }) {
       };
     }
 
-    // Return the accuweather response.
-
+    // Handle accuweather requests.
     Assert.equal(
       provider,
       "accuweather",
-      "Firefox should have called the 'accuweather' Merino provider"
+      "Sanity check: If the request isn't geolocation, it should be accuweather"
     );
-
-    // If this fails, the Rust component returned multiple suggestions, which is
-    // fine and expected when a query matches multiple cities, but then we went
-    // on to make urlbar results for more than one of them, which is not fine.
-    // We should only ever make a result for one suggestion.
-    Assert.equal(
-      callCountsByProvider.accuweather,
-      1,
-      "accuweather provider should be called at most once"
-    );
-
-    for (let [key, value] of Object.entries(expected.weatherParams)) {
-      Assert.strictEqual(
-        params.get(key),
-        value,
-        "Weather param should be correct: " + key
-      );
-    }
-
     let suggestion = { ...WEATHER_SUGGESTION };
-    if (expected.suggestionCity) {
+    if (suggestionCity) {
       suggestion = {
         ...suggestion,
-        title: "Weather for " + expected.suggestionCity,
-        city_name: expected.suggestionCity,
+        title: "Weather for " + suggestionCity,
+        city_name: suggestionCity,
       };
     }
-
     return {
       body: {
         request_id: "request_id",
@@ -786,34 +863,22 @@ async function doCityTest({ desc, query, geolocation, expected }) {
   };
 
   // Do a search.
-  let context = createContext(query, {
-    providers: [UrlbarProviderQuickSuggest.name],
-    isPrivate: false,
-  });
   await check_results({
-    context,
-    matches: !expected
+    context: createContext(query, {
+      providers: [UrlbarProviderQuickSuggest.name],
+      isPrivate: false,
+    }),
+    matches: !suggestionCity
       ? []
       : [
           QuickSuggestTestUtils.weatherResult({
-            city: expected.suggestionCity,
+            city: suggestionCity,
           }),
         ],
   });
 
-  // Check Merino call counts.
-  Assert.equal(
-    callCountsByProvider.geolocation || 0,
-    expected?.geolocationCalled ? 1 : 0,
-    "geolocation provider should have beeen called the correct number of times"
-  );
-  Assert.equal(
-    callCountsByProvider.accuweather || 0,
-    expected ? 1 : 0,
-    "accuweather provider should have beeen called the correct number of times"
-  );
-
   MerinoTestUtils.server.requestHandler = null;
+  return callsByProvider;
 }
 
 function assertDisabled({ message }) {