Page MenuHomePhabricator

Show the value of custom variables in the details view of AbuseFilter
Open, MediumPublic

Description

Imagine you have a rule in your abuse filter like ... ccnorm(added_lines) rlike .... There is no direct way to debug this currently, and you just have to read the documentation to venture a guess as to what the output of that function would be for a particular edit. This makes testing rules very difficult.

The solution I want to propose is this:

  1. Allow assigning the output to a custom variable (... debug := ccnorm(added_lines) { ...)
  2. When looking at the "details" view of abuselog (may it be for a previous edit that triggered a filter, or when using the examine page for batch testing), in addition to the built-in variables (like added_lines) whose value is shown in the table, also show the value of all custom defined variables.

This way one can clearly see what the output of that function was, and more easily debug the filter.

Event Timeline

@Huji: good first task tasks are self-contained, non-controversial issues with a clear approach and should be well-described with pointers to help the new contributor. Given the current task description without any links to information I'm removing the good first task tag. Please re-add the tag once the task description has been polished and provides sufficient information for a new contributor. Thanks!

Huji triaged this task as Medium priority.

Adding to subscribers those users who I know have been actively working on patches/review for AbuseFilter recently.

Currently, the variables shown in detail view include things that are static (e.g. the page namespace for the edited page), or calculated (e.g. the user's number of edits). The latter, of course, is not stored along with the log, so if you access the log details three years later, the user's number of edits will not be the same as when the filter was originally triggered. However, for all of these static and calculated variables, the filter itself does not need to be re-run.

With what I am proposing, the filter needs to be re-run, so that the value of the custom variables is calculated again and displayed in the details view. This means every time a user goes to the detail view of a log entry, more processing needs to be done. So I want to make sure that you all think that is okay.

Also, we need to make sure the version of the log that was active at the time the log was generated is used; the latest version of the log may not have the same custom variables, or their definition may have changed. But this is not too difficult to tackle (something similar to the patch for T52806 can be used).

Lastly, I want to also offer an alternative: instead of showing the custom variables on all detail view pages, we can show it *only* when the detail page is populated using the examine feature. This will restrict its availability to only those who have abusefilter-modify rights and only when they are actively testing (aka examining) a filter to see how it works. Do you think that is a good idea?

Anything that can help with debuging filters is a good idea! Imagine a situation that you are looking at a hit and asking: why did the filter (not) match this edit?

It would be best if the interface provided a detailed step-by-step tree, telling what value each variable had, what result each function call had, whether this condition was true etc.

Huji removed Huji as the assignee of this task.Sep 12 2017, 4:14 AM
Huji added a project: User-Huji.

I absolutely agree with the proposal, such feature would be an incredibly easy way to test filters with many functions and custom variables. By now, I usually check variables in batch with "var == something", or if possible "var irlike something" and try to build their content with gradual steps, which actually is a big waste of time when coding a filter. In this direction, T72152 would also be a great improvement and to me it's quite astonishing that neither of these features were already implemented. I'll try to work out a patch even though I'm not sure I can make it to work.

I found something really weird: AF already has a set up method for showing user set variables, which however for some reason is not used. In fact, I managed to make such variables appear by roughly adding a parameter.

ADD: Just a side note: such variables will be shown only for future logs, i.e. every existent log won't show them. But IMHO this is a really tiny problem.

Change 420757 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Show user set variables in log details

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

Change 420757 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Show user set variables in log details

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

Daimona moved this task from Backlog to Stretch on the AbuseFilter (Overhaul-2020) board.

This is an interesting feature and we'll reconsider it after the architecture review part.

Removing task assignee due to inactivity, as this open task has been assigned to the same person for more than two years (see the emails sent to the task assignee on Oct27 and Nov23). Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome.
(See https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.)

Change 420757 abandoned by Daimona Eaytoy:
[mediawiki/extensions/AbuseFilter@master] [WIP] Show user set variables in log details

Reason:
Too old to be useful. Also uses some hacks that should be avoided nowadays. Rewriting from scratch is certainly easier.

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

Quick list of caveats:

  1. We should keep BC with existing var dumps (that don't have custom vars); it would need some isset-like check
  2. The details view would get bigger. This might be resolved by adding something like a button in the UI, "show/hide custom variables" that would toggle a collapsible section
  3. (pointed out by @matej_suchanek) Ensure T71367 doesn't get worse.