Factor out Merino code into MerinoClient and add MerinoTestUtils
Categories
(Firefox :: Address Bar, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox108 | --- | verified |
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Let's factor out the Merino client code into MerinoClient and add MerinoTestUtils.
Assignee | ||
Comment 1•2 years ago
|
||
This factors out the Merino client code from UrlbarProviderQuickSuggest into
MerinoClient. It also adds MerinoTestUtils.
There are a few reasons for this:
- Weather suggestions will be prefetched from Merino, and it may make sense to
do the prefetching outside of UrlbarProviderQuickSuggest, I'm not sure
yet. - Weather suggestions will be fetched from a slightly different URL from other
suggestions. They'll need to includeproviders=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. - 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
Comment 3•2 years ago
|
||
bugherder |
Assignee | ||
Comment 4•2 years ago
|
||
STR for QA:
Please do some smoke tests for Merino integration as time allows.
- Set
browser.urlbar.quicksuggest.dataCollection.enabled
to true - Set
browser.urlbar.merino.enabled
to true (should be true by default) - Set
browser.urlbar.quicksuggest.remoteSettings.enabled
to false to disable suggestions from remote settings and isolate suggestions from Merino - Try typing different keywords to trigger Suggest suggestions and verify they appear, e.g., "amazon", "nike", "betty"
- Restart Firefox and repeat the previous step
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?
Assignee | ||
Comment 6•2 years ago
|
||
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
toDebug
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.
Comment 8•2 years ago
|
||
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.
Comment 9•2 years ago
|
||
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!
Comment 10•2 years ago
|
||
This issue is now fixed in production.
Assignee | ||
Updated•2 years ago
|
Description
•