[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Lucas_Werkmeister_WMDE added a comment.
This change was deployed for about four hours yesterday, and during that time it seems to have improved the caching behavior a lot: based on Grafana data (permalink), the overall hit rate went from 8.5% to 60%.
awk -F';' '
$1=="hit" { hit+=$3 }
$1=="miss" { miss+=$3 }
ENDFILE { print 100*hit/(hit+miss) "%"; hit=0; miss=0; }
' without-change.csv with-change.csv
However, it may also have caused T179156: 503 spikes and resulting API slowness starting 18:45 October 26 :/TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: WMDE-leszek, Lucas_Werkmeister_WMDECc: Krinkle, aaron, gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lahi, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Wikidata-bugs, aude, Mbch331___
Wikidata-bugs mailing list
Wikidata-bugs@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2017-10-24 (1.31.0-wmf.5)). TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: WMDE-leszek, ReleaseTaggerBotCc: Krinkle, aaron, gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-07-25_(1.30.0-wmf.11)). TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: ReleaseTaggerBotCc: Krinkle, aaron, gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Lucas_Werkmeister_WMDE added a comment. Good point, let’s discuss this in T176312: Don’t check format constraint via SPARQL.TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Lucas_Werkmeister_WMDECc: Krinkle, aaron, gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Lucas_Werkmeister_WMDE added a comment. @Krinkle thanks for your comments! And this uses the same Sparql endpoint at https://query.wikidata.org/ as for public queries? Yup. but I didn't know it was used e.g. when saving edits (assuming validation happens there). No, constraint checks are currently a fairly “external” feature – they’re done when users visit an item, or after they’ve saved a statement, and only for users who have the checkConstraints gadget enabled. There are plans to enable the gadget by default (T173695), and to check constraints before saving, too (T168626), but we don’t plan to make that mandatory. ConstraintCheck/SparqlHelper in Wikidata currently has a timeout of 5 seconds. Presumably in order to support some of the more complex regular expressions and larger input texts. That’s not really the main reason – we also use the query service for type checking: is some item an instance of some class or a subclass of it? We try to do this check in PHP at first, but if that takes too long (e. g. because the type hierarchy is too deep, too branched, or even circular), we bail out and ask the query service instead. For just the regexes, I’d be fine with a much shorter timeout. Response times look fairly good in Graphite (source). Wow, I didn’t know about that thing. Nice! (Looks like I can configure the graph by tweaking the URL even though visiting https://graphite.wikimedia.org/ gets me an authentication error :) ) The minutely p99 over the last 5 days ranges from 100-500ms… This is okay. By the way, what’s the round-trip time for memcached assuming a cache hit? If it’s, dunno, 100 ms, then the patch is probably not worth the trouble :) (And @Jonas, perhaps we can switch the average times in the Grafana board from mean to median? Those graphite stats look a lot less serious than the Grafana board.) I wonder if something like a "simple" PHP or Python subprocess would work (something that runs preg_match or re.match, using tight firejail with cgroup restrictions, like other MediaWiki subprocesses). Hehe, I’ve actually written a service like that :) we have a test system for constraints on Cloud VPS (wikidata-constraints.wmflabs.org), and it doesn’t have enough RAM to run Blazegraph, so I wrote minisparql, which just listens for regex SPARQL queries and checks them with preg_match. Of course, if we were to do something like this on Wikidata itself, we could skip the SPARQL wrapping. But I have no idea how much trouble it is to deploy a new service like that… Or perhaps using LuaSandbox? Yeah, the different pattern flavors are a problem… the situation is already unfortunate because WDQS doesn’t quite implement PCRE either, but at least it’s similar. Lua Patterns seem to be completely different. (We’re stuck with PCRE-like patterns because there’s also an external service, KrBot, which checks constraints daily and saves the results in WD:Database Reports/Constraint Violations, and we need to be (mostly) compatible with it.)TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Lucas_Werkmeister_WMDECc: Krinkle, aaron, gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Krinkle added a comment.
In T173696#3584607, @Krinkle wrote:
[..] why do we need to query an external service to execute a regular _expression_?
In T173696#3584821, @Lucas_Werkmeister_WMDE wrote:
Because checking arbitrary regexes on arbitrary input opens us up to DoS attacks via malicious regexes [..]. This came up during the security review of the extension (T101467), and we eventually decided to check regexes on an external service instead, where queries are subject to a timeout (T102752).
Interesting. And this uses the same Sparql endpoint at https://query.wikidata.org/ as for public queries? I knew this was called from within Wikidata for MediaWiki API queries, but I didn't know it was used e.g. when saving edits (assuming validation happens there).
I'm bringing this up because the reason we want caching is due to its performance, right? ConstraintCheck/SparqlHelper in Wikidata currently has a timeout of 5 seconds. Presumably in order to support some of the more complex regular expressions and larger input texts. I imagine that with a more performant backend caching might not be as needed.
(off-topic: )
Response times look fairly good in Graphite (source). The minutely p99 over the last 5 days ranges from 100-500ms, although with regular spikes of 1-5 seconds. From manual testing I find it's usually ~350ms with short regex/text, and ~600-1200ms for more larger input. The only responses under 300ms are those that were a cache-hit in Varnish (yay) in which case it's typically 100-130ms. This is okay.
On the other hand, considering its for internal execution of a single regex, ~300ms is a lot. I wonder if something like a "simple" PHP or Python subprocess would work (something that runs preg_match or re.match, using tight firejail with cgroup restrictions, like other MediaWiki subprocesses). Or perhaps using LuaSandbox? (below via eval.php):
$sandbox = new LuaSandbox; $sandbox->setCPULimit(0.25); $sandbox->setMemoryLimit(512000); // 512KB
$lua = "function match_text(text, pattern) return string.match(text, pattern) ~= nil end";
$sandbox->loadString($lua)->call(); $result = $sandbox->callFunction('match_text', 'foo', 'f..');
return is_array($result) && isset($result[0]) && $result[0] === true;
These lines take < 1ms. With large input (e.g. wikitext article), and a more complex pattern, between 5 and 10ms. However, I quickly realised Lua doesn't implement PCRE regular expressions by default (T52454, Lua Patterns). I wonder if it's worth revisiting the security implications of adding PCRE bindings to a LuaSandbox (perhaps reduced to a subclass for Wikibase).
Anyway, just a few off topic thoughts :)TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: KrinkleCc: Krinkle, aaron, gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___
Wikidata-bugs mailing list
Wikidata-bugs@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Lucas_Werkmeister_WMDE added a comment. Because checking arbitrary regexes on arbitrary input opens us up to DoS attacks via malicious regexes (e. g. catastrophic backtracking – runtime of checking (x+x+)+y on xxx..xxx is quadratic in input length). This came up during the security review of the extension (T101467: Ex: WikibaseQualityConstraints - remove or sanitize regex for FormatChecker), and we eventually decided to check regexes on an external service instead, where queries are subject to a timeout (T102752: [RFC] Workaround for checking the format constraint).TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Lucas_Werkmeister_WMDECc: Krinkle, aaron, gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Lucas_Werkmeister_WMDE added a comment. So change Iaaac950b48 implements caching on the most granular level we have, for the “format constraint” check: does arbitrary text match arbitrary regex? This is very common constraint type – practically all “external identifier” properties have one or more regexes to ensure that the identifier matches a certain format, many “Commons link” properties have regexes to assert that the link has a particular file name extension, and several “URL” properties check the protocol with a regex (e. g. only https?://.+). It is also, according to our statsd tracking, one of the most expensive constraint types. And as long as the regex flavor is stable, the result is valid indefinitely: it depends only on the text and the regex. The question is whether it makes sense to cache this information, given the scale of Wikimedia’s object cache. I wrote a small script (P5921) to check how many format constraints each property has (it can have several), and how many statements, which should tell us how many combinations of text and regex there are to be cached (approximately). The script fails for one property (too many statements to count in WDQS), so I got the count for that property via another tool (see commit message for details). Overall, I estimate there are about one hundred million text-regex combinations for which we could cache the boolean result. I assume that most of these combinations will never be hit (currently, only 193 users have the checkConstraints gadget enabled, and many items with “external identifier” statements will probably never be visited by any user with the gadget enabled), but still, that’s a huge number, and I have no idea if it’s sensible to cache that or not. Can someone from #performance-team comment on this?TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Lucas_Werkmeister_WMDECc: gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
gerritbot added a project: Patch-For-Review. TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: gerritbotCc: gerritbot, Ladsgroup, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, Lordiis, GoranSMilovanovic, Adik2382, Th3d3v1ls, Ramalepe, Liugev6, QZanden, Lewizho99, Maathavan, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Jonas added a project: Wikidata-Sprint. TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: JonasCc: Ladsgroup, Anomie, daniel, Aklapper, Jonas, Lucas_Werkmeister_WMDE, GoranSMilovanovic, QZanden, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Updated] T173696: Cache constraint check results
Lucas_Werkmeister_WMDE edited parent tasks, added: T103228: Improve performance of constraint check; removed: T173695: Enable constraint checks by default for users. TASK DETAILhttps://phabricator.wikimedia.org/T173696EMAIL PREFERENCEShttps://phabricator.wikimedia.org/settings/panel/emailpreferences/To: Lucas_Werkmeister_WMDECc: Aklapper, Jonas, Lucas_Werkmeister_WMDE, GoranSMilovanovic, QZanden, Agabi10, Izno, Wikidata-bugs, aude, Mbch331___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
