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

[Ingest pipelines] Add ability to stop pipeline simulation #78183

Merged

Conversation

alisonelizabeth
Copy link
Contributor

This PR adds the ability to stop/reset testing a pipeline.

This addresses feedback received here: #74964 (review).

How to test

  1. Create a pipeline and add some sample documents (for a sample pipeline and documents, see: [Ingest pipelines] Test pipeline enhancements #74964).
  2. Click the documents dropdown, and click the "Reset" link.
  3. Verify modal opens
  4. After confirming, verify the sample documents added in step 1 no longer exist, and the status of each processor is reset to "Not run".

Screenshots

Screen Shot 2020-09-22 at 1 20 13 PM

Screen Shot 2020-09-22 at 1 20 19 PM

@alisonelizabeth alisonelizabeth added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.10.0 Feature:Ingest Node Pipelines Ingest node pipelines management labels Sep 22, 2020
@alisonelizabeth alisonelizabeth marked this pull request as ready for review September 22, 2020 17:52
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner September 22, 2020 17:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @alisonelizabeth ! I think this is a nice UX polish! I looked at the code and everything looks good to me, thanks for adding tests too!

I looked at the current UI and I have some suggestions about what I think would be cool, but not necessarily work that needs to be done in this PR.

To my mind the "edit documents" and "reset" buttons represent actions that both are scoped to documents, so it makes sense they live together as they currently do. An improvement on this, I think, is moving them out of the document selector dropdown -- so that the selector is just a selector (second image).

The controls for selecting and managing documents can be grouped visually (per image 1, not showing view output as I think this is distinct from managing documents). These controls could live in a bottom bar because they are not "primary" level actions to editing the pipeline but relevant to the process of testing.

Image 1
Screenshot 2020-09-23 at 12 44 15

Image 2
Screenshot 2020-09-23 at 12 42 25

I think it would be great to hear from @mdefazio or others on this too.

@@ -49,6 +49,12 @@ export const TestPipelineActions: FunctionComponent = () => {
});
};

const stopPipelineSimulation = () => {
setCurrentTestPipelineData({
Copy link
Contributor

Choose a reason for hiding this comment

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

nit; I think a better name for setCurrentTestPipelineData would be testPipelineDataDispatch (if we want to disambiguate) otherwise dispatch.

@mdefazio
Copy link
Contributor

One thought I had on this was instead of 'Reset', this is a toggle button that says 'Autorun test' and then 'Test paused'. (Probably better text here). Then reseting the documents can be done in the document flyout only.

@alisonelizabeth alisonelizabeth requested a review from a team as a code owner September 28, 2020 15:15
@alisonelizabeth
Copy link
Contributor Author

One thought I had on this was instead of 'Reset', this is a toggle button that says 'Autorun test' and then 'Test paused'. (Probably better text here). Then reseting the documents can be done in the document flyout only.

@mdefazio Thanks for the suggestions! Would you mind taking another look when you get a chance?

Based on your feedback on #77939, I've added a "Clear all" button to the flyout and removed the "Reset" button originally added in this PR. The "paused" toggle is an interesting idea. I think it needs to be fleshed out a little more. For example, if it was "paused", would the documents in the dropdown list be disabled? Also, would you still be able to click the "View output" button?

I think adding the "Clear all" button resolves the original concern about being stuck in testing mode and is a good step forward. I think I'd prefer to hold off on the toggle for now, unless you feel strongly about it.

Latest screenshots:

Screen Shot 2020-09-28 at 11 53 18 AM

Screen Shot 2020-09-28 at 11 53 29 AM

Screen Shot 2020-09-28 at 11 53 33 AM

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

This all looks good. I have one tiny nit: Can we update the Edit documents button so it uses the EuiPopoverFooter (and add the fullWidth prop? Might simplify things a bit but also want to make sure the button spans the width of the popover.

The toggle isn't really necessary anymore IMO.

@alisonelizabeth
Copy link
Contributor Author

Thanks @mdefazio! I was not aware of the EuiPopoverFooter. That does simplify things a bit! Mind taking one last look when you get a chance?

Screen Shot 2020-09-28 at 9 20 04 PM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
ingestPipelines 493 +10 483

async chunks size

id value diff baseline
ingestPipelines 814.4KB +5.7KB 808.7KB

History

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

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for making that edit.

phillipb added a commit to phillipb/kibana that referenced this pull request Sep 29, 2020
…-to-timeline

* 'master' of github.com:elastic/kibana: (22 commits)
  update apm index pattern (elastic#78732)
  78024: move transform out of dataset (elastic#78216)
  [QA][Code Coverage] Upload the coverage static site before ingestion (elastic#78695)
  [Discover] Make _source field not clickable (elastic#78698)
  [Fleet] Rename Ingest Manager => Fleet, Fleet => Agents in the UI (elastic#78685)
  [APM] Review feedback from distribution + transaction metrics (elastic#78752)
  [Ingest pipelines] Add ability to stop pipeline simulation  (elastic#78183)
  [CSM] Fix core vital legend background (elastic#78273)
  [Usage Collection] [schema] Support spreads + `canvas` definition (elastic#78481)
  fix lodash imports (elastic#78456)
  [Maps] Add layer type preview icons (elastic#78650)
  [APM] Use transaction metrics for distribution charts (elastic#78484)
  [Uptime] Ml anomaly alert edit (elastic#76909)
  [ML] Limit exposing shared static code through ml/public/index.ts. (elastic#77745)
  making expression debug info serializable (elastic#78727)
  fix lodahs imports in app-arch code (elastic#78582)
  Make Field a React.lazy export (elastic#78483)
  [Security Solution] Improves detections tests (elastic#77295)
  [TSVB] Different field format on different series is ignored (elastic#78138)
  RFC: Improve saved object migrations (elastic#66056)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants