Fixes #3656 - Top sites priority change

[ksy36]

The new script fetches results from both Tranco (global) and Alexa (regional) and saves results in 2 dbs. Regional db only includes domains whose priority is higher than in global db, therefore it's much smaller. I have decided to store the rankings in 2 dbs since once Alexa's API is deprecated, we can only update the global db and preserve the regional one for some time.

The script can be run is this way:
python3 ./tools/fetch_topsites.py --retrieve-regional --ats_access_key=<access_key> --ats_secret_key=<secret_key>

and works as follows:

1. Gets top 10k csv from Tranco and saves it to the "global sites" db and cache.
2. Optionally fetches top 100 sites from specified regions if --retrieve-regional parameter is passed and saves the results in "regional sites" db.

[ksy36]

This class is based on the code from the original topsites.py

[ksy36]

r? @karlcow

[karlcow]

Thanks @ksy36
Big update! Impressive.

I'm a bit concerned by the amount of mock we do for testing the DB. It's code inherited, so probably it would be good to open a new issue on this for the future. That could help us to probably test some weird scenarios.

There's also probably an optimization which would be good to do on the queries in a for loop which will return only one result.

Request Changes just for getting the discussion rolling and see your thoughts.

[karlcow]

I wonder why we left host_name instead of hostname in there. Left over from a previous version?

[ksy36]

Yeah, I've left this part untouched from the previous version. Updated now though :)

[karlcow]

I don't remember why the code is using a for loop. Maybe an opportunity to fix it.

Do we expect regional_site_db.query(SiteRegional).filter_by(url=hostname) to return more than one domain? and it would mean that even if there was more than one domain, only the first hit would be taken into consideration, because of the return.

Basically the code is currently the equivalent of

sites = ['A', 'B', 'C', 'A2', 'C']
def extract(sites):
    for site in sites:
        if site.startswith('A'):
            return site

which would return 'A' always.

Should we just have something like.

Suggested change

	for site in regional_site_db.query(SiteRegional).filter_by(url=hostname): # noqa
	return f'priority-{priorities[site.priority - 1]}'
	site = regional_site_db.query(SiteRegional).filter_by(url=hostname).first()
	if not site:
	site = global_site_db.query(SiteGlobal).filter_by(url=hostname).first()
	if site:
	return f'priority-{priorities[site.priority - 1]}'
	#… and so on for the subdomains.

.first() returns None if the row doesn't exist.

or something else.

Maybe there is a function to extract here. because the code is repeating the code in a loop.

Or maybe I totally misunderstood the code 🍭 Long time I have touched this part.

[ksy36]

The domain names are unique yes, so using .first() will work indeed. Thanks for the suggestion, I've changed the function.

[karlcow]

The fact that we do it here again makes me think there is an opportunity for a function to reduce it and test it.

[karlcow]

For the future, maybe there should be two mock DBs covering what we want instead of mocking. or in the setup a dictionary with data creating the mock DB. to think. Nothing to do now.

[ksy36]

Yeah, good idea, I'll file an issue for that.

[karlcow]

Another thought, this was initially created like this I guess because of priority market for Firefox, but we can imagine that other browsers could have a desire to extend this. Nothing to do now.

[karlcow]

Should the code have a failsafe feature. Either based on the date. "Hey it's past December 15, 2022. We will ignore the request." Or based on failing the request to Alexa.

[ksy36]

Thanks, I've added the check 👍

[ksy36]

Could you please take another look @karlcow? I've refactored extract_priority_label and added the check for Alexa deprecation date.

[karlcow]

Thanks a lot @ksy36 for addressing the changes. 🔝 🥳

[ksy36]

Thanks!!