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

[Uptime] Duration Anomaly Alert #71208

Merged
merged 32 commits into from Jul 14, 2020
Merged

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jul 9, 2020

Summary

Fixes: #69276

User can enable/disable anomaly alert for uptime response duration anomaly detection job.

Once user enable anomaly detection job, alert creation flyout will pop. User can define criteria and connector in flyout.

image

Alternatively, user can create alert by clicking popover in duration chart by clicking enable anomaly alert.
image

If job is already there, it will says "Disable anomaly alert" hence user can delete the alert.

Testing:

To test anomalies alerts, you can change in file xpack/plugins/uptime/server/lib/alerts/duration_anomaly.ts
line 53 from

moment(lastCheckedAt).valueOf(),

to

moment(lastCheckedAt).subtract(15, 'days').valueOf(),

and connect local kibana to cluster https://dev-next-oblt.elastic.dev/ there we have monitors like android-homepage which have data to get anomalies, specify alerts and actions, and you will get messages.

@shahzad31 shahzad31 marked this pull request as ready for review July 9, 2020 12:31
@shahzad31 shahzad31 requested review from a team as code owners July 9, 2020 12:31
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jul 9, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Code is looking great to me, and functionality seemed to work well. I did ask for tests on one file, no real concerns otherwise. @andrewvc I'd prefer if you could also test the functionality. I didn't get quite as much time as I'd like to sit with such a large addition, but it seemed to work well.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM.
Note, I will probably alter these changes very soon as I want to implement a standard way for plugins to call our shared functions when a request object is not available.

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

I notice that the "Enable Anomaly Alert" button does not change state to "Disable" without a page reload. Can we fix that?

This is a partial review to give you early feedback, more to come.


export const DurationAnomalyTranslations = {
defaultActionMessage: i18n.translate('xpack.uptime.alerts.durationAnomaly.defaultActionMessage', {
defaultMessage: `A {severity} level anomaly with score {severityScore} was detected at {anomalyStartTimestamp} on {monitor} response duration in Uptime at url {monitorUrl}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the output, this first sentence is very awkward (I know, I wrote the original). WDYT of the following?

Suggested change
defaultMessage: `A {severity} level anomaly with score {severityScore} was detected at {anomalyStartTimestamp} on {monitor} response duration in Uptime at url {monitorUrl}.
defaultMessage: `Abnormal ({severity} level) response time detected on {monitor}({monitorUrl} at {anomalyStartTimestamp}. Anomaly severity score is {severityScore}.

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

Left some additional comments. The main issue is the lack of tests at this stage, specifically functional tests. We'll need those to proceed.

@shahzad31
Copy link
Contributor Author

Left some additional comments. The main issue is the lack of tests at this stage, specifically functional tests. We'll need those to proceed.

@andrewvc added tests here https://github.com/elastic/kibana/pull/71548/files#diff-93325d7c505bcd0cbcc2c5d74267a00b

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM

});

it('can delete existing job', async () => {
if (await uptime.ml.alreadyHasJob()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems really weird, it's a test that may or may not do anything. This should just go in the before hook unless I'm missing something.

});

it('can create job successfully', async () => {
await uptime.ml.createMLJob();
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this tests both anomaly alerts and enabling ML. We should just move the existing ML tests here to prevent duplication after FF .

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
uptime 252 +2 250

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@shahzad31 shahzad31 merged commit 981d678 into elastic:master Jul 14, 2020
@shahzad31 shahzad31 deleted the anomlay-alert branch July 14, 2020 17:53
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Jul 14, 2020
shahzad31 added a commit that referenced this pull request Jul 14, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] ML Anomaly Alerting
6 participants