@sgiehl opened this Pull Request on February 12th 2018 Member
  • [x] (visually) rename current GeoIP location providers to GeoIP Legacy
  • [x] Deprecate GeoIP Legacy
    • [x] Hide providers once a GeoIP 2 provider has been activated
  • [x] Implement GeoIP2 location provider
    • [x] PHP (w/ php extension)
    • [x] Apache / Nginx ?
  • [x] Implement new auto updater to update GeoIP2 databases
  • [x] Change UI to download/update GeoIP2 databases instead of GeoIP Legacy
    • [x] GeoIP Legacy updater should still work until switched to GeoIP2
    • [x] Show information about configured GeoIP Legacy updater until switched to GeoIP2 (without possibility to change config)
  • [x] Provide mapping from iso region codes to region names
    -> done using a console command to import region names from debian/iso-codes package
  • [x] Update tests to work with GeoIP2, but maybe leave some GeoIP legacy tests
  • [ ] Implement migration script that needs to be run after update
    • [x] old region code (FIPS) needs to be updated to new ones (ISO)
    • [x] Should also update old region/country code for Tibet [ti|1 => cn|14] (refs #11930)
    • [x] Should remove old custom country codes of GeoIP Legacy (such as AP, EU, A1, A2)
    • [ ] Make it somehow possible to run migration in browser (without possible timeouts)
    • [ ] How will old archives be updated?
  • [ ] ~Adjust VisitorGenerator plugin to fake new region codes~ not needed

fixes #4487
refs #12411

@mattab commented on March 19th 2018 Owner

Good progress :+1: from the check list it looks like all the work is done, as we don't really need "trigger update in browser", let's skip this one. Is the work complete and ready to be reviewed & merged?

@sgiehl commented on March 19th 2018 Member

Almost. I will update the list of tasks. But I would disagree with "update in browser". As we don't do the update within the normal update progress we make it impossible to migrate for users without access to the console. Which applies for most users on a shared hosting,

@sgiehl commented on March 19th 2018 Member

I've now finished updating the FIPS => ISO mapping manually. All old codes should now be in the list.
The list of region names currently included may lack some names, as it is currently generated using data from iso-codes. And unfortunately their list seems not to be very up to date. (But I've already added some missing entries, where I've seen missing data while testing)
If we want to have a complete list of names, we might need to purchase the official ISO standard.
I will now have another look at all changes and afterwards it should be good for a first review

@mattab commented on March 20th 2018 Owner

@sgiehl Please find my first review and some questions below:

  • [ ] The PR seems to introduce some schema changes:
    ALTER TABLE `piwik_log_visit` MODIFY COLUMN `location_region` char(3) DEFAULT NULL;
    ALTER TABLE `piwik_log_conversion` MODIFY COLUMN `location_region` char(3) DEFAULT NULL;

    -> as we can't introduce schema changes on log_* table within a major release cycle, should we move the schema changes to Matomo 4.0.0 milestone? Or what are our options ie. is it totally broken if we keep 2 chars region codes?

  • [ ] Does Geoip2 still support ISP and ORG databases (covered in faq) and if so, should we update any of our documentation or in-app product help?
  • [ ] Are you already planning to update our existing FAQs and user guide or should we schedule it after the merge?
  • [ ] need to create a new issue in matomo 4 for Geolocation: remove the GeoIP Legacy Support
  • [ ] before we merge, it's important that we do a performance test of the new geoip2. (we need to make sure it won't introduce slowdowns, or that we know about them and fix it, eg. report any performance issue upstream.) -> Could you report the performance of a geolocation lookup, within Matomo tracker, of Geoip1 VS Geoip2 both by themselves, and also under heavy load?
@sgiehl commented on March 20th 2018 Member

Schema change is required as GeoIP2 returns ISO region codes and those can have up to three characters. So if we want to switch to ISO codes we need to change the schema. Converting the ISO data returned by GeoIP2 to the old FIPS codes does not seem to be a good option. I would estimate that it takes around 3 days to create a proper mapping list. Also we would loose some information as not all ISO codes can be mapped to FIPS. So imho we should move GeoIP2 to Matomo 4, if a schema change can't be done before.

GeoIP 2 ISP/ORG databases are still supported, but they have been merged into one. See https://www.maxmind.com/en/geoip2-isp-database

We should not (yet) remove GeoIP legacy support. Maxmind will still provide updates for the paid databases. Only updates for GeoIP Lite are discontinued.

Let's update the FAQs as soon as we have a plan when this will be merged.

Regarding performance: Would need to set up some tests to check how fast the implementations are.

@mattab commented on March 23rd 2018 Owner

As discussed internally here is a possible way to solve the upgrade path & our inability to introduce a schema change to existing users:

  • we Introduce a new plugin and move there all the logic to tracking and reporting the users' regions.
  • This plugin is disabled by default for people who upgrade not to get the big schema update, but enabled by default for new users as they directly install the CHAR 3 column.
  • For users who upgrade and when the User Region plugin is not activated, then we could display a notification explaining "User Region tracking and reportin has been de-activated. You can re-enable this feature manually here, or run the following command to activate the feature: ./console ....".

Shall we proceed with this solution? Looking forward to merge the work and allowing users to leverage much increased accuracy of Geoip2!

@sgiehl commented on March 23rd 2018 Member

Just realized switching to FIPS region codes will break the visitor map. The map currently handles FIPS codes, and I don't think that can be easily changed...

@sgiehl commented on April 5th 2018 Member

will be replaced by #12699

This Pull Request was closed on April 5th 2018
Powered by GitHub Issue Mirror