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

[Runtime field editor] Preview field against cluster data #101398

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jun 4, 2021

This PR adds the previewing of the runtime field in the context of the latest documents from the index pattern.

When we open the index pattern field editor we load the latest 50 documents from the cluster and display in the Preview panel their fields (initially we only display the first 7 fields).

The error handling currently in place is basic and will be improved on as a separate chunk of work.

Notes on implementation

I have used a virtual list to render the list of fields. This allows the UI to support huge list of fields (5000+) that can occur in certain scenarios. This implies that the list items must have a fix height and that we give an height to the scrollable area.
The height is calculated dynamically with a <EuiResizeObserver /> and passed down to the <VirtualList /> component so we can make use of the full height of the flyout panel to render the fields on any screen size.

How to test

  • Make sure to load the Kibana sample data logs

  • Navigate to the index pattern management app and select the "kibana_sample_data_logs" index pattern

  • Click "Add field"

  • Give a name to the field and toggle the "Set value"

  • The Preview pannel should open with the empty prompt

  • Add a script (e.g. emit("hello"))

  • The Preview panel should update and display the runtime field (name and value) along with the other documents fields

  • Click the "Show more" button to display all the fields

  • The fields should expand and you should be able to scroll all the fields.

  • Now let's do some calculation with our script: change the field type to "long" and the script to emit(doc['bytes'].value * 2)

  • The preview should correctly show the double that what the "bytes" field indicates.

  • Change the document ID and provide a wrong ID

  • There should be an error

  • Now enter a valid ID (e.g. "pyDLo3kBEdBikFTNyctP")

  • You should see that document in the preview and the "Previous" / "Next" button should not be visible anymore

  • Click on the "Load documents from cluster" button

  • You should be back navigating the list of documents loaded from the cluster

Test with 5000 fields

As we are using a virtual list, the number of fields do not have a performance impact in the rendering.
Open the field_list.tsx file and replace the fieldsValues declaration block on L49 with

const fieldsValues = useMemo(
    () =>
      new Array(5000).fill('').map((_, i) => ({
        key: `key__${i}`,
        value: `value__${i}`,
      })),
    []
  );

Changes to the Core public API

This is a minor change.

I have added the hideCloseButton EuiFlyoutProp to be able to hide the close button in the flyout when using the overlays.openFlyout() core handler.

Upcoming work to be done

  • Search fields & pin fields at the top of the list
  • Error handling
  • Preview the "Format" that user might define in the editor
  • Component integration tests

Screenshots

Screenshot 2021-06-04 at 17 11 52

Screenshot 2021-06-04 at 17 12 56

@sebelga sebelga marked this pull request as ready for review June 7, 2021 13:46
@sebelga sebelga requested review from a team as code owners June 7, 2021 13:46
@sebelga sebelga added Project:RuntimeFields Team:AppServices Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Jun 7, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@sebelga
Copy link
Contributor Author

sebelga commented Jun 7, 2021

@yuliacech I've added the overlay to the flyout and now I get the following accessibility issue (basically that the menu on the left is not clickable anymore). Have you solved this somewhere?

1)    Management
     │       Advanced settings:
     │
     │      retry.try timeout: ElementClickInterceptedError: element click intercepted: Element <a class="euiSideNavItemButton euiSideNavItemButton--isClickable" href="/app/management/kibana/settings" rel="noreferrer" data-test-subj="settings">...</a> is not clickable at point (121, 770). Other element would receive the click: <div class="euiOverlayMask euiOverlayMask--belowHeader" data-aria-hidden="true" aria-hidden="true"></div>
     │   (Session info: headless chrome=91.0.4472.77)
     │     at Object.throwDecodedError (/dev/shm/workspace/parallel/24/kibana/node_modules/selenium-webdriver/lib/error.js:550:15)
     │     at parseHttpResponse (/dev/shm/workspace/parallel/24/kibana/node_modules/selenium-webdriver/lib/http.js:565:13)
     │     at Executor.execute (/dev/shm/workspace/parallel/24/kibana/node_modules/selenium-webdriver/lib/http.js:491:26)
     │     at runMicrotasks (<anonymous>)
     │     at processTicksAndRejections (internal/process/task_queues.js:95:5)
     │     at Task.exec (/dev/shm/workspace/parallel/24/kibana/test/functional/services/remote/prevent_parallel_calls.ts:29:22)
     │   Error: r

Full error: https:/elastic/kibana/pull/101398/checks?check_run_id=2764396828

@sebelga sebelga requested review from dborodyansky and removed request for ryankeairns June 7, 2021 15:05
Copy link
Contributor

@dborodyansky dborodyansky left a comment

Choose a reason for hiding this comment

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

So awesome to see this coming to life, @sebelga! Awesome work. Some comments as follows:

Preview panel position
The position of the preview panel has been under discussion and potential trade offs have been raised. Upon further feedback from kibana-design team, I am suggesting we keep position per original design (on the right), add a transition, and assess again when implemented to get the actual feel for the trade offs. The primary rationale is expected hierarchy, and the natural flow of disclosing information from left to right is more intuitive and may warrant (or even be clarified by) shifting the content to afford expansion.

Close vs Cancel
Suggest removing the X button (hideCloseButton), renaming close to Cancel in the footer, and adding a confirmation dialog to the Cancel action if user has entered any fields. This would more clearly present the cause and effect of each action.

Panel width
The main panel section adjusting in width when the preview panel appears is not ideal. It would be better to keep the width or modify from onset if real estate is of concern.

Preview table row height
The table row height seems uncommon. Standard EUI table row height would be preferred.

Field highlighting
Suggest bolding the new preview field in addition to the background highlight to help with contrast accessibility.

Preview panel empty state
There seems to be a typo in the empty prompt. "..of our new fields will appear".

Bug?
Clearing name does not return preview error, as it seems it should.

image

Bug?
Calculation doesn't work for me. Please let me know if I am testing incorrectly.

image

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

You're adding a lot of UI here so we need to add some a11y tests to this too.
(Docs if you need them, and you can find lots of examples by going through this meta ticket)

@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 22, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@sebelga sebelga removed request for a team June 22, 2021 11:03
@sebelga sebelga removed Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 22, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@dborodyansky dborodyansky left a comment

Choose a reason for hiding this comment

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

Looks great @sebelga. Per our discussion, let's track the following enhancements in subsequent PR:

  • Panel transition
  • Error callout icon (alert instead of cross)

We will also investigate additional outstanding UX questions discussed in upcoming user validation sessions.

@sebelga
Copy link
Contributor Author

sebelga commented Jun 22, 2021

Thanks for the review @dborodyansky ! Yes let's continue the UX improvement in separate PRs. 👍

@sebelga sebelga merged commit bb321e6 into elastic:runtime-field-editor/preview-field-workstream Jun 22, 2021
@sebelga sebelga deleted the runtime-field-editor/preview-from-cluster-2 branch June 22, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project:RuntimeFields Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more
Projects
None yet
Development

Successfully merging this pull request may close these issues.