Improve the test to decide which search results to serve #47128
Conversation
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
k8s-ci-robot
added
do-not-merge/work-in-progress
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
cjyabraham
changed the title
cjyabraham
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
|
/area localization |
k8s-ci-robot
added
area/localization
|
I should clarify: I think the proposed method in this PR will work @cjyabraham, and we should test and review it. I was looking at other alternatives that don't try to load anything from Google at all, but we may not need to go that far. |
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
There was a problem hiding this comment.
This update feels pretty performant. It's a touch slow (like 1.5 to 2 counts) when I set the can_google cookie to false, but afterward, on subsequent reloads, PageFind loads as quickly as a regular search does. I haven't been able to hit the full 5-second timeout (which, if it were common, would be too long to wait).
/lgtm
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 6a3a2c8766e44e70480ac3f3c3f514b023fbe995
|
|
Tested in Chrome with and without uBlock Origin on. Same results as Nate, only about a 1-1.5 second wait on Pagefind results the first time the cookie was set to false. I'm happy with the performance there. It's lightweight and relatively performant. This feels like a good solution. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: katcosgrove The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
I was going to remove the |
|
Please do send in that further PR @cjyabraham (not urgent). |
|
Here it is: #47314 |
This addresses one of the issues that came from #47108
In deciding what search results to serve, we had previously been checking to see whether a user was in China using the ipinfo.io service. The problem, however, is that many people who are not in China are running privacy software that blocks this service and so they were incorrectly being deemed to be in China. This means that a lot of people would be getting PageFind search results when it would have been fine to serve them the Google search results.
Additionally, there are people in China who are using VPNs so would be able to access Google search results but our system was instead serving them the inferior PageFind results.
This PR moves this test away from a geolocation of whether someone is in China and focuses instead on whether the user's browser can access Google. This is the only thing that matters anyway. If they can access Google, then they get the Google search results, otherwise it falls back to Pagefind search results.
For now, people can check the dev console to see debug output of either "Google is blocked" or "Google is NOT blocked" which will make testing easier. Before merge, we can remove these logs. After the first page load, a cookie is set and it will no longer perform the test, but you can try it again in an incognito window or by deleting the
can_googlecookie using dev tools.It would be good to test this thoroughly with people inside and outside of China and have some people running privacy blocking plugins in their browsers. In all cases it should correctly determine whether you can access Google and will then serve the appropriate search results.