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.

Sign up for GitHub

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

Jump to bottom

Fix enable alert API to schedule task after alert is updated #55095

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Fix enable alert API to schedule task after alert is updated #55095

merged 1 commit into from
Jan 17, 2020

Conversation

mikecote
Copy link
Contributor

Resolves #54125

Re-enable flaky test in regards to enable alert API. The cause of the flakiness was that the task sometimes executed before the alert was even updated causing errors. It is also good to schedule the task after updating the alert in the scenario the user doesn't have access to update the alert saved object.

@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Jan 16, 2020
@mikecote mikecote self-assigned this Jan 16, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

@mikecote
Copy link
Contributor Author

Running test 42x to ensure no flakiness https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/137/.

gmmorris
gmmorris approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, though the need for yet another update worries me in the context of the ongoing conversation about stress caused by the volatility of how we're using Saved Objects.

Wondering if we should start looking for a better way to do this kind of thing. :/

@mikecote
Copy link
Contributor Author

mikecote commented Jan 17, 2020 •
edited
Loading

LGTM, though the need for yet another update worries me in the context of the ongoing conversation about stress caused by the volatility of how we're using Saved Objects.

Wondering if we should start looking for a better way to do this kind of thing. :/

Thanks for the review! I'm not as concerned when it's an API call instead of something done on a recurring basis (ex: alert execution every 5s). These can take a bit longer and not be as performant to satisfy functionality (within certain limits).

pmuellr
pmuellr approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@pmuellr
Copy link
Member

pmuellr commented Jan 17, 2020

During stress testing, I believe I saw "Invalidated API Key" messages on alert deletion, but wasn't able to get it repro-able, and didn't notice anything while poking around the code. Just to be on the lookout - there may be more lurking ...

@mikecote
Copy link
Contributor Author

@pmuellr

During stress testing, I believe I saw "Invalidated API Key" messages on alert deletion, but wasn't able to get it repro-able, and didn't notice anything while poking around the code. Just to be on the lookout - there may be more lurking ...

Thanks for the heads up! There's definitely some corner cases that can cause these types of errors. I'm hoping to resolve as much of them with this issue in 7.7 #53868.

@mikecote mikecote merged commit 9c2d778 into elastic:master Jan 17, 2020
@mikecote mikecote mentioned this pull request Jan 17, 2020
Merged
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 17, 2020
@mikecote
Fix enable API to schedule task after alert is updated (elastic#55095)
b0ab36d
mikecote added a commit that referenced this pull request Jan 17, 2020
@mikecote
Fix enable API to schedule task after alert is updated (#55095) (#55192)
cc8ed75
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
@gmmorris
Merge remote-tracking branch 'upstream/master'
edb0145 
* upstream/master: (83 commits)
  [Reporting] Fix map tiles not loading by using Chrome's Remote Protocol (elastic#55137)
  [Data Plugin] combine autocomplete provider and suggestions provider (elastic#54451)
  resolves elastic#53038 - remove references to specific license levels (elastic#53858)
  highlighting rules should still know about url parts when in sql state (elastic#55200)
  [Metric] convert mocha tests to jest (elastic#54054)
  [skip-ci] Update vector styling docs for 7.6 UI changes and new features (elastic#55087)
  Fix enable API to schedule task after alert is updated (elastic#55095)
  chore(NA): add 7.6 branch to the list of backport branches (elastic#54998)
  Convert tests to jest in vis_type_timeseries/public & common folders (elastic#55023)
  [ML] Accessibility fix for structural markup on table rows (elastic#55075)
  [Mappings editor] include/exclude fields only support custom options (elastic#54949)
  [Vis] Move Timelion Vis to vis_type_timelion (elastic#52069)
  Deprecate `chrome.navlinks.update` and add documentation (elastic#54893)
  [ML] Single Metric Viewer: Fix time bounds with custom strings. (elastic#55045)
  [Vis: Default editor] EUIficate and Reactify the sidebar (elastic#49864)
  [Mappings editor] Fix cannot set boolean value for "null_value" param (elastic#55015)
  [SIEM] Adds support for apm-* to the network map (elastic#54876)
  [Reporting] Define shims of legacy dependencies (elastic#54082)
  Resolver is overflow: hidden to prevent obscured elements from showing up (elastic#55076)
  Upgraded EUI to 18.2.1 (elastic#55090)
  ...
@mikecote mikecote mentioned this pull request Jan 24, 2020
Merged
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 24, 2020
@mikecote
Fix enable API to schedule task after alert is updated (elastic#55095)
d28ea6f
mikecote added a commit that referenced this pull request Jan 24, 2020
@mikecote
Fix enable API to schedule task after alert is updated (#55095) (#55846)
ab58a71
@mikecote mikecote mentioned this pull request Jan 24, 2020
Merged
@mikecote mikecote added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmmorris gmmorris gmmorris approved these changes

@pmuellr pmuellr pmuellr approved these changes

Assignees

@mikecote mikecote

Labels
Feature:Alerting release_note:fix review Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.6.0 v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
5 participants
@mikecote @elasticmachine @kibanamachine @pmuellr @gmmorris