Closed Bug 1797673 Opened 2 years ago Closed 2 years ago

Factor out Merino code into MerinoClient and add MerinoTestUtils

Categories

(Firefox :: Address Bar, task, P1)

task

Tracking

()

VERIFIED FIXED
108 Branch
Tracking Status
firefox108 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file)

Let's factor out the Merino client code into MerinoClient and add MerinoTestUtils.

This factors out the Merino client code from UrlbarProviderQuickSuggest into
MerinoClient. It also adds MerinoTestUtils.

There are a few reasons for this:

  1. Weather suggestions will be prefetched from Merino, and it may make sense to
    do the prefetching outside of UrlbarProviderQuickSuggest, I'm not sure
    yet.
  2. Weather suggestions will be fetched from a slightly different URL from other
    suggestions. They'll need to include providers=weather as a search param.
    In the future there may be other types of suggestions that also require
    different parameters or whole new endpoints. Supporting options like this is
    a little nicer with a dedicated Merino client class.
  3. In general this code is substantial enough that it makes sense to encapsulate
    it in its own file and class, even if the provider is the only thing that
    will use it.

This is a large patch but a lot of it moves code around. Summary of changes:

  • Factor out Merino client code into MerinoClient
  • Also include some new useful helpers in MerinoClient
  • Consolidate and factor out Merino test code into MerinoTestUtils
  • Also include some new useful helpers in MerinoTestUtils and make other improvements
  • Add a MockMerinoServer class (in MerinoTestUtils.sys.mjs) to help with setting up a mock Merino server
  • Add test_merinoClient.js to test the client in isolation. This file started as a copy of test_quicksuggest_merino.js because it's so similar, and some of the tests in the latter are moved to the former.
  • Add test_merinoClient_sessions.js to test Merino sessions in isolation. Similar to the previous point, this file started as a copy of test_quicksuggest_merinoSessions.js.
  • Modify existing Merino tests so they use MerinoTestUtils
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3e3ea87a6fce Factor out Merino code into MerinoClient and add MerinoTestUtils. r=daisuke
Regressions: 1798087
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch

STR for QA:

Please do some smoke tests for Merino integration as time allows.

  1. Set browser.urlbar.quicksuggest.dataCollection.enabled to true
  2. Set browser.urlbar.merino.enabled to true (should be true by default)
  3. Set browser.urlbar.quicksuggest.remoteSettings.enabled to false to disable suggestions from remote settings and isolate suggestions from Merino
  4. Try typing different keywords to trigger Suggest suggestions and verify they appear, e.g., "amazon", "nike", "betty"
  5. Restart Firefox and repeat the previous step
Flags: qe-verify+
Flags: in-testsuite+

I have verified this issue on the latest Nightly 108.0a1 (Build ID: 20221102174350) on Windows 10 x64, macOS 12.4 and Linux Mint 20.
Here are the results:
After following the STR from comment 4, only in some cases using a new profile, the "amazon", "nike" keywords don't trigger the sponsored results. I have verified multiple providers and found out that only some of them are triggered.
Sponsored results that appear: Homedepot, Michael Kors, Wayfair, Underarmour, Saksoff5th
Sponsored results that DON'T appear: Amazon, Statefarm, Overstock, Nike, Spanx, Chewy, Wix, Bloomingdales, 1800contacts, Groupon, Houzz
and macys.

However, after some time (~10 - 20 minutes), the rest of the Sponsored results were also correctly triggered and everything seems to work as expected.
I have also verified this on Firefox 108.0b7, and this is also reproducible, so I don't think that this bug introduced this issue. Are there any new changes on Merino side that might have caused this?

Status: RESOLVED → VERIFIED
Flags: qe-verify+

I asked Nan and he said Merino experienced high network latency around 13:10–13:40 UTC. Was that about the time you were testing? High latency would cause the client to time out and not show a Merino suggestion. The timeout period is only 200ms, and you might be more likely to hit it in Europe. That would be consistent with your experience where sometimes suggestions appeared and sometimes not -- some fetches happened to time out and some didn't. So that would be my guess.

There are two things you can do to verify if it happens again:

  • Set browser.urlbar.loglevel to Debug to turn on logging and then filter on "Merino" in the browser console to see if any timeouts are logged.
  • Set browser.urlbar.merino.timeoutMs to a large value in milliseconds, e.g., 5000 for 5 seconds, and see if the issue still happens.

Thanks for raising this!

@Drew @Nan, I have retested this, and on my machine, I can still reproducible this even if I change the timoutMS to 5000. We have verified this on 4 different machines, with a different internet connection, and we managed to reproduce this on two machines. However, after ~10-20 minutes, all the Sponsored results seem to be correctly triggered.

Also if there is a problem with latency, shouldn't the "FX_URLBAR_MERINO_RESPONSE" histogram catch this? Here is a screenshot with MERINO histograms: Link.

I have made a screen recording with the browser console opened. You can find the recording here: Link.

Flags: needinfo?(najiang)
Flags: needinfo?(adw)

Thanks, Cosmin!

I managed to reproduce this issue locally and confirmed this issue was not caused by this patch, but rather a Merino internal issue. I will be investigating this, will comment here once I have more details.

Flags: needinfo?(najiang)

Turns out that Merino experienced timeouts when fetching suggestions from Remote Settings. That only happened to some Merino instances which is why this issue seems intermittent.

We have made a hotfix for this tracked via CONSVC-2099.

@Cosmin Thanks again for reporting this!

This issue is now fixed in production.

Blocks: 1799265
Flags: needinfo?(adw)
Blocks: 1815018
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: