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

[ML] Data Visualizer: Remove duplicated geo examples, support 'version' type, add filters for boolean fields, and add sticky header to Discover #136236

Merged

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Jul 12, 2022

Summary

This PR is part of #86387. It adds several enhancements:

  1. Version field types are no longer treated as a keyword type as before. The new behavior/icon matches with Discover's.

Screen Shot 2022-07-12 at 16 12 16

  1. Boolean fields now have filters:

Screen Shot 2022-07-12 at 16 12 30

  1. Geo fields no longer show duplicated results.

  2. Field stats in Discover now have a sticky header when table is scrolled (see issue [Discover] Pin header row in Field Statistics when scrolling #136299):

Screen.Recording.2022-07-13.at.16.11.50.mov
  1. Finally, this PR also changes so that the ML Index data visualizer's data view header uses the nice display name instead of by the index pattern.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@qn895 qn895 self-assigned this Jul 12, 2022
@qn895 qn895 force-pushed the ml-dv-duplicated-geo-bool-filter-version-icon branch from 8629402 to e923e33 Compare July 12, 2022 21:13
@qn895 qn895 force-pushed the ml-dv-duplicated-geo-bool-filter-version-icon branch from e923e33 to 4b64a60 Compare July 12, 2022 21:30
@qn895 qn895 marked this pull request as ready for review July 13, 2022 15:17
@qn895 qn895 requested a review from a team as a code owner July 13, 2022 15:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895 qn895 force-pushed the ml-dv-duplicated-geo-bool-filter-version-icon branch from 9e8b3c5 to 3dc8912 Compare July 13, 2022 20:16
@qn895 qn895 requested a review from a team as a code owner July 13, 2022 21:25
@qn895 qn895 changed the title [ML] Data Visualizer: Remove duplicated geo examples, support 'version' type, and add filters for boolean fields [ML] Data Visualizer: Remove duplicated geo examples, support 'version' type, add filters for boolean fields, and add sticky header to Discover Jul 13, 2022
@@ -40,6 +40,7 @@ export const JOB_FIELD_TYPES = {
TEXT: 'text',
HISTOGRAM: 'histogram',
UNKNOWN: 'unknown',
VERSION: 'version',
Copy link
Member

Choose a reason for hiding this comment

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

nit, UNKNOWN should be the last item in this list

Copy link
Member

Choose a reason for hiding this comment

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

not for this PR, but JOB_FIELD_TYPES should probably be replaced with ES_FIELD_TYPES

Copy link
Member Author

@qn895 qn895 Jul 14, 2022

Choose a reason for hiding this comment

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

Updated here cc5f0fd

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest changes and LGTM.
Love the sticky table header in Discover!

@qn895 qn895 force-pushed the ml-dv-duplicated-geo-bool-filter-version-icon branch from 1ab83b9 to cc5f0fd Compare July 14, 2022 16:29
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.

Sass changes LGTM.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataVisualizer 364 365 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataVisualizer 544.5KB 545.2KB +683.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dataVisualizer 16.6KB 16.6KB +18.0B

History

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

cc @qn895

@qn895 qn895 merged commit 2a6f8e8 into elastic:main Jul 14, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:File and Index Data Viz ML file and index data visualizer :ml release_note:enhancement v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants