-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Polish pipeline debugging workflow #76058
[Ingest pipelines] Polish pipeline debugging workflow #76058
Conversation
Here are some initial thoughts:
|
Thanks @mdefazio for the initial feedback!
👍 I don't have a strong opinion about this, but don't mind implementing it.
For me, it feels a little cumbersome to first add JSON for the sample documents and then move your mouse back up to click "Run the pipeline". When filling out a form, I typically would expect the submit button to be below. I understand this is only one field though so we might treat it a little differently.
Can you clarify what you mean here? JL is working on building out the forms for each processor type.
If you click the "Output" tab, the simulation will rerun with your unsaved changes, so you are able to toggle back and forth between configuration and output and make changes. I understand this may not be obvious though. If you cancel, the output will be reset to the original state. I don't think we've typically disabled forms if the user has not changed any values. Similarly, we don't have confirmation modals for exiting forms with unsaved changes. I think the latter is needed, but just wanted to point out that it is probably an issue we need to solve across several of our apps with forms. |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
There was a problem hiding this 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 had one code change in view that would be great to get your thoughts on, but I view it as non-blocking for this contribution.
In terms of UX everything worked as expected from your PR description. I think it would be great if we can make the documents dropdown a little bit bigger:
I really like the "Data in"/"Data out" sections added per processor. It gives users a lot of insight into the per-step transformations. I wonder if it is worth add a way to view these on the processor itself? I'd be interested to hear your thoughts @mdefazio !
return ( | ||
<EditProcessorForm | ||
{...rest} | ||
processor={processor!} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to remove the !
[form, onClose, onSubmit] | ||
); | ||
|
||
const resetProcessors = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we wrap this in a useCallback
?
@@ -149,6 +156,8 @@ export const PipelineProcessorsContextProvider: FunctionComponent<Props> = ({ | |||
}); | |||
break; | |||
case 'managingProcessor': | |||
setUnsavedProcessorFormData(processorTypeAndOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If unsavedProcessorDataFormData
is only used inside of the form can we push the state further down the component tree? Perhaps the state tracking this could live in the processor settings form component where the tabs get rendered?
Additionally, I think the getDefaultProcessorOptions
function that we are passing into the processor settings form fields component could be slightly updated. Maybe something like in processor_form.container
:
// ...
const unsavedState = useRef<{} | undefined>();
// we set unsavedState.current whenever a form update happens...
const getProcessor = useCallback((): ProcessorInternal => {
let options;
if (unsavedFormData) {
options = unsavedFormData.current.options;
} else {
defaultFields = { fields: processor?.options ?? {} };
}
return { ...processor, options: };
}, [processor]);
// ...
Then we always sync using the form.subscribe callback that is calling onFormUpdate
. I think one step further would be to hold "unsaved" state in a useRef
. Did not test this 😅 - lmk what you think!
Also, then instead of passing in processor
and getDefaultOptions
to processor fields form, we just pass in the getProcessor
callback since it is a "computed" value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion. I went ahead and implemented this, but only update the ref on submit rather than the form.subscribe
callback, as the form data ends up getting reset on tab change here.
It is a little bit tricky to assess, but I am assuming we have not introduced changes that require test coverage changes. |
Thanks for the review @jloleysens! I addressed your feedback via 1aa0ef4. I also made the dropdown wider and fixed an alignment issue with the custom icons within the callout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments regarding design and UX:
- Let's change success
check
to acheckInCircle
so it stands out more. - Change the dot icon color to
euiColorLightShade
so it doesn't stand out more. My thinking here is that by making more contrast between these it will be more obvious where in the list the pipeline stopped. - Perhaps change 'Test' to 'Test pipeline' in the updated testing module (also would match the flyout header title) so it's clearer what this area does
- I think we could add a status and timestamp in the flyout that indicates that testing state. I find myself unsure when it ran and what action in the UI will trigger a new run. (maybe a larger UI issue here?)
- Add a
play
icon to 'Run the pipeline' button in document tab since we have the refresh icon in the output tab - 'Output' button in test module should read 'View output' so it represents an action
- Maybe not for this PR, but I noticed that the 'Refresh' button in the pipeline list is using the secondary color—this should be
primary
to match the Create button
@elasticmachine merge upstream |
Thanks @mdefazio! Great suggestions. I addressed most of your comments. Would you mind taking another look? I'm still not 100% on the best approach for adding a status/timestamp to the flyout. As mentioned 1:1, perhaps it's best to punt on this until we get some user testing.
Great catch. I took a look at some of our other UIs, and we're also using the secondary color 🤔. The button was not introduced as part of this PR so I'm going to leave as-is for now, and it also might be something we need to address across other apps. Latest screenshots: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I see we don't have the euiColorLightShade
as an option for the icon. Maybe this is something we can look into on the EUI side. But we'd obviously like to remove those hex values in a follow up PR.
💛 Build succeeded, but was flaky
Build metrics@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
…s-for-710 * 'master' of github.com:elastic/kibana: (95 commits) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) [@kbn/utils] Adds missing dependency (elastic#77536) Add the Enterprise Search logo to the Overview search result (elastic#77514) [Logs UI] [Metrics UI] Remove saved object field mappings (elastic#75482) [Security Solution][Exceptions] Exception modal bulk close option only closes alerts generated by same rule (elastic#77402) Adds @kbn/utils package (elastic#76518) Map layout changes (elastic#77132) [Security Solution] [Detections] EQL Rule Creation (elastic#76831) Adding test user to maps tests under documents source folder (elastic#77245) Suppress error logs when clients connect over HTTP instead of HTTPS (elastic#77397) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx
* master: (55 commits) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) ...
* master: (54 commits) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) ...
This PR contains a number of general usability and design improvements.
I tried to break the work up logically by commit (although I didn’t do the best job toward the end 😅). To test, please follow the same instructions in #74964 and ensure there are no regressions.
Notable changes:
manage_processor_form
). I felt this was getting overly complex, and there was enough divergence that it made sense to create two separate components.What’s missing:
Screenshots