Right now an echo_event row is inserted every time a notification is sent. However, if "web" delivery is disabled for the recipients (or for the entire notification type), we correctly skip inserting an echo_notification row, but we still insert an echo_event row. We do still need this row if one of the recipients uses batched emails (because the echo_email_batch table needs an event_id to refer to), but if none of the recipients use batching, we should not insert an echo_event row.
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Avoid event insertion if possible | mediawiki/extensions/Echo | master | +204 -25 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | None | T340453 [Epic] FY 2023-24 Growth Maintenance Work | |||
| Resolved | KStoller-WMF | T347840 [Epic] Q2 FY 2023-24 Growth Maintenance Work | |||
| Resolved | None | T308084 Reduce DB space used by Echo notifications | |||
| Resolved | matej_suchanek | T221258 Avoid inserting echo_event rows when not needed |
Event Timeline
Comment Actions
The least-effort solution (without defining new services and responsibilities) seems to be to simply pass around the same Event instance and insert it only when its id is needed for reference (Notification, Notifier). NotificationJob (see also T101050) should also be changed to not require the id, the event should be serialized for the job payload instead.
Comment Actions
Change #1074717 had a related patch set uploaded (by Matěj Suchánek; author: Matěj Suchánek):
[mediawiki/extensions/Echo@master] [POC] Avoid event insertion if possible
Comment Actions
@Mooeypoo Amir just told me about this task. We should definitly re-think at what point events get inserted into the database.
Comment Actions
MW Group won't take ownership on this ticket, the ownership of Echo is not changing.
MW engineering group is exploring whether we can create a notification platform in Core that can result in bringing Echo's platform operation into core, and when that happens, the group will take charge on that piece. But we're not taking ownership on bugs or requests in Echo.
It is really useful for us to know what some of the issues/requests that are related to Echo's operations, so we can take that into account if/when we can bring it into core, but we are not planning ownership or maintainership over bugs or improvement requests to Echo.
Comment Actions
Change #1074717 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Avoid event insertion if possible
Comment Actions
I've marked this as a risky change for the train next week: T375661#10277259
I don't expect any problems beyond the existing T378349, but this is changing many aspects for Echo, not sure that I've covered them all.
Comment Actions
QA Note: This change should not cause any user-visible changes at all. But if anything seems out of the ordinary with Notifications (or emails or push messages), then please highlight that here.
Comment Actions
Smoke test: when wmf.2 reached group1 (Wikidata et co.), the event insertion rate immediately decreased, yet the notification rate stayed the same: https://grafana.wikimedia.org/d/onyD7cOMk/echo-extension-notification-baseline-track.
Comment Actions
Thx, @Michael! I checked in betalabs for Echo notifications regression (including recording in db) - didn't notice any anomalies. Also, run a spot-check in production (wmf.2) - all looks normal.
grafana Echo event shows two noticeable drops on 11/06 and 11/07.
grafana Echo Notifications shows a normal pattern as far as I could see.