Add divergence summary notification on a given interval #85

pete-eiger · 2023-09-21T19:50:15Z

Added NOTIFICATION_MODE config var, supports live, periodic-report and periodic-update options, live being the current functionality, periodic-report sending a summary of all divergences on a given interval and periodic-update accumulates the notifications that live would send, but sends them at a given interval (both periodic options can be configured with the new NOTIFICATION_INTERVAL, default is 24 every hours)
Added another tick (notification_interval) in tokio::select to facilitate the new daily action if NOTIFICATION_MODE=periodic_report
Added condition for sending divergence notifications when a divergence is caught, based on NOTIFICATION_MODE
Aggregated the live notifications such that they're sent in a batch message, instead of each one being sent as a separate message

Cargo.toml

subgraph-radio/src/operator/attestation.rs

subgraph-radio/src/operator/mod.rs

hopeyen · 2023-09-26T20:10:08Z

subgraph-radio/src/operator/attestation.rs

@@ -648,6 +654,10 @@ pub async fn process_comparison_results(
        }
    }

+    if !notification_summary.is_empty() {


hmmm there's a difference in the idea of interval versus a ratelimit. I can see how the current changes can easily turn into a daily report notification (can be a periodic_report enum)

for the ratelimit purpose of this PR (which I think you reframed into interval and became more of a daily report idea), I think the live notifications should be throttled into an aggregation, like your notification_summary + a dedup + tracked by the notifier. Then here you check if the summary is empty and if it should be ratelimited conditioning on the mode, before sending the summary.

I don't understand this comment

okay, do you have a question about it? could you tell me what do you not understand so I can explain further

I think we agree on the periodic_report option flow & implementation right? So it's just the live flow that you have concerns about here? How/why does a ratelimit fit into the live flow? If users want to limit their notifications they can use periodic_report, that's why we're adding it (I think).

I agree about the dedup but why does the summary have to be tracked by notifier? Isn't it properly constructed now during each comparison event?

Ok, I think the confusion here is caused by what should the aggregated notifications show to the users when they have set a notification interval.

If users use live, the worst case is they may get 1 notification every comparison interval ~300s if there's any result worth notifying on the result type

If users use periodic_report, they will get a summary generated based on the stored states by their configured interval. The report informs the state of the system but doesn't indicate what has changes from before and possibly be the same as the previous interval. This is good as it is consistent and predictable.

My point with this comment is we are missing a mode where the users will get notified by their configured interval (like periodic_report) and only if there's something worth notifying (similar to the live check). In order for the radio to know if it should notify across comparison intervals, the notifier has to know either what was the states in the previous state or it tracks worthy notifications along the way. Though this mode of notification doesn't have to live here with live and could live with the notification_interval matched on notification_mode

let me know if this is more clear, or if you want to leave this to another PR

definitely more clear, thank you! 💯

subgraph-radio/src/state.rs

hopeyen · 2023-09-27T17:08:51Z

subgraph-radio/src/operator/notifier.rs

@@ -14,16 +14,30 @@ pub struct Notifier {
    discord_webhook: Option<String>,
    telegram_token: Option<String>,
    telegram_chat_id: Option<i64>,
+    pub notification_mode: NotificationMode,
+    pub notification_interval: u64,
+    pub notifications: Vec<String>,


if we update notifications to be an Arc<Mutex<Vec<String>>> then do we still need notifier to be in an Arc<Mutex<>>?

Sadly nope, the whole notifier gets passed around between threads, but even if it wasn't there's trait issues if we try that, the Serialize and PartialEq traits on Notifier complain about an Arc<Mutex<>> field

Hmmmmm okay, then what if we ask persisted state to track the notifications instead of for notifier to track it? this way we get to keep notifier simple and notifications can be preserved across startups as well. This can be something to discuss later though might be relatively easy to refactor now

subgraph-radio/src/state.rs

pete-eiger · 2023-09-28T13:31:59Z

periodic-update:

periodic-report:

live:

hopeyen

notifications system on steroids ❗

pete-eiger force-pushed the petko/daily-notifications branch from ab1ff65 to 31b3152 Compare September 25, 2023 06:44

pete-eiger marked this pull request as ready for review September 25, 2023 08:19

pete-eiger requested a review from hopeyen September 25, 2023 08:21

pete-eiger force-pushed the petko/daily-notifications branch 2 times, most recently from 57d363c to 646861f Compare September 25, 2023 10:28

pete-eiger changed the title ~~feat: add option for daily notifications~~ feat: add divergence summary notification on a given interval Sep 25, 2023

pete-eiger force-pushed the petko/daily-notifications branch 10 times, most recently from a7414ba to aebb8ab Compare September 26, 2023 10:50

pete-eiger changed the title ~~feat: add divergence summary notification on a given interval~~ Add divergence summary notification on a given interval Sep 26, 2023

pete-eiger force-pushed the petko/daily-notifications branch 3 times, most recently from a855dd0 to e07ce1d Compare September 26, 2023 19:35

hopeyen previously approved these changes Sep 26, 2023

View reviewed changes

pete-eiger dismissed hopeyen’s stale review via a064a16 September 27, 2023 05:53

pete-eiger force-pushed the petko/daily-notifications branch 3 times, most recently from 0a163bf to f40601b Compare September 27, 2023 06:30

pete-eiger requested a review from hopeyen September 27, 2023 06:38

pete-eiger force-pushed the petko/daily-notifications branch 2 times, most recently from dc53683 to ba5d573 Compare September 27, 2023 16:02

hopeyen reviewed Sep 27, 2023

View reviewed changes

pete-eiger force-pushed the petko/daily-notifications branch from ba5d573 to cdb399c Compare September 27, 2023 18:37

pete-eiger force-pushed the petko/daily-notifications branch from cdb399c to 99d7b85 Compare September 27, 2023 19:57

pete-eiger requested a review from hopeyen September 27, 2023 19:58

pete-eiger force-pushed the petko/daily-notifications branch 2 times, most recently from f218d96 to 93bbb06 Compare September 28, 2023 12:28

hopeyen previously approved these changes Sep 28, 2023

View reviewed changes

feat: add divergence summary notification on a given interval

aa293a7

pete-eiger dismissed hopeyen’s stale review via aa293a7 September 28, 2023 15:15

pete-eiger force-pushed the petko/daily-notifications branch from 93bbb06 to aa293a7 Compare September 28, 2023 15:15

hopeyen self-requested a review September 28, 2023 15:16

hopeyen approved these changes Sep 28, 2023

View reviewed changes

pete-eiger self-assigned this Sep 28, 2023

pete-eiger merged commit 526630f into dev Sep 28, 2023
7 checks passed

pete-eiger deleted the petko/daily-notifications branch September 28, 2023 17:55

pete-eiger mentioned this pull request Oct 4, 2023

chore: update docs ahead of Subgraph Radio 0.1.8 release graphops/docs#81

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add divergence summary notification on a given interval #85

Add divergence summary notification on a given interval #85

pete-eiger commented Sep 21, 2023 •

edited

Loading

hopeyen Sep 26, 2023

pete-eiger Sep 27, 2023

hopeyen Sep 27, 2023

pete-eiger Sep 27, 2023 •

edited

Loading

hopeyen Sep 27, 2023

pete-eiger Sep 27, 2023

hopeyen Sep 27, 2023

pete-eiger Sep 27, 2023

hopeyen Sep 27, 2023 •

edited

Loading

pete-eiger commented Sep 28, 2023

hopeyen left a comment

Add divergence summary notification on a given interval #85

Add divergence summary notification on a given interval #85

Conversation

pete-eiger commented Sep 21, 2023 • edited Loading

hopeyen Sep 26, 2023

Choose a reason for hiding this comment

pete-eiger Sep 27, 2023

Choose a reason for hiding this comment

hopeyen Sep 27, 2023

Choose a reason for hiding this comment

pete-eiger Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

hopeyen Sep 27, 2023

Choose a reason for hiding this comment

pete-eiger Sep 27, 2023

Choose a reason for hiding this comment

hopeyen Sep 27, 2023

Choose a reason for hiding this comment

pete-eiger Sep 27, 2023

Choose a reason for hiding this comment

hopeyen Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

pete-eiger commented Sep 28, 2023

hopeyen left a comment

Choose a reason for hiding this comment

pete-eiger commented Sep 21, 2023 •

edited

Loading

pete-eiger Sep 27, 2023 •

edited

Loading

hopeyen Sep 27, 2023 •

edited

Loading