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

Before sending notification verifying no changes in alert for sometime #178

Merged
merged 3 commits into from Jun 13, 2018

Conversation

GowthamShanmugam
Copy link
Collaborator

tendrl-bug-id: #177
bugzilla: 1564175

Signed-off-by: GowthamShanmugasundaram gshanmug@redhat.com

@GowthamShanmugam GowthamShanmugam requested a review from a team as a code owner June 7, 2018 09:29
@GowthamShanmugam GowthamShanmugam changed the title Before sending notification verifying no changes in alert for sometime Before sending notification verifying no changes in alert for sometime (Need to verify) Jun 7, 2018
@codecov
Copy link

codecov bot commented Jun 7, 2018

Codecov Report

Merging #178 into master will increase coverage by 0.37%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   23.26%   23.64%   +0.37%     
==========================================
  Files          12       12              
  Lines         417      423       +6     
  Branches       50       52       +2     
==========================================
+ Hits           97      100       +3     
- Misses        315      317       +2     
- Partials        5        6       +1
Impacted Files Coverage Δ
tendrl/notifier/notification/__init__.py 82.79% <50%> (-2.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58e3b80...77e69e1. Read the comment docs.

tendrl-bug-id: Tendrl#177
bugzilla: 1564175

Signed-off-by: GowthamShanmugasundaram <gshanmug@redhat.com>
lock.release()
if (datetime.utcnow() - parser.parse(
alert.time_stamp
).replace(tzinfo=None)).seconds >= 120:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it won't sent notification if any changes in alert for last 120 sec. And i have verified "dateutil" package is there in centos and rhel and fedora also

Copy link
Member

Choose a reason for hiding this comment

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

How we decide 120 sec is enough and foolproof to wait for?

@GowthamShanmugam GowthamShanmugam changed the title Before sending notification verifying no changes in alert for sometime (Need to verify) Before sending notification verifying no changes in alert for sometime Jun 7, 2018
@GowthamShanmugam
Copy link
Collaborator Author

@r0h4n @shtripat @nthomas-redhat please review

@GowthamShanmugam
Copy link
Collaborator Author

@fbalak if possible please verify and comment here

…t status for sometime

tendrl-bug-id: Tendrl#177
bugzilla: 1564175

Signed-off-by: GowthamShanmugasundaram <gshanmug@redhat.com>
…t status for sometime

tendrl-bug-id: Tendrl#177
bugzilla: 1564175
@GowthamShanmugam
Copy link
Collaborator Author

@shtripat grafana sometimes changing alert status in 60 sec but raising notification after 120 sec, it is not happening always but sometimes it is happening. Warning dashbaord raise alert raised info alert after 60 sec when threshold cross 90 and above, but critical raise alert after 120 sec only. i have checked the alert history they modified alert status in 60 sec but notification came after 120 sec only.
i feel in distributed system while pushing data some rebalance is happening so utilization value keep increasing and decreasing so some calculation they are doing

Copy link
Member

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

Looks fine. But make sure all possible scenarios are verified with trivial ones as well as corner cases.

@GowthamShanmugam
Copy link
Collaborator Author

@shtripat i have tested with @fbalak machine lot of times it works fine

@r0h4n r0h4n merged commit d4353f2 into Tendrl:master Jun 13, 2018
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.

None yet

3 participants