Thanks for laying out this information @dbarratt.

We should bear in mind that this review will happen next quarter, once we've implemented something preliminary. We should also bear in mind that the approach we take will be constrained by the licenses; if they don't allow us to store data, we'll have to use option 3 - which is why we're taking that approach to begin with.

(Note that, although we're storing the MaxMind data, @aezell found that our license won't allow us to give our users information about other IP addresses from that dataset. Currently we only send details about their own IP address.)

With that in mind I'd suggest a few changes:

There are at least three ways to accomplish this: [...]

We should make it clear that this might be out of our hands, and highlight that we're particularly interested in discussing option 3, since this seems the most risky performance-wise, but also the most likely license-wise.

Are there any objects to any of the methods above?
If so, are there ways we can mitigate these objections?
Can you think of any other ways we might be able to solve this problem?
Which solution would your team prefer?

These questions might be a bit too broad by the time we have the review... We'll most likely have implemented a basic (though not finalised) version of option 3 by then. We may be in a position to discuss alternative approaches, but it'll depend on the licenses.

@Tchanders I added some clarity on which option we would be perusing and removed the questions. How does that look?

Tagging Performance-Team. We're at the start of the project so we'll add more details as we can, but wanted to request this sooner rather than later. Thanks.

In T260821#6453709, @Gilles wrote:

Since making an API that can return information about any IP address is probably out of the question, given that we shut down a service that did just that 4 years ago due to abuse.

The plan is to create an endpoint(s) that accept a log id or revision id (See T260603). Effectively users would only be able to lookup IP addresses that have made logged actions or revisions.

We may create an endpoint that would allow for any IP address, but this would be limited to use by privileged users (probably only checkusers), and would only exist until T261555 is completed.

Thanks for the information and suggestions @Gilles.

We had read about that previous service - thanks for bringing it up. We're planning to accept a log/revision ID as @dbarratt says. We'd also be adding an 'ipinfo' permission, that is only available to certain user groups (starting with checkusers). In this situation, do you think we'd still want to verify that the revision/log ID is present on the page?

The only reason it might accept an IP address would be for checkusers who are seeing the IP address of a logged-in user on Special:CheckUser or Special:Investigate - that IP address wouldn't be associated with a revision/log ID. However, the only reason to provide this tool on these pages would be for the checkusers' convenience, and not for the sake of IP masking. Unless this tool could provide better information than the information that checkusers can get by copying and pasting the IP address into their preferred tools, that would arguably be pointless. @Niharika - do you think we could revisit whether this should be available for checkusers investigating logged-in users' IP addresses? If not then it wouldn't need to accept IP addresses.

@aezell is currently discussing the MaxMind license terms, but under the present terms we would not be able to make use of our MaxMind dataset for this tool. I think one of the most significant questions here will be whether we are able to agree a license with MaxMind or any other service that allows us to do any type of storage/caching of their data. If not, then the third party would need to be queried on demand. Since we're having to start building this product before these questions are resolved, it seems sensible to me to assume the worst-case scenario for now, that we can't store any data. I suspect we'd be open to changing the product specifications a bit if it was felt that this was beyond what was acceptable performance-wise - e.g. not using hover (@Niharika or @Prtksxna might want to comment further).

@Niharika I suppose there's a tension here in that, from a product perspective we might prefer to use the service(s) with the highest quality data, but from a performance perspective we might prefer to use the service(s) with the most lenient license.

I'm told that MaxMind has a new competitor, https://db-ip.com/ which makes their dataset available in MaxMind's mmdb format: https://db-ip.com/db/ which means that the related tooling and libraries work with it. It's probably worth looking into their T&C as well. The free version of that database is CC BY licensed: https://db-ip.com/db/ip-to-location That could be a great starting point.

Chances are that this PHP library https://github.com/maxmind/GeoIP2-php pointed at the db-ip free database would perform well. Your new endpoint/API will need to be instrumented anyway, if the PHP library doesn't perform well it will be obvious.

In T260821#6461554, @Gilles wrote:

I'm told that MaxMind has a new competitor, https://db-ip.com/ which makes their dataset available in MaxMind's mmdb format: https://db-ip.com/db/ which means that the related tooling and libraries work with it. It's probably worth looking into their T&C as well. The free version of that database is CC BY licensed: https://db-ip.com/db/ip-to-location That could be a great starting point.

They have a number of competitors (See T248525). This isn't one I was aware of, but I can add it to my spreadsheet.

Chances are that this PHP library https://github.com/maxmind/GeoIP2-php pointed at the db-ip free database would perform well. Your new endpoint/API will need to be instrumented anyway, if the PHP library doesn't perform well it will be obvious.

It looks like we may be able to use our existing MaxMind license. I've created T262963 to have their library reviewed by the Security-Team in order to have it (and the dataset we currently pay for) deployed to production. :)

@aezell is currently discussing the MaxMind license terms, but under the present terms we would not be able to make use of our MaxMind dataset for this tool.

Update from @aezell is apparently that we can use it, so that's what we'll do - simplifies the problem a lot.

According to @Niharika we may want to look into using multiple services in the future, but not for the first iteration.

Unrelated to the performance review, but there seems to be a current limitation in ipv6 address comparison and 0-padding that I saw on the test wiki:

I see that the API responses don't contain any caching information:

While the JS seems to account for not re-requesting past API requests for a given IP in the current pageview, this means that after a page refresh or visiting another page with the same IP, you would be missing out on leveraging the browser cache. I think it would make sense to cache these API responses at least in the user agent, and it could be done for a long period, since the IP geo database isn't updated that frequently.

You should instrument the time it takes between a click on the (i) button and when the information is actually displayed. This can easily be done with mw.track. By measuring performance as experienced by real users this way, you will be able to see if there is any degradation once this feature gets more a lot more use once in production. You also get to track usage for free by doing this.

I see that the ResourceLoader module is correctly loaded only when dealing with the Special:Log page and a user who has the right privileges, that's great.

I would recommend against the placeholder animation with diagonal grey lines currently displayed before the results of the API request come in, since the API responses are usually very fast:

It's best to avoid drawing attention to short waits. Animated progress indicators become useful when the user is waiting for a long time (eg. waiting for the result of an expensive advanced search query). If you're worried that some users might wait 5-10 seconds for those API responses sometimes, then it's best to wait a couple of seconds before displaying the animation. This would ensure that most users (who will have a response under 2 seconds, for example) don't see the animation, and those who have very slow internet connections would see it, as they would be waiting a while for a result.

Looking at init.js, some of the variable initialisations could be done more lazily: https://github.com/wikimedia/mediawiki-extensions-IPInfo/blob/master/modules/ext.ipInfo/init.js#L4-L10 You're immediately looking for revIdAncestor and logIdAncestor in the DOM, as well as creating the button, when there are conditions - like not being a valid IP address - that will leave that unused. It's best to postpone doing those DOM lookups and object creation until you're 100% sure that they're going to be needed.

which will give all users the information from the varnish cache

Is the plan for this feature to be ultimately accessible to everyone, including logged-out users?

Is the plan for this feature to be ultimately accessible to everyone, including logged-out users?

No. This feature would be available to some logged-in users. We plan to make it available to checkusers only first and then potentially admins.

@Gilles Thanks for the thorough review - and not just of performance!

In T260821#6600060, @Gilles wrote:

Unrelated to the performance review, but there seems to be a current limitation in ipv6 address comparison and 0-padding that I saw on the test wiki:

We're working on this - filed as T266881 and T267015

I think it would make sense to cache these API responses at least in the user agent, and it could be done for a long period, since the IP geo database isn't updated that frequently.

Filed as T267230

You should instrument the time it takes between a click on the (i) button and when the information is actually displayed.

WIP task - T267235. This is WIP until the design is finalised (the button+popup might be changed).

I would recommend against the placeholder animation with diagonal grey lines currently displayed before the results of the API request come in, since the API responses are usually very fast

Filed as T267237

Looking at init.js, some of the variable initialisations could be done more lazily

Filed as T267240

which will give all users the information from the varnish cache

Is the plan for this feature to be ultimately accessible to everyone, including logged-out users?

I believe David was just thinking theoretically here.

Hi @Niharika: I see some movement in other tasks and such, is there a new expected timeline when Performance needs to do their review?

In T260821#7366619, @greg wrote:

Hi @Niharika: I see some movement in other tasks and such, is there a new expected timeline when Performance needs to do their review?

Oh yes! We are working on the MVP and hope to have the tool ready for soft launch in the next couple months. I am not exactly sure when we will be ready for a performance review so I'll ping @phuedx who knows better.