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

[Alerting] replace index threshold graph usage of watcher APIs with new API #59385

Merged
merged 6 commits into from
Mar 6, 2020
Merged

[Alerting] replace index threshold graph usage of watcher APIs with new API #59385

merged 6 commits into from
Mar 6, 2020

Conversation

pmuellr
Copy link
Member

@pmuellr pmuellr commented Mar 4, 2020

Summary

Part of the removal of the watcher APIs used in the alerting ui plugin. Replaced with a new API provided by the built-in index threshold alertType.

Note that I moved some portable code in the alerting and alerting_builtins plugins from server to common, to reuse in the UI.

And time_buckets are gone :-)

I had to thread the alert interval into the form, wasn't currently being used, but we end up needing it to generate the graph data with the new APIs.

@pmuellr pmuellr requested a review from a team as a code owner March 4, 2020 22:51
@pmuellr pmuellr added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Mar 4, 2020
@elasticmachine
Copy link
Contributor

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

@pmuellr pmuellr added v7.7.0 release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels Mar 4, 2020
@pmuellr pmuellr force-pushed the alerting/index-threshold-graph branch from 4ab53f6 to dc70d72 Compare March 5, 2020 01:08
@pmuellr pmuellr force-pushed the alerting/index-threshold-graph branch from dc70d72 to 9e3843a Compare March 5, 2020 01:52
@pmuellr pmuellr changed the title [Alerting] replace index threshold graph usage of watcher APIs with new APIs [Alerting] replace index threshold graph usage of watcher APIs with new API Mar 5, 2020
@pmuellr
Copy link
Member Author

pmuellr commented Mar 5, 2020

If you want to see the graph in action, you can use this simulator which can generate interesting data for the graph, changing every second: https://github.com/pmuellr/es-apm-sys-sim

mikecote
mikecote approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍 just one comment but awesome to see time buckets are gone!!

x-pack/plugins/triggers_actions_ui/kibana.json Outdated
@@ -3,5 +3,5 @@
"version": "kibana",
"server": false,
"ui": true,
"requiredPlugins": ["management", "charts", "data"]
"requiredPlugins": ["management", "charts", "data", "alerting", "alertingBuiltins"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if these should be optional dependencies? This would allow the scenario the alerting plugin is disabled but actions / connectors plugin is enabled. (UI would still show)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, that makes sense. I think for both of these, I ended up moving some code to common, that this plugin imported, so wanted to note the dependency, somehow. Of course, it doesn't matter if those deps aren't enabled :-). Optional makes more sense.

YulNaumenko
YulNaumenko approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko 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 Author

pmuellr commented Mar 5, 2020

@elasticmachine merge upstream

@pmuellr
Copy link
Member Author

pmuellr commented Mar 6, 2020

@elasticmachine merge upstream

@pmuellr
Copy link
Member Author

pmuellr commented Mar 6, 2020

@elasticmachine merge upstream

@pmuellr
Copy link
Member Author

pmuellr commented Mar 6, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@pmuellr pmuellr merged commit ba40d25 into elastic:master Mar 6, 2020
@pmuellr pmuellr mentioned this pull request Mar 6, 2020
Merged
pmuellr added a commit to pmuellr/kibana that referenced this pull request Mar 6, 2020
@pmuellr
[Alerting] replace index threshold graph usage of watcher APIs with n…
fcba2c1 
…ew API (elastic#59385)

Changes the alerting UI to use the new time series query HTTP endpoint provided by the builtin index threshold alertType; previously it used a watcher HTTP endpoint.

This is part of the ongoing index threshold work tracked in elastic#53041
pmuellr added a commit that referenced this pull request Mar 6, 2020
@pmuellr
[Alerting] replace index threshold graph usage of watcher APIs with n…
0f728d7 
…ew API (#59385) (#59557)

Changes the alerting UI to use the new time series query HTTP endpoint provided by the builtin index threshold alertType; previously it used a watcher HTTP endpoint.

This is part of the ongoing index threshold work tracked in #53041
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 9, 2020
@gmmorris
Merge branch 'master' into alerting/view-in-app
57d5b4f 
* master: (154 commits)
  Add an optional authentication mode for HTTP resources (elastic#58589)
  Implement embeddable drilldown menu options (elastic#59232)
  [Alerting] "Create alert" graph visualization design improvements (elastic#59399)
  Alerting update route throttle property is missing (elastic#59580)
  [SIEM] Adds 'Load prebuilt rules' Cypress test (elastic#59529)
  Show error if field is not found during filter rendering (elastic#59298)
  Navigate back to discover app during test, because the saved search from the preceding test has major performance problems when used with this test (elastic#59571)
  Check for alert dialog when doing a force logout (elastic#59329)
  ensure fs deletes are not cwd dependent (elastic#59570)
  Empty message for APM service map (elastic#59518)
  [Drilldowns] <ActionWizard/> Component (elastic#59032)
  [Reporting] Improve the page exit error messages (elastic#59351)
  Ensure logged out starting state for tests that need it (elastic#59322)
  Hide input value from kbn-config-schema error messages (elastic#58843)
  [ML] Transforms: Migrate client plugin to NP. (elastic#59443)
  [ML] Disable failing functional tests
  [SIEM] Update Timeline to use the latest euiFlyoutBody style (elastic#59524)
  Temporarily remove the project mappings for PR labels (elastic#59493)
  [Alerting] replace index threshold graph usage of watcher APIs with new API (elastic#59385)
  [ML] Show view series link in anomalies table for machine_learning_user role (elastic#59549)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@mikecote mikecote mikecote approved these changes

Assignees
No one assigned
Labels
backported Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pmuellr @elasticmachine @kibanamachine @mikecote @YulNaumenko