automod: action limits; create reports for interaction churn #465
Conversation
There was a problem hiding this comment.
Looks mostly good to me.
I'm getting a little twitchy at the sheer size of the PersistAccountActions func, though. I think I read it, and I think I understand it, and together with the tests I think it's probably correct... but it's getting harder to be confident, and taking longer to read. It might be good to break it up soon. I tried to offer a few inline thoughts about where, but they're very preliminary and it's up to you if you can make any use of them :)
automod/event.go
Outdated
| e.Increment("automod-quota", "takedown") | ||
| } | ||
| } | ||
|
|
||
| if newTakedown || len(newLabels) > 0 || len(newFlags) > 0 || len(newReports) > 0 { |
There was a problem hiding this comment.
(again in the "i'm not quite sure how I'd factor these things out, but, thinking out loud" vein...)
These four condition clauses seem like a good focal point for guiding a (?future) refactor.
When I was reviewing this, I noticed myself checking this line against all the other branches below to see if a slack notification would be accurately sent for exactly every event that would have at least one other persisted effect.
I think the answer is still "yes", as intended. It's just getting a bit harder to see as all the other instances of the same condition are drifting further apart. Maybe roughly a function per section below that's gated on each of these clauses? Or perhaps if we bundle all these values in a small struct, so their collective role is easier to see (and maybe add some logging methods to that, as a bonus? because I think "what updates we're about to push" is actually about what belongs in the slack message too)?
Also: I believe all the needsPurge = true assignments and eventual check of that bool can be replaced by... just checking 3 of these 4 again, directly, too.
There was a problem hiding this comment.
there is a bit of a corner case with reports, where it might not be known that they were not created until the actual attempt to do so (because there is an existing report). in that case a slack message gets sent out, but no report is created, and the account metadata cache is not purged.
I guess we could treat the skip-record-creation-at-last-minute thing as a real corner-case and always purge the cache?
There was a problem hiding this comment.
Yeah, I was wondering about that too.
I was wondering if maybe the heavier query for prior reports should actually move earlier? Because it's also potentially mildly confusing if the slack notification gets sent out saying a report to happen but it's actually not.
There was a problem hiding this comment.
It isn't super great how it is, but it does enable running with pushing to slack without actual admin-token access to the mod service, which I think is helpful as a possible way to deploy/operate, both for internal folks and third parties. The idea there is folks can develop rules and just manually independently report them in-app, based on a slack channel. Maybe that is too jank to care about, but I was running that way for a bit.
|
I agree overall, these persist functions have gotten unwieldy and hard to understand, even for me. I'll look in to refactoring. |
bnewbold
force-pushed
the
bnewbold/automod-action-limits
branch
from
1a1224c to
0d4a31f
Compare
|
I took at pass at breaking this function out and refactoring it. The control flow and behavior should be the same (still pretty complex), but hopefully clearer and more readable. |
Co-authored-by: Eric Myhre <[email protected]>
The motivation here is to start auto-reporting in production based on specific rules. Specifically, this PR would start reporting on interactions churn (follow/unfollow), letting human mods confirm before taking action on an account.
Before we do that, need to de-duplicate reports. For example, if an account creates thousands of spammy posts, only want to report the account once. Generally want to prevent run-away rules from creating millions of reports, or doing thousands of automated account takedowns (for example).
This PR prevents re-reporting based on daily counters (fast checks), as well as double-checking against the mod service API just before filing a report (slower, but reliable).
This PR also adds "quotas" for mod actions, implemented using the counter system. This isn't perfect (the counter system itself might be buggy or broken (causing duplicate actions), but seems like a good start. If quotas are exceeded, automod will log and skip taking additional actions until the next day.