Skip to content
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

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

dcadenas
Copy link
Contributor

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:

  • Integrate these metrics into grafana and create alert for Slack notification

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10816463729

Details

  • 27 of 35 (77.14%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 69.666%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/domain/follows_differ.rs 21 23 91.3%
src/metrics.rs 6 12 50.0%
Totals Coverage Status
Change from base Build 10816339215: 0.06%
Covered Lines: 1281
Relevant Lines: 1427

💛 - Coveralls

Copy link
Member

@mplorentz mplorentz left a 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);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 👍

Comment on lines 361 to 368
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(),
));
Copy link
Contributor Author

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.

@dcadenas dcadenas merged commit d7d21e6 into main Sep 12, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants