Page MenuHomePhabricator

Re-enable filter profiling on every wiki
Closed, ResolvedPublic

Description

With T166802, T177017 and their subtasks, the filter profiling is slowly being re-added to wikis. In fact, from the time it was disabled, it's now more performant and will possibly be even better after patches like this will be merged. So I'm proposing to set both $wgAbuseFilterProfile and $wgAbuseFilterRuntimeProfile true by default (and also rename them), to give a heavy help to filter mantainers. Of course, the patch itself is straightforward to write, but I'd first like to hear some thoughts, although I think the reasons that caused profiling to be disabled don't exist anymore. Adding as subscribers some people from previous related tasks.

Current status (Jan 24th)

Profiling is now enabled everywhere in production. We should wait a couple of weeks to see if any problem arises, especially the ones outlined in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/211951/. If everything is fine, the next step is https://gerrit.wikimedia.org/r/486279 + https://gerrit.wikimedia.org/r/486280 to remove the global from AbuseFilter and WMF config.
In the meanwhile, we have to understand whether enabling $wgAbuseFilterRuntimeProfile everywhere would be safe. If so, we should then follow a similar process: enable it for every wiki, wait a couple of weeks, then if everything is fine remove it from AbuseFilter and WMF config. Regardless of this, https://gerrit.wikimedia.org/r/486284 should be merged to add some more light stats.

Questions for Performance-Team

  • Is Xenon happy with profiling, currently enabled everywhere? The recording method is AbuseFilter::recordProfilingResult, and what it does is get and set 3 stash keys for every active filter for every action (edit, move, delete, account (auto)creation and upload), in a deferredupdate. (See "future plans" below for improvements)
  • Would Graphite be OK with new profiling data from every wiki? The recording method is AbuseFilter::recordRuntimeProfilingResult, and what it does is update 3 statsd keys for every edit. See here a list of wikis where this is already in place.

Future plans

The final goal of this task is to have all profiling enabled by default and drop profiling globals. Once that will be done, patches like https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/478232/ will be easier to write and, starting with https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/201104/, we'll add profiling data on-wiki, possibly making it more efficient. The latter will also change the profiling system so that recordProfilingResult will only use a single stash key for every filter.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Daimona triaged this task as Medium priority.Mar 29 2018, 4:48 PM

Change 423660 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Re-enable filter profiling on every wiki

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

As I wrote in the commit, the problems which caused profiling to be disabled don't exist anymore, thus I can't see any reason to keep it disabled. Anyway, I'll still wait some time before putting this in a SWAT window.

This means that a lot of data will start flooding graphite and I'm not sure we are set to handle that much load. Someone in analytics(?) or performance(?) should +1 this before it goes out. Maybe @aaron or @Nuria? I don't really know who to ping here.

@dmaza Yeah, I suspected it. However, since profiling has been enabled for some time in the past, I believe we should be able to re-enable it without specific problems. Or is there something different since then that requires attention?

I'm not sure, as @dmaza said, that we want the RunTime profiling thing enabled everywhere so I'd leave the RunTimeProfiling section untouched for now. As for $wgAbuseFilterProfile I think that indeed we should enable it as it is useful for maintenance purposes, but IIRC this was not enabled by default due to performance reasons. As such, I wonder if we should add someone from Performance-Team / Performance-Team / Wikimedia-Performance-publish here to sign-off? Thanks.

@MarcoAurelio performance troubles should be solved, AFAIK profiling doesn't imply performance drops from users' POV. Let's wait for thoughts about the RunTimeProfiling.

I'm splitting the patch to separately enable wgAbuseFilterProfile and wgAbuseFilterRuntimeProfile. Since it seems easier to implement the former, which is also the one needed for https://gerrit.wikimedia.org/r/#/c/201104/, I'd propose to start by enabling it.

Change 423945 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Enable $wgAbuseFilterRuntimeProfile on every wiki

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

About wgAbuseFilterProfile, I'd like to wait for T191430 to be solved before enabling it. If there aren't opposing opinions, I'll put it in the first SWAT window available after that task will be solved.

Any news about this? We're still waiting for the above task to be solved before enabling the per-filter profiling, but it'd be good to also enable RuntimeProfile in order to give more uniform stats with other tasks.

After several months, profiling now works again for stashed edits, which means that we don't have any further blocker on this. I've scheduled the patch for enabling $wgAbuseFilterProfile everywhere for tomorrow's SWAT. Do we have any news about the other one?
Ideally, in the end we should have either a single config variable to control both options. Actually, it'd be even better to remove such globals and make profiling always enabled: after all, it's disabled only because it used to suffer from performance troubles. With T193064 we could actually remove data from graphite/grafana, too.

Change 423660 merged by jenkins-bot:
[operations/mediawiki-config@master] Enable $wgAbuseFilterProfile on every wiki

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

Mentioned in SAL (#wikimedia-operations) [2019-01-24T12:17:55Z] <zfilipin@deploy1001> Synchronized wmf-config/abusefilter.php: SWAT: [[gerrit:423660|Enable $wgAbuseFilterProfile on every wiki (T191039)]] (duration: 00m 54s)

Change 486279 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove $wgAbuseFilterProfiling

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

Change 486280 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Remove $wgAbuseFilterProfile

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

Current status

Profiling is now enabled everywhere in production. We should wait a couple of weeks to see if any problem arises, especially the ones outlined in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/211951/. If everything is fine, the next step is https://gerrit.wikimedia.org/r/486279 + https://gerrit.wikimedia.org/r/486280 to remove the global from AbuseFilter and WMF config.
In the meanwhile, we have to understand whether enabling $wgAbuseFilterRuntimeProfile everywhere would be safe. If so, we should then follow a similar process: enable it for every wiki, wait a couple of weeks, then if everything is fine remove it from AbuseFilter and WMF config.

Change 486284 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Save slow filters data without checking wgAbuseFilterRuntimeProfile

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

Change 486469 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove $wgAbuseFilterRuntimeProfiling

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

Change 486470 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Remove $wgAbuseFilterRuntimeProfile

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

Daimona raised the priority of this task from Medium to High.Jan 25 2019, 3:42 PM

The only thing that's really left to do is understand if profiling could still cause performance problems. All patches here are trivial to review. Furthermore, removing profiling globals would open the way for a couple of important patches (mentioned in the description), which in turn are needed for other improvements. Since there's not much to do but it blocks several other patches, I'm raising priority.

Krinkle subscribed.

Thanks @Daimona for looping us in. We've looked at the Xenon data from before and after this change, as well as the Save Timing metrics, and everything looks more or less the same as before. Closing this as such.

@Krinkle Many thanks, that's great news! However, there are still a couple of things to do here, basically all the patches above are still waiting to be merged :) For now, we can go on with one and two.

If you confirm that the sending that data to Graphite (point 2 in task description) is fine as well, then I'll also get runtime profiling enabled on every wiki in a SWAT window next week, and then we'll be able to take the final step with the removal of the other global.

Yep, the statsd traffic isn't a concern either, thanks for checking!

Change 423945 merged by jenkins-bot:
[operations/mediawiki-config@master] Enable $wgAbuseFilterRuntimeProfile on every wiki

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

Mentioned in SAL (#wikimedia-operations) [2019-02-04T19:22:21Z] <jforrester@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: T191039 Enable wgAbuseFilterRuntimeProfile on all wikis (duration: 00m 47s)

Change 486284 abandoned by Daimona Eaytoy:
Save slow filters data without checking wgAbuseFilterRuntimeProfile

Reason:
Done in Ib1112e2fefd0631550d386ba87e5f87db84c3036, this was meant to be merged quickly, but we're ready to do it all together in the other patch.

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

Change 486470 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Remove $wgAbuseFilterRuntimeProfile

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

Change 486279 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove $wgAbuseFilterProfiling

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

Change 486469 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove $wgAbuseFilterRuntimeProfiling

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

Change 486280 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Remove $wgAbuseFilterProfile

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

Change 486279 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Remove $wgAbuseFilterProfiling

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

Change 486469 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Remove $wgAbuseFilterRuntimeProfiling

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

Only the 2 config patches are left, and they're scheduled in tomorrow's SWAT.

@Raymond could you please move abusefilter-edit-status-profile to abusefilter-edit-status overwriting the old content in the latter? I forgot to ask before, but you already have clearance.

@Raymond could you please move abusefilter-edit-status-profile to abusefilter-edit-status overwriting the old content in the latter? I forgot to ask before, but you already have clearance.

@Daimona Yes, will do after the next sync this night.

Change 486470 merged by jenkins-bot:
[operations/mediawiki-config@master] Remove $wgAbuseFilterRuntimeProfile

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

Mentioned in SAL (#wikimedia-operations) [2019-03-25T11:48:08Z] <lucaswerkmeister-wmde@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: [[gerrit:486470|Remove $wgAbuseFilterRuntimeProfile (T191039)]] (duration: 00m 49s)

Change 486280 merged by jenkins-bot:
[operations/mediawiki-config@master] Remove $wgAbuseFilterProfile

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

FTR, the removal of globals in the AF codebase wasn't still in production. While I knew this, I though I had set them to true in the default config, but I didn't. Deploying the changes above would have disabled profiling everywhere.
I'll schedule it for deployment next week, after the train.

Mentioned in SAL (#wikimedia-operations) [2019-03-25T12:00:40Z] <lucaswerkmeister-wmde@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: [[gerrit:498814|Revert "Remove $wgAbuseFilterRuntimeProfile" (T191039)]] (duration: 00m 51s)

Change 498817 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Revert "Revert "Remove $wgAbuseFilterProfile""

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

Change 498818 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[operations/mediawiki-config@master] Revert "Revert "Remove $wgAbuseFilterRuntimeProfile""

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

Change 498817 merged by jenkins-bot:
[operations/mediawiki-config@master] Revert "Revert "Remove $wgAbuseFilterProfile""

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

Mentioned in SAL (#wikimedia-operations) [2019-04-01T11:17:54Z] <zfilipin@deploy1001> Synchronized wmf-config/abusefilter.php: SWAT: [[gerrit:498817|Revert "Revert "Remove $wgAbuseFilterProfile"" (T191039)]] (duration: 00m 52s)

Change 498818 merged by jenkins-bot:
[operations/mediawiki-config@master] Revert "Revert "Remove $wgAbuseFilterRuntimeProfile""

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

Mentioned in SAL (#wikimedia-operations) [2019-04-01T11:25:11Z] <zfilipin@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: [[gerrit:498818|Revert "Revert "Remove $wgAbuseFilterRuntimeProfile"" (T191039)]] (duration: 00m 51s)