[Monitoring] Optimizing alerting code #83681

igoristic · 2020-11-18T18:02:17Z

Resolves #80065

Shared threshold/legacy alert functionality is moved into base alert/watches
processData method is now shared across all none legacy alerts
All "legacy" watches type alerts now share processData and fetchData methods
Some helper methods were also unified
Removed dead code
Mostly code that wasn't used any more due to removal of some features, mainly stack products and resolved functionality
Server/UI shared elements moved into common folder (mostly done in [Monitoring] Thread pool rejections alert #79433)
In some cases we were importing a lot of alerts related code into our frontend app which pretty much imported the whole server app due to other required imports/dependancies
Also moved all the types/helpers shared between server and public folder into common folder
Test alert triggers with multiple environments primarily CCR/remote features, and with/without security
Fix unit/functional tests

…izing_alerts

elasticmachine · 2020-11-30T10:48:13Z

Pinging @elastic/stack-monitoring (Team:Monitoring)

chrisronline · 2020-11-30T14:38:45Z

@igoristic

Thanks for putting this up!

I'm having a hard time reviewing this, as I'm not really sure what problems we're solving.

I see a few changes that maybe make the code easier to read, but it would help me greatly if you could list out what you changed and why, which will help give me context into the review.

For example, in the description:

Shared threshold alert functionality is moved into base alert

What does this mean exactly? What functionality was moved to the base?

igoristic · 2020-12-01T18:47:39Z

@chrisronline I added more details to the body, but I'm happy to do a walk-through/zoom if it helps

chrisronline

The link back from the internalFullMessage is broken - it just renders like: http://elasticsearch/nodes? Is that a regression in this PR?

chrisronline · 2020-12-02T18:07:26Z

Also, can we separate this into three separate PRs?

Shared threshold/legacy alert functionality is moved into base alert/watches
Removed dead code
Server/UI shared elements moved into common folder

I'm finding it hard to understand what code changes contribute to which.

igoristic · 2020-12-02T18:38:50Z

@chrisronline

The link back from the internalFullMessage is broken...

I think we should keep track of everything we find and compare 'em with master. If it's indeed a regression I'll address it in this branch, otherwise it should go into a separate issue

Also, can we separate this into three separate PRs...

I agree this should have been broken down into couple of PRs (from the beginning), but at this state it might be too late since all the changes are coupled together. It would take a lot of work decoupling everything, and might require some extra (throw away) work just so they could function independently. I will keep this in mind when doing future PRs similar in nature

igoristic · 2020-12-02T18:48:19Z

If you want I can go through it all via zoom, would be more than happy to

chrisronline · 2020-12-02T19:00:10Z

@jasonrhodes Curious to your thoughts here. Should we get on a zoom on discuss it more? Or do you think we can discuss everything here?

jasonrhodes · 2020-12-02T19:02:40Z

@jasonrhodes Curious to your thoughts here. Should we get on a zoom on discuss it more? Or do you think we can discuss everything here?

My advice is to do the zoom call, and if after that call, it still feels difficult to review, we should split into 2-3 PRs even if it takes some work. That is probably the safest thing to do. If the zoom call can help illuminate the changes sufficiently, we can keep it, but I'd leave that up to Chris as the reviewer.

jasonrhodes · 2020-12-02T19:04:04Z

I think we should keep track of everything we find and compare 'em with master. If it's indeed a regression I'll address it in this branch, otherwise it should go into a separate issue

This makes sense and sounds like how we should try to treat most code reviews.

chrisronline

This looks great. Thanks for hoping on a zoom to help talk it through. I only had one small comment for now

x-pack/plugins/monitoring/server/alerts/alerts_factory.test.ts

chrisronline

There seem to be at least one test removed that should be put back, but once that's done, this LGTM! Great work!

…izing_alerts

kibanamachine · 2020-12-08T06:40:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b225580

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`monitoring`	612	611	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`monitoring`	951.7KB	944.7KB	-7.0KB

Distributable file count

id	before	after	diff
`default`	47694	47692	-2

History

💚 Build #92613 succeeded 13bc1b2
💔 Build #92557 failed 5523673
💔 Build #90641 failed 9c39cfb
💔 Build #88652 failed 854f9fa

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

* [Monitoring] Optimizing alerting code (#83681) * Optimize alerting code * Merged all the branches * resolved conflict * optimizing all branches merged * Fixed tests and resolved conflicts * Fixed jest tests * Resolved merge conflicts with the alerting team's PR # Conflicts: # x-pack/plugins/monitoring/public/components/apm/instance/instance.js # x-pack/plugins/monitoring/public/components/beats/beat/beat.js * Update instance.js

igoristic · 2020-12-08T18:23:44Z

Backport:
7.x: c60bd21

igoristic added 5 commits November 16, 2020 11:49

Optimize alerting code

0873903

Merge branch 'master' of https://github.com/elastic/kibana into optim…

03eacf4

…izing_alerts

Merged all the branches

7d59917

resolved conflict

5081944

Merge branch 'master' of https://github.com/elastic/kibana into optim…

854f9fa

…izing_alerts

igoristic added enhancement New value added to drive a business result Team:Monitoring Stack Monitoring team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 18, 2020

igoristic added this to the Stack Monitoring UI 7.11 milestone Nov 18, 2020

igoristic added 5 commits November 25, 2020 23:06

resolved conflict

98b04a5

Merge branch 'master' of https://github.com/elastic/kibana into optim…

c3d61e5

…izing_alerts

Merge branch 'master' of https://github.com/elastic/kibana into optim…

daa358e

…izing_alerts

optimizing all branches merged

6f3368d

Merge branch 'master' of https://github.com/elastic/kibana into optim…

9c39cfb

…izing_alerts

igoristic marked this pull request as ready for review November 30, 2020 10:48

igoristic requested a review from a team November 30, 2020 10:48

chrisronline suggested changes Dec 2, 2020

View reviewed changes

chrisronline mentioned this pull request Dec 2, 2020

[Monitoring] Alerting action link is broken #84819

Open

chrisronline reviewed Dec 3, 2020

View reviewed changes

x-pack/plugins/monitoring/server/alerts/alerts_factory.test.ts Show resolved Hide resolved

chrisronline approved these changes Dec 7, 2020

View reviewed changes

igoristic added 7 commits December 7, 2020 15:05

Merge branch 'master' of https://github.com/elastic/kibana into optim…

6570b04

…izing_alerts

Merge branch 'master' of https://github.com/elastic/kibana into optim…

24bc860

…izing_alerts

Merge branch 'master' of https://github.com/elastic/kibana into optim…

d499ac1

…izing_alerts

Fixed tests and resolved conflicts

5523673

Fixed jest tests

13bc1b2

Merge branch 'master' of https://github.com/elastic/kibana into optim…

d7612f3

…izing_alerts

Resolved merge conflicts with the alerting team's PR

b225580

igoristic merged commit a41881d into elastic:master Dec 8, 2020

igoristic deleted the optimizing_alerts branch December 8, 2020 15:16

igoristic mentioned this pull request Dec 8, 2020

[7.x] [Monitoring] Optimizing alerting code (#83681) #85284

Merged

chrisronline mentioned this pull request Dec 16, 2020

[Monitoring][Alerting] CCR read exceptions alert #85908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Monitoring] Optimizing alerting code #83681

[Monitoring] Optimizing alerting code #83681

igoristic commented Nov 18, 2020 •

edited

Loading

elasticmachine commented Nov 30, 2020

chrisronline commented Nov 30, 2020

igoristic commented Dec 1, 2020

chrisronline left a comment

chrisronline commented Dec 2, 2020

igoristic commented Dec 2, 2020

igoristic commented Dec 2, 2020

chrisronline commented Dec 2, 2020

jasonrhodes commented Dec 2, 2020

jasonrhodes commented Dec 2, 2020

chrisronline left a comment

chrisronline left a comment

kibanamachine commented Dec 8, 2020

igoristic commented Dec 8, 2020

[Monitoring] Optimizing alerting code #83681

[Monitoring] Optimizing alerting code #83681

Conversation

igoristic commented Nov 18, 2020 • edited Loading

elasticmachine commented Nov 30, 2020

chrisronline commented Nov 30, 2020

igoristic commented Dec 1, 2020

chrisronline left a comment

Choose a reason for hiding this comment

chrisronline commented Dec 2, 2020

igoristic commented Dec 2, 2020

igoristic commented Dec 2, 2020

chrisronline commented Dec 2, 2020

jasonrhodes commented Dec 2, 2020

jasonrhodes commented Dec 2, 2020

chrisronline left a comment

Choose a reason for hiding this comment

chrisronline left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 8, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

History

igoristic commented Dec 8, 2020

igoristic commented Nov 18, 2020 •

edited

Loading