Skip to content
This repository was archived by the owner on Sep 12, 2024. It is now read-only.

Alerts revamp #974

Merged
merged 38 commits into from
Jun 7, 2022
Merged

Alerts revamp #974

merged 38 commits into from
Jun 7, 2022

Conversation

Samyak2
Copy link
Contributor

@Samyak2 Samyak2 commented Jun 2, 2022

Tasks:

  • Update query for daily reports to get anomaly data for a single day
  • Separate alerts data for each day
  • Disable subdims
  • Change percent change logic
  • Store previous value in TriggeredAlerts
  • Update individual email alert format
  • Update individual slack alert format
  • Update reports email alert format
  • Update reports slack alert format
  • Selective sub-dimensions

Debanjan Dey and others added 15 commits May 30, 2022 13:43
added disclaimer for consolidated alerts reports
- Alerts do not consider subdim level anomalies now
- The TriggeredAlerts query for reports is now changed to use the
  anomaly timestamp and look for today-DAYS_OFFSET_FOR_ANALTYICS data
- Because of the above change, we need to ensure that there is only one
  day's data in a particular TriggeredAlerts row (since we only look for
  the first data point in a TriggeredAlerts row when querying).
Instead of splitting only during TriggeredAlerts storage time, split
before doing anything with the data.

So separate alerts are sent and separate rows of TriggeredAlerts are
used for different days.
Previous point is used for both daily and hourly time series frequency
also changed previous_value to be rounded to 2 decimals
Moved the data_datetime filter thing to a function
Since doing in the backend means the dashboard only gets data of that
day.
All day's TriggeredAlerts data is stored, but only the latest day's data
is used for sending alerts.

This prevents a spam of alerts if a user had shutdown CG for a while and
then started it up.
The CLI now allows specifying last_anomaly_timestamp to make testing
easier. Since alerts is stateful, it would have required updating the DB
everytime to trigger an alert.
Issues fixed:
- The previous data used in previous point calculation was
  (last_anomaly_timestamp - 1day) which doesn't include the previous
  point in case of an hourly KPI.
- The format function was called on all the points before segregation
  which means that it could have multiple days of data. So it was
  possible that (last_anomaly_timestamp - 1days) did not include the
  previous point for points that are older than 1 day. The whole logic
  was wrong if there were multiple days of data!
Changed made according to requirements.

Non-html changes:
- Added a few property fields to AnomalyPointFormatted that are used in
  the format
- Added `is_hourly` to AnomalyPointFormatted (could not be a computed
  property since we don't store the TS frequency in
  AnomalyPointFormatted).
- Passed date (of anomaly) to _send_email_alert to be used in the
  summary section
    - The same needs to be done for slack
@Samyak2 Samyak2 added ✨ enhancement New feature or request don't merge PR which doesn't need to be merged right now ❗alerts Alert formatting, scheduling, etc. labels Jun 2, 2022
@Samyak2 Samyak2 added this to the v0.8.0 milestone Jun 2, 2022
@netlify
Copy link

netlify bot commented Jun 2, 2022

Deploy Preview for frontend-sb canceled.

Name Link
🔨 Latest commit 4fb3064
🔍 Latest deploy log https://app.netlify.com/sites/frontend-sb/deploys/629ee4f705d1bc000965e894

@gitpod-io
Copy link

gitpod-io bot commented Jun 2, 2022

@Samyak2 Samyak2 requested a review from Amatullah June 7, 2022 05:42
@Samyak2 Samyak2 marked this pull request as ready for review June 7, 2022 05:44
@Samyak2 Samyak2 merged commit e6df226 into develop Jun 7, 2022
@Samyak2 Samyak2 deleted the feature/alerts-revamp branch June 7, 2022 05:48
@Samyak2 Samyak2 removed the don't merge PR which doesn't need to be merged right now label Jun 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
❗alerts Alert formatting, scheduling, etc. ✨ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants