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

feat(notifier, api): fix sending notifications on muted and deleted triggers #946

Conversation

almostinf
Copy link
Member

@almostinf almostinf commented Oct 24, 2023

Fix sending notifications on muted and deleted triggers and muted metrics

This PR solves the problem of unwanted notifications being sent on triggers or metrics to Maintenance or removed triggers

To do this, for each notification we collect a checkData array (optimized for the case where there are many notifications with the same timestamp). If there is no trigger, the corresponding checkData = nil, whether a trigger or a metric is on Maintenance can be found by the Maintenance field

For optimization purposes, all notifications are divided into delayed and not delayed notifications and all checks needed for validation are performed on delayed notifications. Also all validations are done in a single transaction to avoid data race

Notification types have also been added:

  • valid - correct notifications
  • removed - incorrect notifications (for example, with a removed trigger).
  • ignored - notifications whose trigger or metric is on Maintenance

Divided 1 big PR into 3 parts, here are the rest:

@almostinf almostinf requested a review from a team as a code owner October 24, 2023 17:54
@almostinf almostinf changed the base branch from master to feat/add-getting-checkdatas-and-merge October 24, 2023 17:54
@almostinf almostinf changed the title feat: fix sending notifications on muted and deleted triggers feat(notifier): fix sending notifications on muted and deleted triggers Oct 24, 2023
@almostinf almostinf changed the title feat(notifier): fix sending notifications on muted and deleted triggers feat(notifier, api): fix sending notifications on muted and deleted triggers Oct 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2023

Codecov Report

Merging #946 (4a21c7e) into feat/add-redis-notifier-config (342211a) will increase coverage by 0.09%.
The diff coverage is 77.27%.

❗ Current head 4a21c7e differs from pull request most recent head fa7a3eb. Consider uploading reports for the commit fa7a3eb to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@                        Coverage Diff                         @@
##           feat/add-redis-notifier-config     #946      +/-   ##
==================================================================
+ Coverage                           69.05%   69.14%   +0.09%     
==================================================================
  Files                                 196      196              
  Lines                               11092    11199     +107     
==================================================================
+ Hits                                 7660     7744      +84     
- Misses                               2985     2999      +14     
- Partials                              447      456       +9     
Files Coverage Δ
datatypes.go 80.37% <100.00%> (+1.31%) ⬆️
database/redis/notification.go 76.42% <74.45%> (-1.15%) ⬇️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

return nil, err
}

for i, triggerID := range triggerIDs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут тоже же можно от i избавиться))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А как избавиться? Тут i нужно, для доступа к соответствующим значениям в другом массиве

@almostinf almostinf requested a review from kissken November 2, 2023 15:25
@almostinf almostinf requested a review from Tetrergeru November 3, 2023 16:22
@almostinf
Copy link
Member Author

/build

Copy link

Build and push Docker images with tag: feat-fix-sending-on-muted-and-deleted-triggers.2023-11-13.7acacd8

Base automatically changed from feat/add-getting-checkdatas-and-merge to feat/add-redis-notifier-config November 13, 2023 07:38
@almostinf almostinf merged commit 887a4d7 into feat/add-redis-notifier-config Nov 13, 2023
4 checks passed
@almostinf almostinf deleted the feat/fix-sending-on-muted-and-deleted-triggers branch November 13, 2023 07:52
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.

4 participants