[00:00:31] Now there is this chain https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1306758 [09:45:04] Krinkle: https://w.wiki/Rw4B [11:41:12] tgr|away: edge intervention for mobile apps ready for review - https://gerrit.wikimedia.org/r/c/operations/puppet/+/1306230 [13:56:21] Krinkle: oh btw, on thumbor thumbnailing svg to webp https://grafana.wikimedia.org/d/Pukjw6cWk/thumbor?orgId=1&from=now-7d&to=now&timezone=utc&var-quantile=0.75&var-engine=svg&refresh=1m&viewPanel=panel-63 [13:58:52] Amir1: hm.. but the qps also changed a lot [13:59:19] doesn't mean 1:1 a concrete svg.webp is produced faster than svg.png [13:59:34] it has mostly recovered I think [13:59:55] https://grafana.wikimedia.org/d/Pukjw6cWk/thumbor?orgId=1&from=now-7d&to=now&timezone=utc&var-quantile=0.75&var-engine=svg&refresh=1m&viewPanel=panel-59 [14:01:30] so the theory is that thumbor is calling rsvg on these requests in away that produces a webp directly without a png? I know it's not chaining from the filename, all thumbor knows is source and destination, but I mean internally, afaik rsvg has no webp output mode [14:01:31] https://gerrit.wikimedia.org/g/operations/software/thumbor-plugins/+/499c5a3121f36e98091664f2a05a27620af57fcc/wikimedia_thumbor/engine/svg/svg.py#33 [14:01:44] and that svg-to-webp is faster than svg-to-png [14:01:55] rsvg output command hardcodes png [14:08:29] oh interesting, I don't know how the perf improvement is coming from. The rate isn't much higher now [14:08:47] I'm wondering if it now has support for webp and we could add that [14:09:48] https://gnome.pages.gitlab.gnome.org/librsvg/devel-docs/features.html it does support it now [14:10:22] I hope it's not asking something else to generate the svg [14:10:27] e.g. imagemagic doing it or something [14:10:35] there is mention mention of svg input in the imagick engine [14:10:45] and that's the only one doing webp output afaik [14:10:48] I wouldn't be surprised to be honest 😅 [14:10:51] maybe they're layered [14:10:57] we have never seen any bugs in thumbor ever [14:10:57] but I don't see any code doing both. [14:11:41] I'm guessing it's layered and/or the webp output is comingn from rsvg in a way I can't see. [14:11:49] I have confirmed it is indeed generating image/webp output [14:11:57] so it's not e.g. serving PNG under .webp or something [14:12:33] based on turnilo. Content type header is webp [14:13:17] and the engine metris in prometheus are presumaby based on engine selection and not independent URL matching [14:13:27] so that suggests it is indeed caling the svg.py file and not imagic else [14:14:16] the current librsvg says they support webp but we hard-code png in the output. Maybe something overrides it [14:16:13] tomorrow-me rabbit hole [14:19:19] https://gerrit.wikimedia.org/r/plugins/gitiles/operations/software/thumbor-plugins/+/bd557b6c12c106ea23c4b76e5c63bc501899c4ba%5E%21/#F63 [14:19:25] This is the commit that implemented webp output [14:19:49] I'm not sure how it ends up in imagic, but I think that's what it does. svg to png first and then something is adding the second imagic stage. [14:20:11] not a spike on imagemagic engine calls in prometheys though [14:20:16] but maybe that metric is limited to the first engine [14:20:22] https://grafana.wikimedia.org/d/Pukjw6cWk/thumbor?orgId=1&from=now-7d&to=now&timezone=utc&var-quantile=0.75&var-engine=svg&refresh=1m&viewPanel=panel-65 [14:20:30] since it wouldn't be a separate request [14:22:05] thumbor_engine_user_seconds > thumbor.engine.processing_utime > https://gerrit.wikimedia.org/g/operations/software/thumbor-plugins/+/499c5a3121f36e98091664f2a05a27620af57fcc/wikimedia_thumbor/engine/proxy/proxy.py#151 [14:24:41] yeah, seems one metric per request, but maybe something elsewhere is allowing for multiple engines [14:24:50] I'll leave that unknown for now. Not sure [14:24:59] I guess hugh would know [14:27:35] there's some weird stuff in there about an intermediary jpg as well, but I'm pretty sure that's outdated variable names and not real [14:27:51] you wouldnt want a lossless webp for an svg to come from a lossy jpg [14:28:01] https://gerrit.wikimedia.org/r/c/operations/software/thumbor-plugins/+/519379 [14:28:07] https://gerrit.wikimedia.org/r/plugins/gitiles/operations/software/thumbor-plugins/+/499c5a3121f36e98091664f2a05a27620af57fcc/wikimedia_thumbor/engine/imagemagick/imagemagick.py#243 [14:28:14] for svg/png inputs, the intermediary is png32 [14:28:19] instead of jpg [14:29:42] anyway, so yeah, not sure why average latency would be lower. My guess is: it's not measuring the webp conversion, and traffic has shifted to be more diverse and so more of them are simpler/easier vs before only popular ones and whatever bias the popular subset has with it is now less visible [14:30:36] I've compared https://upload.wikimedia.org/wikipedia/commons/thumb/8/80/Wikipedia-logo-v2.svg/1280px-Wikipedia-logo-v2.svg.webp to png, and compared png/webp for cases at https://commons.wikimedia.org/wiki/Librsvg_bugs and didn't see any rendering differences [14:32:21] so I'm assuming it's all still rsvg at least, but if that did change, I'm sure we'll hear it soon enough [16:20:50] Amir1: is there a patch that uses https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1304640 ? I thought I saw one for WAN cache or something. Maybe that was rebased onto something else? [16:21:50] AaronSchulz: this could benefit from it now https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1304603 [16:22:26] i.e. nothing relies on it but if merged, I can make a follow up to clean up stuff from that patch [16:22:43] I mostly just wanted to see where it was going. [16:23:12] beside this, I don't have any patch in mind being made [16:23:16] "TODO: Factor out db config into a lightweight dedicated service so it wouldn't rely on WAN" [16:23:28] I see [16:23:49] it could be useful in other areas but I haven't seen any. I could imagine it but haven't looked [16:25:46] Amir1: can you make the WAN cache cleanup patch and then I can merge both? [16:25:58] also, it's just conceptually cleaner, db config vs service dependencies like WAN/CP/etc. [16:25:59] (as a stack of course) [16:26:01] sure thing! [16:26:56] I just want to see it working as intended (which sometimes avoid the need for follow-up patches). [16:27:11] yeah, totally [16:32:34] done https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1306957 [16:33:24] that ?? 1 is not really needed as the conf is basically guaranteed to have 'servers' but better safe than sorry. [16:34:47] Its awkward having several similar factory/config services, but that's hard to avoid in the short-term. I don't see any technical problem with how it splits the config/MWLBFactory. Maybe later killing the DBLoadBalancerFactoryConfigBuilder is the right move as suggested. [16:34:55] Somehow we ended up with the LBFactory factory config to get the LBFactory factory to get the LBFactory to get the LB, to get DB connections, but it's not like callers have to care about that :) [16:36:15] Oh yeah. That's actually on my list to kill. It shouldn't be a service. Maybe a class still but definitely not a,service [16:39:14] * AaronSchulz It's really just a bunch of helper glue, used in a one-and-done way, mostly for compatibility. We don't want it bloating ServiceWiring, but doesn't need to persist as a service, yeah. [16:40:37] * AaronSchulz It's not like we want random callers making their own LBFactory instances or something. [16:40:58] Yeah. What could possibly go wrong [17:56:59] https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1306967 [17:57:02] Even cleaner [18:59:47] * AaronSchulz Amir1: I added some comments to https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1304603 while looking at the newer patches. I was confused by the LBFactoryMulti case. [19:50:50] AaronSchulz: I'm 99% sure we still set servers there too [19:51:07] I can double check but on phone right now [22:01:56] MWLBConfig probably should have been named MWLBFactoryConfig. I missed that, hehe. [22:26:06] Technically it's MWLBFactoryConfigFactory but let's have mercy on devs [22:29:06] MWLBFCFactory :-P (that is a joke!) [22:30:22] unrelated: I don't know how it already has merge conflict but this would be nice https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1306796 (and the parent). After these patches are deployed, I'll switch the production to use redis for lock mgmt. That'd move a lot of load off the prod dbs