-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect emptied nos contact lists + metrics #17
Conversation
Pull Request Test Coverage Report for Build 10816463729Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good enough to merge, however I would really like to include a bit more metrics data before we check-off that task - either in this PR or another.
@@ -357,13 +352,19 @@ fn log_line( | |||
|
|||
// Investigate states in which there are no followees but there are unfollowed followees | |||
if followed_counter == 0 && unfollowed_counter > 0 && unchanged == 0 { | |||
metrics::sudden_follow_drops().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these metrics calls automatically include metadata like the ID of the event that triggered it and/or the author pubkey? I think those would be really useful for post-mortems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't be a good idea because of cardinality issues. In fact, we should get back to the event service and remove those metrics that include free form labels. But we do log each of these instances. So when we receive alerts, we can always track what happened through the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I finally looked up the definition of cardinality and it isn't what I thought. TIL 👍
return Some(format!( | ||
"ALL UNFOLLOWED: Npub {}: date {}, {} unfollowed, {} unchanged, {}", | ||
follower.to_bech32().unwrap_or_default(), | ||
timestamp_diff, | ||
unfollowed_counter, | ||
unchanged, | ||
event.as_json() | ||
event.as_json(), | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we log when this happens, we can filter by the nos
client, but we log for any client tag when this situation arises.
This is one of the pending tasks listed in #1.
Implements a metric and alert based on the iOS issue to tag the nos client during contact lists updates.
I assumed that the tag will be
nos
but we can always change that.I also added a new metric to detect when we ignore a contact list because we have a fresher version.
Pending work: