Page MenuHomePhabricator

Make some improvements to Logger::debouncedLog function
Closed, ResolvedPublic2 Estimated Story Points

Description

Background

Logger::debouncedLog is defined here: https://gerrit.wikimedia.org/g/mediawiki/extensions/IPInfo/+/02f7561b060ab0f97e37e78e85ae9162ca9262f9/src/Logging/Logger.php#206

It checks whether a log entry has already been made within the last 24 hours (by default), and prevents logging if so.

We should make some improvements, as outlined in T303034#7752841:

It acquire an actor id for read. If the function has to acquire a new id (because there is no) then the next select on the logging table would not find anything, because there is nothing, findActorId is enough here.

There is no need to select the whole and exact COUNT(*) to only find there is nothing (code compares with 0). Just check if here is a match for one row and treat no match as false (Using selectRow with '1' - https://codesearch.wmcloud.org/search/?q=selectRow%5Cs*%5C(%5B%5E%2C%5D%2B%2C%5Cs*%271%27&i=nope&files=%5C.php&excludeFiles=&repos=).

Event Timeline

@Umherirrender I've left this out of the task description (also from T303034#7752841):

LIKE queries with % at the begin are not good from performance view. Is the check for the level necessary? It seems that log entry always has the level set. This is not a big deal, because there are so much fields in the WHERE this does not make performance worse.

We do need to make sure we don't repeat the log if the level is the same, but that we make a new log entry if the level is different. We shouldn't have to check more than 2 rows at this point.

STran set the point value for this task to 2.Mar 14 2022, 5:08 PM

Change 773741 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/IPInfo@master] Improve Logger::debouncedLog

https://gerrit.wikimedia.org/r/773741

Change 773741 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Improve Logger::debouncedLog

https://gerrit.wikimedia.org/r/773741

dom_walden subscribed.

I did some regression testing of the logging functionality.

I looked up a load of logs and revisions via IPInfo and checked that exactly one log entry was made per IP.

I repeated this less than 24 hours later and checked that no new log entries were made.

I wasn't sure under what circumstances a user would not have an actor id. I am not sure it is possible, so I didn't test this case.

Test Environment: https://test.wikipedia.org IP Info 0.0.0 (a37fa42) 17:03, 28 March 2022.