Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow use of GeoLite2-ASN database as ISP database #12874

Merged
merged 4 commits into from May 20, 2018
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented May 9, 2018

fixes #12871

@sgiehl sgiehl added the Needs Review PRs that need a code review label May 9, 2018
@sgiehl sgiehl added this to the 3.5.1 milestone May 9, 2018
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, some system tests appear to be failing now.

@diosmosis
Copy link
Member

Unrelated to this PR, but noticed in https://github.com/matomo-org/matomo/blob/3.x-dev/plugins/GeoIp2/GeoIp2.php#L26, we're checking if the current location provider is an instance of the GeoIp2 plugin. I think the full namepsace needs to be there. Missed this in last PR review.

@diosmosis
Copy link
Member

Tested locally, works, just need to fix the tests.

@sgiehl
Copy link
Member Author

sgiehl commented May 18, 2018

@diosmosis fixed the check and updated the tests.

@diosmosis diosmosis changed the title Adds possibility to use GeoLite2-ASN database as ISP database Allow use of GeoLite2-ASN database as ISP database May 20, 2018
@diosmosis diosmosis merged commit d7abde2 into 3.x-dev May 20, 2018
@diosmosis diosmosis deleted the geoip2asn branch May 20, 2018 19:24
@mackuba
Copy link

mackuba commented Jul 27, 2018

Hey, this be available in the next release, right?

@diosmosis
Copy link
Member

@mackuba this should be in 3.5.1, so should be in the latest release.

@mackuba
Copy link

mackuba commented Jul 28, 2018

@diosmosis ah, right, I've looked in the code on the server and the changes are there.

Is it necessary to enable the Provider plugin to see ISPs on the location reports page? I've dug into the code a little bit, and it looks like without the Provider plugin the location_provider column isn't even present in the database, but I've read somewhere that the Provider plugin is disabled by default, because it runs reverse DNS which is rather slow - but that shouldn't be needed at all if I get the provider info from the MaxMind database?... Does it make sense what I'm saying?

@diosmosis
Copy link
Member

Looks like the plugin has to be installed for the column to be in the table and for the provider to be looked up: https://github.com/matomo-org/matomo/blob/3.x-dev/plugins/UserCountry/Columns/Provider.php#L31. I think it also needs to be enabled in order to display the report. The reverse DNS logic shouldn't execute if the provider information is obtained via GeoIP2 (I believe).

I think otherwise you have to be using a Maxmind GeoIP implementation and the ASN database has to be named 'GeoLite2-ASN.mmdb' or 'GeoIP2-ISP.mmdb'.

@mackuba
Copy link

mackuba commented Jul 29, 2018

Great, thanks!

InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Adds possibility to use GeoLite2-ASN database as ISP database

* Adds some tests for GeoIP2 files

* fix tests

* fix check for GeoIp2 provider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.5.0] Download of the geoip2 ISP/ASN database fails
3 participants