[00:00:29] oh right, the contact you choose has to be deduped for the update code to hit [00:00:42] i.e. there has to be exactly one contact wiht that name + email combo [00:01:00] will edit the comment to note that [00:01:31] ejegg: is that dedupe on donations run change a setting [00:01:48] not any more [00:01:54] it's on for keeps [00:01:56] hmmm [00:02:03] i just have like 3000 jimmys locally [00:02:17] heh, yeah, so it will keep making more [00:02:31] because it can't find a single unique one to dedupe into [00:02:50] hmm ok [00:03:18] when there are multiple existing candidate that share the fname,lname,email it doesn't guess one, it just gives up, makes a new record, and lets human dedupers do the work [00:03:30] actually, your local db is probably ripe for a batch merge [00:03:46] pretty painless from the new deduper UI [00:03:47] i guess why didnt it dedupe on the first one but maybe there were already multiple in the test data [00:04:12] so the 'on forever' change was just last week [00:04:37] ahhh haha [00:05:57] ok its telling me to do streches [00:06:07] spelled right [00:08:11] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10fundraising sprint Parallel processing roller coaster, 10Recurring-Donations, 10Patch-For-Review: Recurring queue consumer fails with funny stats execption - https://phabricator.wikimedia.org/T289175 (10Ejegg) [00:08:33] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10fundraising sprint Parallel processing roller coaster, 10Recurring-Donations, 10Patch-For-Review: Recurring queue consumer fails with funny stats execption - https://phabricator.wikimedia.org/T289175 (10Ejegg) a:03Ejegg [00:08:57] maybe i shouldn't have done that right now hah [00:09:18] oh yeah i guess it would have been easier to just edit one to be unique [00:10:04] yeah if its working with your tests locally im happy to +2 [00:10:49] err, still fiddling with phab, haven't actually re-set-up everything to run locally yet [00:11:05] okie [00:15:55] ok got a unique one hah [00:16:38] ok cool broke it locally [00:18:53] dang, my test seems to have discarded the first message [00:19:05] oh i guess i forgot to mutate the subscr_id [00:20:43] i was lazy and just made 2 ingenico monthly convert donations [00:22:37] oh hey, that would work too [00:22:45] good call [00:25:05] udpated that comment again with your method! [00:25:15] need to remember to run the ctqc too [00:25:22] oho [00:26:25] or well the error is gone just need to run the ctqc to get the monthly converts happily in the db [00:28:37] haha well maybe not cause then it fails on mail not being set up maybe thats fine to not run it [00:29:34] but looks good for fixing the actual issue [00:34:01] i guess i dont understand the whole issue at all was yesterday the first time the same person had two messages in one run? [00:34:55] so previously all the code which called that timer start and end was in the same function (wmf_civicrm_contribution_message_insert) [00:35:06] and that function reset the unique token at the start [00:35:18] ahhh was that in the civi deploy then? [00:35:45] yeah, the lastest refactor I think made the recurring start functions use the new code [00:35:58] and they DON'T reset the unique token at the start of each message [00:36:58] it still seems like a funky way to do the stats collecting [00:37:33] and I think the fact that the unique tokens have a '.' which the stats container interprets as a namespace delimiter is a further layer of brokenness [00:38:30] (03CR) 10Cstone: [C: 03+2] "This fixes it locally for me" [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713714 (https://phabricator.wikimedia.org/T289175) (owner: 10Ejegg) [00:38:37] woohoo! [00:38:52] ok, i'll deploy that, then look for a better fix [00:49:03] (03Merged) 10jenkins-bot: Reset unique stats token on each message [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713714 (https://phabricator.wikimedia.org/T289175) (owner: 10Ejegg) [01:10:08] (03PS1) 10Ejegg: Reset unique stats token on each message [wikimedia/fundraising/crm] (deployment) - 10https://gerrit.wikimedia.org/r/713730 (https://phabricator.wikimedia.org/T289175) [01:10:21] (03CR) 10Ejegg: [V: 03+2 C: 03+2] Reset unique stats token on each message [wikimedia/fundraising/crm] (deployment) - 10https://gerrit.wikimedia.org/r/713730 (https://phabricator.wikimedia.org/T289175) (owner: 10Ejegg) [01:13:48] !log updated fundraising CiviCRM from 73f6ec9190 to 8ed303f2d1 [01:13:54] Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log [01:14:01] ok, that should stop those failures [01:23:21] (03CR) 10Ejegg: Remove unused parameters from txn building fns (031 comment) [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/584062 (owner: 10Ejegg) [01:54:51] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10fundraising sprint Parallel processing roller coaster, 10Recurring-Donations, 10Patch-For-Review: Recurring queue consumer fails with funny stats execption - https://phabricator.wikimedia.org/T289175 (10Ejegg) OK, that fix is deployed and the recu... [01:56:02] oh fun, now that the recurring qc is running full tilt, we get the database contention failmails [01:56:33] haha im watching ejegg it just looks like one time [01:56:39] yep, another reason to prioritize https://phabricator.wikimedia.org/T240581 [01:56:57] yeah i agree [01:57:52] i can requeue those in a bit ejegg|away [01:59:41] oh cool! Do you already have a script to make json out of those var_dump style logs? [01:59:57] oh they were just in the damaged table does that not work [01:59:59] oh, or you mean the contended ones [02:00:02] cool cool [02:00:03] i was just going to wait until its not chaos [02:00:08] that works fine [02:01:35] this is all done, right? https://phabricator.wikimedia.org/T261713 [02:01:57] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10FR-India: CiviCRM queue fail due to emojis in address or an Indian script being used - https://phabricator.wikimedia.org/T261713 (10Ejegg) I think we can close this now, right @Eileenmcnaughton & @Dwisehaupt ? [02:02:21] (03PS1) 10Eileen: More preferred syntax updates [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713732 (https://phabricator.wikimedia.org/T285321) [02:02:33] I also want to prioritize https://phabricator.wikimedia.org/T241261 [02:03:08] I'm 98% sure that emoji one is done and yes to all the JSON [02:03:08] another minor patch on the preferred syntax change ^^ and then I'm down to just worrying about the message template adaptation [02:03:21] ie https://gerrit.wikimedia.org/r/713732 [02:03:27] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM: Queue consumers should log messages as JSON - https://phabricator.wikimedia.org/T241261 (10Ejegg) @DStrine this would have been pretty helpful to rescue those subscr_start messages dropped by T289175 , and this would be a very quick change. [02:03:40] RECOVERY - check_redis on frqueue1003 is OK: OK: REDIS 5.0.3 on 127.0.0.1:6379 has 1 databases (db0) with 5 keys, up 86 days 12 hours - memory use is 6.88M (peak 15.07M, 0.14% of max, fragmentation 1.68%), connected_slaves is 3, donations is 6, jobs is 0, jobs-adyen is 0, jobs-paypal is 0, payments-antifraud is 0, payments-init is 0, pending is 0, recurring is 5117, refund is 0, unsubscribe is 3 [02:05:01] hmph, the new preferred join format seems worse to me at a glance [02:05:13] but i guess there are reasons it's been chosen [02:05:41] ejegg|away: multiple joinable fields potentially [02:06:25] sure, but you could also join a lot of different tables on contact_id! [02:09:32] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10FR-India: CiviCRM queue fail due to emojis in address or an Indian script being used - https://phabricator.wikimedia.org/T261713 (10Eileenmcnaughton) 05Open→03Resolved a:03Eileenmcnaughton Yeah - I think so [02:13:55] ejegg|away: back at the keyboard finally, looks like it's all merged... and even deployed? [02:16:12] yep AndyRussG and the queue is processing [02:16:56] cstone: yaaaayyyy [02:17:23] cstone: so I guess I should look at other stuff, maybe eileen has stuff for review, or maybe I'll try apple pay some more? [02:18:25] AndyRussG: I think I only really have this one in review at the moment - https://gerrit.wikimedia.org/r/c/wikimedia/fundraising/crm/+/713732 [02:18:36] which I just added :-) [02:18:59] I potentially have an upcoming one when I see how the tests go [02:19:54] K thanks, looking! :) [02:34:44] eileen: just out of curiosity, is it documented somewhere that contact.something is equivalent to contact_id.something? I imagine so? [02:35:45] yeah mostly through the api explorer - but I'm going off https://integration.wikimedia.org/ci/job/wikimedia-fundraising-civicrm-docker/5837/console [02:39:54] eileen: so things are failing without that patch? [02:40:12] erm - that could be copy & paste from the last patch [02:40:18] I think it might just be really noisy [02:40:31] note - that is when I throw the new 5.41 version at it we see that [02:40:45] hmm [02:41:01] the tests pass on both versions meaning that both syntaxes work on both versions [02:41:08] but it's really noisy [02:49:04] oki [02:49:33] eileen: I just saw failures on the Jenkins output you linked above, but maybe that's not what you were looking at? [02:49:54] AndyRussG: yeah - those are what I'm working on now [02:50:04] ah oki [02:51:18] eileen: so I did some api fiddling in the explorer and on the command line, looking at contact vs contact_id, mainly just to content myself, and will now +2 ;p [02:51:36] (03CR) 10AndyRussG: [C: 03+2] "Cooolio!!!!" [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713732 (https://phabricator.wikimedia.org/T285321) (owner: 10Eileen) [02:53:28] (03PS1) 10Eileen: New fix for 5.41 fails on render template [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713734 (https://phabricator.wikimedia.org/T285321) [02:53:50] AndyRussG: this is the fix for that other fail -https://gerrit.wikimedia.org/r/c/wikimedia/fundraising/crm/+/713734 [02:56:00] (03PS1) 10Eileen: More deprecated api syntax fixes [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713735 [02:56:03] eileen: cool! I'm gonna go pick up Sofi from her friend's place, back in 45 min to 1 hr :) [02:56:25] have fun [03:02:29] (03CR) 10jerkins-bot: [V: 04-1] More preferred syntax updates [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713732 (https://phabricator.wikimedia.org/T285321) (owner: 10Eileen) [03:09:11] that failmail thats happening I am going to requeue them once the recurring queue consumer finishes [04:14:03] eileen: I'm back but my brain is a bit mush now [04:14:09] :-) [04:14:29] I guess the thing to do will be to review and merge the one that fixes tests, then rebase the failing one on top of it [04:14:47] so the failing one is in the civicrm repo [04:15:08] 1) DonationStatsCollectorTest::testGetOverallAverageGatewayTransactionAge with data set #0 (array('ACME_PAYMENTS'), array('-1 hour')) [04:15:10] 22:02:04 Failed asserting that 3601 matches expected 3600. ? [04:15:18] hmmm [04:15:33] oh yeah - I got confused which patch that is in - my money says its an intermittant [04:15:42] ^ this was on the one I just +2'ed (https://gerrit.wikimedia.org/r/c/wikimedia/fundraising/crm/+/713732/1) [04:15:55] (03CR) 10Eileen: "recheck" [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713732 (https://phabricator.wikimedia.org/T285321) (owner: 10Eileen) [04:16:07] AndyRussG: ah I knew I'd seen it! [04:16:12] jack was doing something with stats collector versions and such for an unrelated hting [04:16:17] *thing [04:16:43] I just told jenkins to try again since it just feels like something took a bit of a second longer than it expected [04:18:49] (03PS2) 10Eileen: New fix for 5.41 fails on render template [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713734 (https://phabricator.wikimedia.org/T285321) [04:19:09] (03PS2) 10Eileen: More deprecated api syntax fixes [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713735 [04:19:34] AndyRussG: I also rebased the others to be direct on master - there is no dependency between them - it's just my local order [04:52:46] AndyRussG: yep it passed on re-running [05:02:39] eileen: ah oki so a flapping test and not related to the other statscollector stuff I guess... [05:03:00] AndyRussG: yeah - is it flapping or was it just once? [05:07:30] hmmm well it's true I haven't seen it flap more than once [05:08:10] AndyRussG: yeah I'm suspicious that it seemed to be one second over one hour - makes me think a test just took longer than usual [05:08:16] for unknown reasons [05:25:39] hmmmm [05:25:46] eileen: heading to sleep, cya! :) [05:25:53] night [06:15:43] (03CR) 10jerkins-bot: [V: 04-1] Localisation updates from https://translatewiki.net. [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713760 (owner: 10L10n-bot) [06:21:46] (03CR) 10Raimond Spekking: [C: 03+2] "false positive" [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713760 (owner: 10L10n-bot) [07:45:36] PROBLEM - check_mysql on frdev1001 is CRITICAL: SLOW_SLAVE CRITICAL: Slave IO: Yes Slave SQL: Yes Seconds Behind Master: 1400 [07:50:36] PROBLEM - check_mysql on frdev1001 is CRITICAL: SLOW_SLAVE CRITICAL: Slave IO: Yes Slave SQL: Yes Seconds Behind Master: 1615 [07:55:36] RECOVERY - check_mysql on frdev1001 is OK: Uptime: 4212982 Threads: 17 Questions: 178549510 Slow queries: 3437529 Opens: 1191076421 Flush tables: 1 Open tables: 200 Queries per second avg: 42.380 Slave IO: Yes Slave SQL: Yes Seconds Behind Master: 0 [14:25:06] 10Fundraising-Backlog, 10fundraising sprint Parallel processing roller coaster: Non-English descriptor change back to 877 number string - https://phabricator.wikimedia.org/T289171 (10Ejegg) Looks like Amazon uses AMZN.COM/BILL in their charge descriptor [15:00:04] 10Fundraising-Backlog, 10fundraising sprint Parallel processing roller coaster: Non-English descriptor change back to 877 number string - https://phabricator.wikimedia.org/T289171 (10DStrine) Ok good to know. We're too close to en6c emails to play with this and potentially loose donations. I'll note this for f... [15:17:45] hi fr-tech [15:21:25] (03PS1) 10Ejegg: Eliminate need to init() import timer [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713889 (https://phabricator.wikimedia.org/T289175) [15:21:41] hi cstone! [15:21:44] and hi fr-tech! [15:27:04] (03PS3) 10Ejegg: More deprecated api syntax fixes [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713735 (owner: 10Eileen) [15:27:19] hi ejegg cstone [15:27:47] (03CR) 10Ejegg: [C: 03+2] "Yep, it's consistent with the new recommendations!" [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713735 (owner: 10Eileen) [15:28:11] hi ejegg damilare [15:28:29] (03PS3) 10Ejegg: New fix for 5.41 fails on render template [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713734 (https://phabricator.wikimedia.org/T285321) (owner: 10Eileen) [15:29:04] (03CR) 10Ejegg: [C: 03+2] "Yep, this works locally too." [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713734 (https://phabricator.wikimedia.org/T285321) (owner: 10Eileen) [15:29:11] hi damilare [15:32:07] ok, i'mma grab a list of subscr_ids from the logs to see which contribution_recur rows we're missing [15:34:17] (03CR) 10jerkins-bot: [V: 04-1] Eliminate need to init() import timer [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713889 (https://phabricator.wikimedia.org/T289175) (owner: 10Ejegg) [15:37:49] helloo fr-tech damilare ejegg cstone :) [15:39:24] (03Merged) 10jenkins-bot: More deprecated api syntax fixes [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713735 (owner: 10Eileen) [15:40:10] ejegg: out of curiosity, do you have a link to the new recommendations mentioned in eileen's commit messages? [15:40:11] (03Merged) 10jenkins-bot: New fix for 5.41 fails on render template [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713734 (https://phabricator.wikimedia.org/T285321) (owner: 10Eileen) [15:47:39] AndyRussG: i think it's just some commits in Civi core at this point [15:47:53] that deprecate the old fk aliases [15:49:25] let's see, here'a bug mentioning the deprecation: https://lab.civicrm.org/dev/core/-/issues/2689 [15:49:57] ah oki thanks! [15:50:56] ejegg: yesterday I was able to check locally that the api calls to contact.x vs contact_id.x give the same result [15:51:08] and the api explorer also suggests contact_id rather than just contact [15:53:35] AndyRussG: ok, here's the commit where those were deprecated: https://lab.civicrm.org/dev/core/-/commit/2b139d1bfb55457662d5eabcd9baa9e7b82195bb [15:54:41] ejegg: ah great thanks! yeah sounds logical now [15:54:56] I guess for us that'll be in our civicrm repo, not the crm repo [15:55:01] so the change to SchemaMapBuilder in that commit adds a join option for each FK, and then if the FK column ends in _id, it adds a deprecated join option for the table name [15:55:24] so that particular commit SHA only exists upstream [15:56:15] our repo mostly has a monster commit every couple of months when (usually eileen) unzips a big tarball of the latest RC or release over our existing codebase [15:56:30] then a few things cherry-picked here and there for urgent fixes [15:57:31] ejegg: I think your patch is the alternative to what I mentioned, which will need refactoring of the tests to get through [15:57:39] I'll reply on the thread [15:58:39] (03PS1) 10Damilare Adedoyin: Non-English descriptor change back to 877 number string [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) [15:58:47] jgleeson: oh shoot, i just noticed the fails [15:59:20] ejegg: ah right gotcha thanks much for the explanation! [15:59:38] ejegg: fwiw I'm looking now at this one that u requested review on: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DonationInterface/+/584062 [15:59:39] so when someone starts a timer that hasn't been stopped, I guess we want to simply reset the existing start time [15:59:56] AndyRussG: oh right, just some cleanup of obsolete stuff [16:00:50] it seems all straightforward and predictable except for the removal from lines 796+ in gateway.adapter.php [16:01:13] (03CR) 10jerkins-bot: [V: 04-1] Non-English descriptor change back to 877 number string [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) (owner: 10Damilare Adedoyin) [16:01:21] ejegg: hmmm that has side effects [16:02:12] if you accidentally override start without knowing (like in this case) you end up with the wrong timings [16:02:41] we shouldn't be calling start/end timer on the same stat twice I don't think as the stats names should be specific to events happening in relation to a donation [16:02:42] AndyRussG: you mean the remove of that $js param? [16:03:05] e.g. email_update_12345 [16:03:39] stsndup [16:03:41] with an a [16:06:27] (03PS2) 10Damilare Adedoyin: Non-English descriptor change back to 877 number string [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) [16:09:00] (03CR) 10jerkins-bot: [V: 04-1] Non-English descriptor change back to 877 number string [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) (owner: 10Damilare Adedoyin) [16:12:07] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10FR-India: CiviCRM queue fail due to emojis in address or an Indian script being used - https://phabricator.wikimedia.org/T261713 (10Dwisehaupt) Yes, the utf8mb4 conversion is complete from the ops end. [16:15:03] 10Fundraising-Backlog, 10fundraising sprint Parallel processing roller coaster, 10Patch-For-Review: Non-English descriptor change back to 877 number string - https://phabricator.wikimedia.org/T289171 (10Damilare) a:03Damilare [16:22:13] (03PS2) 10Ejegg: Eliminate need to init() import timer [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713889 (https://phabricator.wikimedia.org/T289175) [16:29:38] ok, ^^^ is getting further in the tests https://integration.wikimedia.org/ci/job/wikimedia-fundraising-civicrm-docker/5852/console [16:29:56] argh AndyRussG I thought you were going to share a non-work thing about tomorrow... not tomorrow [16:30:08] you could share it now if you'd like? [16:30:44] nice ejegg [16:32:57] jgleeson: thanks! it's just a bit of cooking stuff, with recipe and photo, it can wait until standup tomorrow, I'd say :) [16:33:25] ah nice. until tomorrow then :) [16:34:02] :) :) [16:34:06] oh hey, i just realized the matching_gifts_employers_check script actually runs the Sync + Export commands [16:34:18] so we can simplify that process-control yaml [16:38:19] hmm ejegg doesn't that change now do both [16:39:07] the process-control job is currently running MatchingGiftPolicies.Sync, then running the drush mgec script [16:39:16] oh, which I think is actually counter-productive [16:39:37] since that script relies on its own Sync call finding somethign new [16:39:51] which it won't if it's always running right after another Sync call [16:41:04] ok, pushed a change to the yaml [16:42:06] ejegg: re that lastest patch set. I think that code is basically allowing the stats to be misused [16:42:59] as in, if something sets a start time and then later on the start time is set again... when the end time is set it's not counting the actual time between the first and last call [16:43:01] yeah, looking at the latest p-c log for that job I see the output from the Sync command has 3 new results, but then the drush thing redoes the sync and finds 'No new employers available to sync since the last update' since the last update was 1 sec ago [16:43:56] jgleeson: so the only reason you'll call start on the same timer twice without an end() in between is if you get an exception, right? [16:44:19] so in PS2, when there's an exception and we don't get an end timer, we just start with a new unique tag [16:44:26] and leave the old start timer dangling [16:44:43] it still produces the same prometheus output [16:45:49] my goal here is for any code to be able to call the WmfContact Save functions without having to have this mysterious stats init call somewhere way up in the loop entry [16:46:05] ok so this isn't a normal code journey. this is happening when an exception is thrown after the start timer is called? [16:46:29] right jgleeson, and that happens a fair amount in tests [16:46:36] since we test a bunch of expected failures [16:47:56] so in a stats collector, i WANT to be able to just say 'startTimer(x), stopTimer(x), startTimer(x), stopTimer(x), export' and get an export that has the average duration of the x timer [16:48:02] sorry we're talking across two different use cases [16:48:17] I'm talking about the thing failing repeatedly in production [16:48:18] without having to say 'resetTimer' before the second start [16:48:32] trying to set the same timer start value twice [16:49:17] jgleeson: ah, so in production it failed because this line wasn't there: [16:49:20] https://gerrit.wikimedia.org/r/c/wikimedia/fundraising/crm/+/713714/1/drupal/sites/all/modules/queue2civicrm/recurring/RecurringQueueConsumer.php#17 [16:49:47] and the Recurring consumer in some cases calls code to update the contact [16:50:05] yeah and that's how it works, but when you call start a timer you assign a value to a stat in the instance with a unique name which is then later added to and then diffed to work out the difference when you end it [16:50:07] that code to update the contact has recently been refactored to use WmfContact\Save code [16:51:02] ah so you're keeping the unique token, just not needing to set it as a separate call [16:51:03] jgleeson so previously, all the timer start/stop calls were made from within the one big wmf_civicrm_contribution_message_import function [16:51:23] and it would init() at the start of the fn and deinit() at the end [16:51:42] but now that the function has been broken apart and there are calls to start and end timers from other places [16:51:59] it's possible to reach timer code without the init() / deinit() guarantee [16:53:20] and we COULD keep the patch that I had up last night, and put big warnings on the WmfContact\Save code with makes timer calls ("Make sure to re-init the timer before calling this function a second time") [16:53:34] but that feels pretty fragile [16:54:49] so right, it feels a little better for the start/stop calls to handle their own unique namespaces [16:55:40] sorry on a call, just reading [16:56:46] (03PS3) 10Damilare Adedoyin: Non-English descriptor change back to 877 number string [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) [16:58:06] ok so yeah to everything you said except the part about clearing namespaces that already exist [16:58:12] this is bad [16:58:25] the stats shouldn't be removing any preexisting stuff [16:58:46] right, in this patch they don't clear anything from the stats container [16:58:50] 10Fundraising-Backlog, 10fundraising-tech-ops, 10fr-donorservices: Civi certificate request: Lefki - https://phabricator.wikimedia.org/T288986 (10Dwisehaupt) [16:59:12] they just pick a new unique tag if the start is called twice [16:59:23] and leave the orphan start tag in the stats container [16:59:25] https://gerrit.wikimedia.org/r/c/wikimedia/fundraising/crm/+/713889/2/drupal/sites/all/modules/wmf_civicrm/ImportStatsCollector.php#77 [16:59:27] this line dos [16:59:28] 10Fundraising-Backlog, 10fundraising-tech-ops, 10fr-donorservices: Civi certificate request: Lefki - https://phabricator.wikimedia.org/T288986 (10Dwisehaupt) 05Open→03Resolved Verified certificate has been installed and civi logins are successful. [16:59:29] does [16:59:48] jgleeson: so that doesn't touch the stats container [17:00:07] just the ImportStatsCollector's associative array [17:00:43] 10Fundraising-Backlog, 10fundraising-tech-ops, 10fr-donorservices: Civi certificate request: Yamini - https://phabricator.wikimedia.org/T288987 (10Dwisehaupt) [17:00:55] so if we call start('verify') and it assigns 'verify_12345' as a unique namespace for the 'verify' description [17:01:03] 10Fundraising-Backlog, 10fundraising-tech-ops, 10fr-donorservices: Civi certificate request: Yamini - https://phabricator.wikimedia.org/T288987 (10Dwisehaupt) 05Open→03Resolved Verified certificate has been installed and civi logins are successful. [17:01:06] it will store that start time in the container under 'verify_12345' [17:01:11] ok I see what you mean [17:01:16] it leaves orphans [17:01:19] then if we call start('verify') again it just grabs a new tag [17:01:26] let's just check if that's going to be a problem when export is called [17:01:30] yah, but so does the existing code [17:01:38] if you call ->init() again [17:01:48] before you call ->end() [17:02:20] we shouldn't really be missing the endTimer call though unless like you say we throw an exception [17:02:50] and the code that is dispatching responsibility of to other methods should clean up it's timers [17:03:10] e.g. startTimer() [17:03:17] $this->othermethod() [17:03:20] hmmm, i don't want to have to sprinkle lots of timer management code around the entry points and exit points of all the code [17:03:21] endTimer() [17:03:34] i just want to startTimer and endTimer and later export [17:03:44] that's kinda how it works though. let me show you an example [17:04:07] so the export code checks for hasTimerDiff on each namespace [17:04:21] that way it ignores orphaned start timers [17:04:46] jgleeson: so in practice, we DO see a fair number of exceptions during message processing [17:04:56] ejegg: see here, each timer call is wrapping something we want to time https://github.com/wikimedia/wikimedia-fundraising-crm/blob/a304003570749fc5e4244aa50df54f865af6110e/drupal/sites/all/modules/wmf_civicrm/wmf_civicrm.module#L207 [17:05:04] often those messages are just sent to the damaged queue with a retry [17:05:44] so in an exception scenario we wouldn't be getting to the point of calling export I don't think [17:05:58] which is why we haven't had this problem before [17:06:00] jgleeson the export is called after the loop is over [17:06:18] the exception is called inside that _import function, after the timer is started [17:06:26] and then caught higher up in the loop [17:06:59] the reason we haven't had this problem before is that everything calling timer code used to go through the wmf_civicrm_contribution_message_import function [17:07:03] which called ->init() [17:07:24] but now we have that broken into smaller pieces of code, mostly in WmfContact\Save [17:07:47] and in eileen's latest refactor, those pieces of code are called outside wmf_civicrm_contribution_message_import [17:08:26] but she's still using the same stats counter? [17:08:30] so the quick fix was just to add more ->init() calls in faraway parts of the code (e.g. start of recurring queue consumer's loop) [17:08:56] jgleeson: yeah, she moved the timer start and stop calls over with the rest of the code she moved [17:09:25] and added an extra check to init the timers if they're not initialized yet [17:09:33] yeah seen that bit [17:09:41] hmmm [17:10:19] so let's just make it possible to call start(x), stop(x), start(x), stop(x), without needing an init() call in between [17:11:50] I think the next bit of code that will potentially hit this error will be the csv imports [17:12:28] those currently call wmf_civicrm_contribution_message_import but once they are changed to call the WmfContact\Save things, they'll need a new $timer->init() call inserted somewhere up in their loop too [17:12:30] agreed. removing the init() saves doing that elsewhere. [17:14:26] so did we confirm that the wmfContact::save() is throwing an exception that is being handled which is the root cause of the orphan stats? [17:15:51] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10fundraising sprint Parallel processing roller coaster, 10Recurring-Donations: Trickle of damaged recurring donation messages with contact_id but without gateway_txn_id - https://phabricator.wikimedia.org/T285396 (10Cstone) They are all non USD with... [17:16:59] it's odd because when we released the patch last night to get more info about which stat was not numeric there were no others we could see [17:17:17] basically something is creating orphan stats and it's bugging me [17:17:56] and this patch will skip over orphans and proceed, which is fine because it makes it less brittle, but there's still something odd happening [17:18:42] I was mistaken earlier when I thought you were removing stat namespaces. I can see you're just resetting the child class custom property [17:24:38] jgleeson: the orphan stats aren't a problem [17:25:08] the problem comes on the second recurring signup message that calls contact update [17:25:32] brb [17:25:49] so when we process new recurring signups we don't send those through wmf_civicrm_contribution_message_import [17:42:21] 10Fundraising-Backlog, 10Wikimedia-Fundraising-CiviCRM, 10fundraising sprint Parallel processing roller coaster, 10Recurring-Donations: Trickle of damaged recurring donation messages with contact_id but without gateway_txn_id - https://phabricator.wikimedia.org/T285396 (10Cstone) Looks like they started Ju... [18:13:44] cstone: I noticed that date and looked at gerrit commits around that time but couldn't see any likely culprits [18:13:54] yeah same jgleeson [18:14:18] spent about 10 minutes trying to figure out how to search a time period in gerrit [18:23:11] i wonder if its just exchange rage shenanigans somehow that the USD converted amount dropped below 1.00 and its losing it somewhere [18:32:29] bah I don't know what I did but i suddenly have no invoice_ids for new adyen recurrings in my local db I did yesterday [18:34:14] oh i know why [18:41:57] oh hey, good news, looks like most of those subscriptions ended up making it into Civi eventually [18:42:11] I only see one missing and that should be not too bad to reconstitute [19:14:07] nice ejegg [19:14:16] ok I guess I should review your stats patch [19:21:39] cstone: the amount is unusual [19:21:46] on those other ones [19:22:21] 10Fundraising Sprint Ketchup Flume Ride, 10Fundraising Sprint Mandatory corn dogs, 10Fundraising-Backlog, 10fundraising Sprint NULL calorie food cart, and 4 others: Creating New Donor Advised Fund Import - https://phabricator.wikimedia.org/T283104 (10MDemosWMF) Great, thanks @Eileenmcnaughton ! Would you l... [19:33:46] (03CR) 10Ejegg: "AndyRussG so we never pass a value for the $js param into the buildTransactionNodes. As a result, it's always false. And the only place we" [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/584062 (owner: 10Ejegg) [19:34:56] going to do a quick errand fr-tech [19:38:40] (03CR) 10Jgleeson: [C: 03+2] "Nice work Dami. This one looks good to me! Let's coordinate the deployment of this one with dstire." [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) (owner: 10Damilare Adedoyin) [19:38:53] (03CR) 10Jgleeson: [C: 03+2] "dstrine* even" [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) (owner: 10Damilare Adedoyin) [19:52:27] (03PS3) 10Jgleeson: Eliminate need to init() import timer [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713889 (https://phabricator.wikimedia.org/T289175) (owner: 10Ejegg) [19:52:33] (03Merged) 10jenkins-bot: Non-English descriptor change back to 877 number string [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713894 (https://phabricator.wikimedia.org/T289171) (owner: 10Damilare Adedoyin) [20:30:07] hey eyener so I looked quickly at the details of the table for landingpageimpression_raw [20:30:09] https://www.mediawiki.org/wiki/User:AGreen_(WMF)/Draft:Mapping_of_EventLogging_properties_to_FundraisingImpressions_database#Mapping_from_LandingPageImpression_events [20:30:57] so it looks like every row in that table should correspond to a pageview of a landing page on donate wiki [20:31:43] there is also a slightly more filtered table, donatewiki_unique [20:32:53] IIRC that table does something to remove duplicate pageviews, like for example if the same user clicked on the same link in an e-mail twice [20:33:03] I would try querying both tables to see if there's any significant difference [20:34:02] I think you should be able to filter on utm_campaign and/or dt (date) to limit your results to a given e-mail send [20:34:31] I guess how well that will work will depend on exactly which values are added to the links in the e-mails [20:37:54] hope this is useful! [20:38:13] pls ping if u need more digging :) [21:19:37] (03Abandoned) 10Jgleeson: Add unit tests for civicrm import stats patch to confirm stats output. [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/630683 (https://phabricator.wikimedia.org/T261689) (owner: 10Jgleeson) [21:19:58] (03PS1) 10Jgleeson: Add unit tests for civicrm import stats patch to confirm stats output. [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713941 (https://phabricator.wikimedia.org/T261689) [21:29:58] (03CR) 10Eileen: "recheck" [wikimedia/fundraising/crm/civicrm] - 10https://gerrit.wikimedia.org/r/713378 (owner: 10Eileen) [21:31:16] (03CR) 10jerkins-bot: [V: 04-1] Add unit tests for civicrm import stats patch to confirm stats output. [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713941 (https://phabricator.wikimedia.org/T261689) (owner: 10Jgleeson) [21:58:50] (03PS2) 10Jgleeson: Add unit tests for civicrm import stats patch to confirm stats output. [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713941 (https://phabricator.wikimedia.org/T261689) [22:03:05] (03PS1) 10Ejegg: JSON encode logged queue messages [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713948 (https://phabricator.wikimedia.org/T241261) [22:03:29] (03PS2) 10Ejegg: JSON encode logged queue messages [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713948 (https://phabricator.wikimedia.org/T241261) [22:06:34] fr-tech I'm going to deploy payments-wiki [22:07:08] should revert the non-english descriptors back to the 877 number and add the tax clause to france forms [22:07:45] (03PS1) 10Ejegg: Merge branch 'master' into deployment [extensions/DonationInterface] (deployment) - 10https://gerrit.wikimedia.org/r/713952 [22:07:50] (03CR) 10Ejegg: [C: 03+2] Merge branch 'master' into deployment [extensions/DonationInterface] (deployment) - 10https://gerrit.wikimedia.org/r/713952 (owner: 10Ejegg) [22:09:16] (03Merged) 10jenkins-bot: Merge branch 'master' into deployment [extensions/DonationInterface] (deployment) - 10https://gerrit.wikimedia.org/r/713952 (owner: 10Ejegg) [22:09:18] (03PS1) 10Ejegg: Update DonationInterface submodule [core] (fundraising/REL1_35) - 10https://gerrit.wikimedia.org/r/713953 [22:09:46] (03CR) 10Ejegg: [C: 03+2] Update DonationInterface submodule [core] (fundraising/REL1_35) - 10https://gerrit.wikimedia.org/r/713953 (owner: 10Ejegg) [22:12:34] ejegg: i think dami wanted to wait for the green light from dstrine on the short desc [22:12:48] dstrine: is that change ok to go out? [22:13:13] damilare|away: mentioned he wanted to confirm before deploying [22:14:11] (03PS3) 10Jgleeson: Add unit tests for civicrm import stats patch to confirm stats output. [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713941 (https://phabricator.wikimedia.org/T261689) [22:18:56] (03CR) 10Jgleeson: [C: 03+2] "Definitely makes life easier for the calling code and fixes the bug. Nice work!" [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713889 (https://phabricator.wikimedia.org/T289175) (owner: 10Ejegg) [22:19:58] thanks jgleeson! [22:20:01] (03Merged) 10jenkins-bot: Update DonationInterface submodule [core] (fundraising/REL1_35) - 10https://gerrit.wikimedia.org/r/713953 (owner: 10Ejegg) [22:20:03] And thanks for writing those tests [22:20:15] lemme pull those down just as soon as I get payments-wiki up [22:33:01] (03Merged) 10jenkins-bot: Eliminate need to init() import timer [wikimedia/fundraising/crm] - 10https://gerrit.wikimedia.org/r/713889 (https://phabricator.wikimedia.org/T289175) (owner: 10Ejegg) [22:42:28] !log updated payments-wiki from 0a27dbe9b6 to 564daed816 [22:42:36] Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log [22:56:27] (03PS1) 10Ejegg: Use parameter for displayed email address [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713960 (https://phabricator.wikimedia.org/T286880) [22:56:30] (03PS1) 10Ejegg: Fix monthly donation descriptor [extensions/DonationInterface] - 10https://gerrit.wikimedia.org/r/713961 (https://phabricator.wikimedia.org/T289171) [22:56:44] couple little i18n fixes for review fr-tech ^^^ [22:57:12] France form footer looks good [22:57:56] perhaps we could let it be wider on responsive screens is all