Skip to content

total_seconds() instead of seconds#815

Merged
RoiTabach merged 4 commits intomasterfrom
ele-692-alert-suppression-support-more-than-24
Apr 24, 2023
Merged

total_seconds() instead of seconds#815
RoiTabach merged 4 commits intomasterfrom
ele-692-alert-suppression-support-more-than-24

Conversation

@RoiTabach
Copy link
Copy Markdown
Contributor

No description provided.

@RoiTabach RoiTabach requested a review from IDoneShaveIt April 20, 2023 11:15
@linear
Copy link
Copy Markdown

linear bot commented Apr 20, 2023

ELE-692 Alert Suppression: support more than 24 hours

Describe the problem
As an Elementary user, I want alerts to be suppressed for more than 24 hours.

Describe the solution
When calculating the diff between current timestamp and last alert run, don't just take the ().seconds part but the entire timestamp diff.

Describe the tests of the solution
I think it's enough to manually test that it's working since it's such a small change

Describe the documentation
Remove the line in the Docs that says that only 0-24 hours are suppported. https://docs.elementary-data.com/quickstart/send-slack-alerts

Additional context
Jocelyn told me she wants to suppress volume anomalies for 48 hours since they have 2 days with peak sales. She just put 24h there. Norm Warren sent in support an example where he tried to suppress for 72 hours.

Reviewer
Idan has the most context

@github-actions
Copy link
Copy Markdown
Contributor

👋 @RoiTabach
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@RoiTabach
Copy link
Copy Markdown
Contributor Author

Docs changes:
Before:
image

image

After:
image

image

@RoiTabach
Copy link
Copy Markdown
Contributor Author

Expand the unit test on get_suppressed_alerts , have all the examples be 24 hours.

Copy link
Copy Markdown
Contributor Author

@RoiTabach RoiTabach left a comment

Choose a reason for hiding this comment

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

.

@RoiTabach RoiTabach marked this pull request as ready for review April 23, 2023 11:26
Copy link
Copy Markdown
Contributor

@IDoneShaveIt IDoneShaveIt left a comment

Choose a reason for hiding this comment

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

LGTM!

@RoiTabach RoiTabach merged commit 06173a3 into master Apr 24, 2023
@RoiTabach RoiTabach deleted the ele-692-alert-suppression-support-more-than-24 branch April 24, 2023 06:21
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.

2 participants