Add request flyouts to JSON watch form and Threshold Watch edit form. #43232

cjcenizal · 2019-08-13T22:42:05Z

This shows the request for JSON and Threshold watches. The logic for generating a request from the threshold watch form was distributed throughout the client and server and required significant refactoring for me to use it on the client.

Reviewers

I recommend reviewing the commit history first to get a feel of the intention behind each commit, and then reviewing each commit individually to follow my workflow. This way you'll be reviewing one cohesive group of changes at a time, which should be easier to follow and review.

Notable changes

I'm concerned about some specific changes to the code. I'll add comments on the relevant lines.

I've made some subtle changes to the action validation logic. I need to verify with @sebelga that the validation still behaves as desired.
I've removed the boom error logic from the actions. Originally this would have thrown an error if incorrect payloads were sent to the API but I don't believe this is needed since we control the UI that consumes the API.

To-do

Verify that Monitoring Watch behavior is still correct.
- I learned from Chris Roberson that Monitoring doesn't make use of any of Watcher's Kibana API endpoints. So we can safely remove awareness of Monitoring from de/serialization logic.
Verify that APM's dependency upon our API isn't broken.
Test Email action and make sure it's being created correctly and is accurately represented in the request flyout.
Add tests for serialization functions.
register_execute_route and register_visualize_route depend upon Watch.fromDownstreamJson, which is what the serialization functions replace. I need to verify I haven't broken these routes. DONE: these routes look like they are functioning correctly, though there is still plenty of room for improvement and refactoring. Though that work is outside the scope of this PR.

Release note

This is a minor enhancement to display the underlying Elasticsearch endpoint in Watcher's create and edit forms. Users can copy and paste this endpoint and payload to execute the request manually or use it within a script or other automated system.

elasticmachine · 2019-08-13T22:42:07Z

Pinging @elastic/es-ui

elasticmachine · 2019-08-13T23:42:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-14T01:22:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-08-14T02:52:56Z

Retest

elasticmachine · 2019-08-14T03:57:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth

@cjcenizal tested locally and LGTM. Thanks for adding this!

One thing to note, the name field in the UI is actually added to the metadata field on the server side (https://www.elastic.co/guide/en/elasticsearch/reference/current/watcher-api-put-watch.html#_request_body_7).

This is more of a UI thing, so not sure if we need to surface it in the request flyout, but just wanted to point it out.

…hresholdWatch function. - Refactor build helpers to accept specific arguments instead of the entire watch object, to make dependencies more obvious.

- Move action models into common directory. - Remove boom error reporting from action models because this is an unnecessary level of defensiveness since we control the UI that consumes this API. - Make changes to action model valiation which may or may not break the UX. - Add serializeJsonWatch function and consume it within the JSON Watch edit form.

elasticmachine · 2019-08-14T23:17:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-14T23:35:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-15T01:48:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

- Convert tests from Mocha to Jest. - Remove mocks and fix assertions that depended upon mocked dependencies. These assertions were low-value because they tested implementation details. - Remove other assertions based upon implementation details.

elasticmachine · 2019-08-15T06:05:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…request-flyout

…he create endpoint.

cjcenizal · 2019-09-19T19:55:16Z

@elastic/apm I believe APM consumes the Kibana Watcher API. Could you please verify that I haven't broken anything you depend upon in this PR?

elasticmachine · 2019-09-19T22:04:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga

LGTM. I haven't tested locally. I guess that all the tests you removed is because the UI has changed so it is probably better for @alisonelizabeth to make sure those are not needed anymore.

I really wish Watcher could be refactored at some point, the concept fromUpStream fromDownstream makes this always dificult to go back to this app 😊

sebelga · 2019-09-23T08:41:31Z

x-pack/legacy/plugins/watcher/common/models/action/action.test.js

@@ -35,43 +31,13 @@ describe('action', () => {
        };
      });

-      it(`throws an error if no 'id' property in json`, () => {


I am not the last one that changed the code so I am guessing that if your removed all these tests is because we don't need those validations anymore in the UI.

The original implementation threw boom errors when these properties were missing in fromUpstreamJson, which I believe means it was making assertions against the response from Elasticsearch when retrieving a watch with actions. I removed this logic because it both seems like an extreme corner case and the user also wouldn't have a clear way of fixing the problem.

I see. Sounds more like an api integration tests indeed. 👍

💯 I was thinking the same thing!

sorenlouv

This seems to break the APM integration with watcher. We do a
PUT /api/watcher/watch/{watcher_id} which returns a 404. If you changed the endpoint can you perhaps update it accordingly in:
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/apm/public/services/rest/watcher.js

Thanks :)

EDIT: above might not be correct. But I can't verify whether watcher works because I get the following error when going to the watcher ui:

Uncaught TypeError: Cannot read property 'space' of null
    at new NavControlPopover (nav_control_popover.tsx:109)
    at constructClassInstance (react-dom.development.js:11312)
    at updateClassComponent (react-dom.development.js:14481)
    at beginWork (react-dom.development.js:15438)
    at performUnitOfWork (react-dom.development.js:19106)
    at workLoop (react-dom.development.js:19146)
    at HTMLUnknownElement.callCallback (react-dom.development.js:149)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:199)
    at invokeGuardedCallback (react-dom.development.js:256)
    at replayUnitOfWork (react-dom.development.js:18372)

Looks like Watcher expects Spaces to be enabled, and fails if it isn't.

…request-flyout

elasticmachine · 2019-09-23T19:45:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-24T00:44:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…request-flyout

…riting it.

…ing conflict resolution in last merge.

cjcenizal · 2019-09-24T21:09:09Z

@sqren Could you take another look? The problem was that the code was overwriting provided metadata. I've updated the logic to add onto it instead of overwrite it.

elasticmachine · 2019-09-24T23:00:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-24T23:46:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv

Metadata works again 🎉
Thanks for fixing this 👍

…elastic#43232) * Refactor watch serialization logic into common serialization functions. - Refactor build helpers to accept specific arguments instead of the entire watch object, to make dependencies more obvious. - Move action models into common directory. * Remove boom error reporting from action models because this is an unnecessary level of defensiveness since we control the UI that consumes this API. * Convert tests from Mocha to Jest. - Remove mocks and fix assertions that depended upon mocked dependencies. These assertions were low-value because they tested implementation details. - Remove other assertions based upon implementation details. * Remove serializeMonitoringWatch logic, since Monitoring doesn't use the create endpoint.

…#43232) (#46634) * Refactor watch serialization logic into common serialization functions. - Refactor build helpers to accept specific arguments instead of the entire watch object, to make dependencies more obvious. - Move action models into common directory. * Remove boom error reporting from action models because this is an unnecessary level of defensiveness since we control the UI that consumes this API. * Convert tests from Mocha to Jest. - Remove mocks and fix assertions that depended upon mocked dependencies. These assertions were low-value because they tested implementation details. - Remove other assertions based upon implementation details. * Remove serializeMonitoringWatch logic, since Monitoring doesn't use the create endpoint.

elasticmachine · 2019-09-25T22:11:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-09-26T03:27:39Z

@sebelga I'd love to get rid of the upstream and downstream stuff too. The good news is this PR is a big step in that direction. Now that we have serialization functions, it should be a straightforward process of migrating over the last few consumers of get upstreamJson, fromDownstreamJson, and getPropsFromDownstreamJson when we need to. I believe this is just register_execute_route.js and register_visualize_route.js. Then we can delete those methods.

At that point we would need to write the complementary deserialization functions, and migrate over the various consumers of get downstreamJson and fromUpstreamJson. This will be straightforward. The most tedious aspect will be updating all of the tests, though I expect we can just delete some of them.

That would take care of this stuff on the server, but there are still client-side models with get upstreamJson and fromUpstreamJson. Migrating these to use the deserializers will probably be straightforward but tedious.

Because all of this work is time-consuming and prone to introducing errors if not done carefully, I'd like to defer it until a feature or bugfix necessitates it.

sebelga · 2019-09-26T13:43:26Z

@cjcenizal Yeah I understand the risk/time-consuming part. It will be very important to type well the contract before starting the refactor. This means making sure there aren't any instances of any in the consumers and typing the function return statement correctly. Once this is done correctly, the refactor should not be too time-consuming or dangerous as tsc will start throwing error everywhere the contract isn't respected.

Add request flyout to JSON watch form.

458db73

cjcenizal added release_note:enhancement Feature:Watcher v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.4.0 labels Aug 13, 2019

cjcenizal requested a review from alisonelizabeth August 13, 2019 22:42

Fix TS errors.

07db8a0

alisonelizabeth approved these changes Aug 14, 2019

View reviewed changes

cjcenizal added 2 commits August 14, 2019 15:20

Refactor threshold watch serialization logic into a common serializeT…

2901eab

…hresholdWatch function. - Refactor build helpers to accept specific arguments instead of the entire watch object, to make dependencies more obvious.

cjcenizal force-pushed the watcher-show-request-flyout branch from eaff848 to 215cd03 Compare August 14, 2019 22:25

cjcenizal force-pushed the watcher-show-request-flyout branch from bc2b99b to 86d274a Compare August 15, 2019 04:24

cjcenizal force-pushed the watcher-show-request-flyout branch from 86d274a to cc91b97 Compare August 15, 2019 05:03

cjcenizal added v7.5.0 and removed v7.4.0 labels Sep 13, 2019

cjcenizal added 2 commits September 19, 2019 12:24

Merge branch 'master' of github.com:elastic/kibana into watcher-show-…

6e27036

…request-flyout

Remove serializeMonitoringWatch logic, since Monitoring doesn't use t…

3cea9f0

…he create endpoint.

cjcenizal requested review from sebelga and chrisronline September 19, 2019 19:52

Fix layout error with form buttons in Create Threshold Alert form.

c21ef6d

sebelga approved these changes Sep 23, 2019

View reviewed changes

sorenlouv requested changes Sep 23, 2019

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into watcher-show-…

55613ab

…request-flyout

Add serialization unit tests.

07438d7

cjcenizal added 4 commits September 24, 2019 12:49

Merge branch 'master' of github.com:elastic/kibana into watcher-show-…

d5f3bda

…request-flyout

Fix APM's use cases by adding onto supplied metadata instead of overw…

d64984d

…riting it.

Reinstate JSON watch request flyout that was accidentally removed dur…

08e4e0c

…ing conflict resolution in last merge.

Fix JSON watch form spacing.

c8d46aa

Remove metadata from request preview for threshold alerts.

fb8941f

sorenlouv approved these changes Sep 25, 2019

View reviewed changes

cjcenizal merged commit 178d47c into elastic:master Sep 25, 2019

cjcenizal deleted the watcher-show-request-flyout branch September 25, 2019 20:17

cjcenizal changed the title ~~Add request flyout to JSON watch form.~~ Add request flyouts to JSON watch form and Threshold Watch edit form. Sep 25, 2019

cjcenizal mentioned this pull request Sep 25, 2019

[7.x] Add request flyouts to JSON watch form and Threshold Watch edit form. (#43232) #46634

Merged

This was referenced Jan 7, 2020

Watcher Threshold Alert creates empty order in terms agg, throws errors #53974

Closed

Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function #54391

Merged

cjcenizal mentioned this pull request Apr 20, 2020

Threshold watches contain irregular and unnecessary configuration #64022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add request flyouts to JSON watch form and Threshold Watch edit form. #43232

Add request flyouts to JSON watch form and Threshold Watch edit form. #43232

cjcenizal commented Aug 13, 2019 •

edited

Loading

elasticmachine commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

elasticmachine commented Aug 14, 2019

cjcenizal commented Aug 14, 2019

elasticmachine commented Aug 14, 2019

alisonelizabeth left a comment

elasticmachine commented Aug 14, 2019

elasticmachine commented Aug 14, 2019

elasticmachine commented Aug 15, 2019

elasticmachine commented Aug 15, 2019

cjcenizal commented Sep 19, 2019

elasticmachine commented Sep 19, 2019

sebelga left a comment

sebelga Sep 23, 2019

cjcenizal Sep 23, 2019 •

edited

Loading

sebelga Sep 24, 2019

cjcenizal Sep 24, 2019

sorenlouv left a comment •

edited

Loading

elasticmachine commented Sep 23, 2019

elasticmachine commented Sep 24, 2019

cjcenizal commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

sorenlouv left a comment

elasticmachine commented Sep 25, 2019

cjcenizal commented Sep 26, 2019 •

edited

Loading

sebelga commented Sep 26, 2019

Add request flyouts to JSON watch form and Threshold Watch edit form. #43232

Add request flyouts to JSON watch form and Threshold Watch edit form. #43232

Conversation

cjcenizal commented Aug 13, 2019 • edited Loading

Reviewers

Notable changes

To-do

Release note

elasticmachine commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

💔 Build Failed

elasticmachine commented Aug 14, 2019

💔 Build Failed

cjcenizal commented Aug 14, 2019

elasticmachine commented Aug 14, 2019

💚 Build Succeeded

alisonelizabeth left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 14, 2019

💔 Build Failed

elasticmachine commented Aug 14, 2019

💔 Build Failed

elasticmachine commented Aug 15, 2019

💔 Build Failed

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

cjcenizal commented Sep 19, 2019

elasticmachine commented Sep 19, 2019

💔 Build Failed

sebelga left a comment

Choose a reason for hiding this comment

sebelga Sep 23, 2019

Choose a reason for hiding this comment

cjcenizal Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

sebelga Sep 24, 2019

Choose a reason for hiding this comment

cjcenizal Sep 24, 2019

Choose a reason for hiding this comment

sorenlouv left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 23, 2019

💚 Build Succeeded

elasticmachine commented Sep 24, 2019

💚 Build Succeeded

cjcenizal commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

💚 Build Succeeded

elasticmachine commented Sep 24, 2019

💚 Build Succeeded

sorenlouv left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 25, 2019

💔 Build Failed

cjcenizal commented Sep 26, 2019 • edited Loading

sebelga commented Sep 26, 2019

cjcenizal commented Aug 13, 2019 •

edited

Loading

cjcenizal Sep 23, 2019 •

edited

Loading

sorenlouv left a comment •

edited

Loading

cjcenizal commented Sep 26, 2019 •

edited

Loading