[ML] Add recognized modules links for Index data visualizer by qn895 · Pull Request #131342 · elastic/kibana

qn895 · 2022-05-02T18:04:18Z

Summary

Part of #86387. This PR re-adds recognized module links for the Index data visualizer. It also removes additionalLinks as a props for both Index data visualizer and File data visualizer. It's replaced async asyncLinkCards which is an array of promises that allows for us to pass in multiple links, and to be able to format the content dynamically.

This PR also moves the link to Discover up top.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

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

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2022-05-02T21:40:28Z

Pinging @elastic/ml-ui (:ml)

x-pack/plugins/ml/public/application/datavisualizer/file_based/file_datavisualizer.tsx

peteharverson · 2022-05-03T15:18:24Z

x-pack/plugins/ml/public/application/datavisualizer/file_based/file_datavisualizer.tsx

  const [FileDataVisualizer, setFileDataVisualizer] = useState<FileDataVisualizerSpec | null>(null);

-  const links: ResultLink[] = useMemo(
+  const asyncLinkCards = useMemo(


These show up when accessing the file view from within the ML plugin, but not when I upload a file from the Kibana Home page - Upload a file.

If I'm remembering correctly, we purposely didn't add any ML links to the file data vis when being used inside kibana's Add Data page.
It would be a lot more work to register the links on start up. We'd have to add a registry for the links to be stored in. Whereas currently the links are passed on component render.

Also most users will not be jumping from there to ML.
There are far more popular plugins in kibana that should come before ML in that list. but when using it inside ML, it makes sense to have links to other parts of ML

jgowdyelastic · 2022-05-04T08:56:24Z

x-pack/plugins/ml/public/application/datavisualizer/file_based/file_datavisualizer.tsx

+              return false;
+            }
+          },
+        } as ResultLink),


rather than asserting ResultLink here, the asyncLinkCards declaration could be asyncLinkCards: AsyncLinkCards

Updated here 846bd87

jgowdyelastic · 2022-05-04T09:06:11Z

x-pack/plugins/ml/public/application/datavisualizer/index_based/index_data_visualizer.tsx

-        dataTestSubj: 'dataVisualizerCreateAdvancedJobCard',
-      },
+        'data-test-subj': 'dataVisualizerCreateAdvancedJobCard',
+      } as ResultLink,


Rather than asserting as ResultLink, this function could have a return signature of Promise<ResultLink[]>

Updated here 846bd87

jgowdyelastic · 2022-05-04T09:08:00Z

...plugins/data_visualizer/public/application/common/components/results_links/results_links.tsx

+}
+
+export type AsyncLinkCards = Array<
+  (params: AsyncLinkCardParams) => Promise<ResultLink | ResultLink[] | undefined>


It would probably simplify the code if we only ever returned an array of ResultLink rather than having to deal with single links.

Updated here 846bd87

jgowdyelastic · 2022-05-04T09:08:32Z

...plugins/data_visualizer/public/application/common/components/results_links/results_links.tsx

+            );
+          }
+
+          if (results) {


as mentioned above, if we only ever dealt with ResultLink arrays, this logic could be removed

alternatively, if we do still need single result links, the single link could be put in an array and processed the same, e.g.

const tempResults = Array.isArray(results) ? results : [results]; return await Promise.all(results.map ....

Updated here 846bd87

jgowdyelastic · 2022-05-04T09:14:26Z

...plugins/data_visualizer/public/application/common/components/results_links/results_links.tsx

+  globalState?: any;
+}
+
+export type AsyncLinkCards = Array<


I'm not sure about the name AsyncLinkCards
It implies that there are also non-async links cards.
The functions that return the ResultsLinks are all async, so adding the word async to the name seems unnecessary.
Maybe something like GetResultLinks would be more accurate? Although i'm not crazy about that name.

Updated here 966ff9f

jgowdyelastic · 2022-05-04T09:16:20Z

...plugins/data_visualizer/public/application/common/components/results_links/results_links.tsx

  createDataView: boolean;
  showFilebeatFlyout(): void;
-  additionalLinks: ResultLink[];
+  asyncLinkCards?: AsyncLinkCards;


IMO the original additionalLinks name is better here.
Possibly changed to getAdditionalLinks?

Updated here 966ff9f

jgowdyelastic

LGTM

peteharverson

Tested latest changes and LGTM

qn895 · 2022-05-04T17:10:15Z

@elasticmachine merge upstream

kibana-ci · 2022-05-04T18:21:40Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dataVisualizer`	347	348	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`dataVisualizer`	19	24	+5

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`dataVisualizer`	2	3	+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`	539.6KB	540.0KB	+377.0B
`ml`	3.3MB	3.3MB	+458.0B
total			+835.0B

Page load bundle

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

id	before	after	diff
`ml`	39.3KB	39.5KB	+114.0B

Unknown metric groups

API count

id	before	after	diff
`dataVisualizer`	23	28	+5

History

💚 Build #42527 succeeded 966ff9f
💚 Build #42172 succeeded bcc57c2
💚 Build #41893 succeeded b3556e9
💔 Build #41871 failed 6db8c35

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

cc @qn895

…131342) * [ML] Add dynamic registration of links for both index and file * [ML] Consolidate type imports * [ML] Revert uptime changes * [ML] Fix cards visible when canDisplay is false * [ML] Shorten create job text * [ML] Remove as assertions * [ML] Rename to GetAdditionalLinks Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[ML] Add dynamic registration of links for both index and file

6db8c35

qn895 mentioned this pull request May 2, 2022

[Meta][ML] Data Visualizer redesign & plugins integration #86387

Closed

35 tasks

qn895 added 2 commits May 2, 2022 14:09

[ML] Consolidate type imports

f6f2f8d

[ML] Revert uptime changes

b3556e9

qn895 self-assigned this May 2, 2022

qn895 added v8.3.0 release_note:enhancement :ml labels May 2, 2022

qn895 marked this pull request as ready for review May 2, 2022 21:40

qn895 requested a review from a team as a code owner May 2, 2022 21:40

peteharverson reviewed May 3, 2022

View reviewed changes

x-pack/plugins/ml/public/application/datavisualizer/file_based/file_datavisualizer.tsx Outdated Show resolved Hide resolved

peteharverson reviewed May 3, 2022

View reviewed changes

qn895 requested a review from jgowdyelastic May 3, 2022 15:41

qn895 added 2 commits May 3, 2022 12:05

[ML] Fix cards visible when canDisplay is false

040b42f

[ML] Shorten create job text

bcc57c2

jgowdyelastic reviewed May 4, 2022

View reviewed changes

qn895 added 2 commits May 4, 2022 10:43

[ML] Remove as assertions

846bd87

[ML] Rename to GetAdditionalLinks

966ff9f

jgowdyelastic approved these changes May 4, 2022

View reviewed changes

peteharverson approved these changes May 4, 2022

View reviewed changes

qn895 added the Feature:File and Index Data Viz ML file and index data visualizer label May 4, 2022

Merge branch 'main' into ml-dv-register-recognized-modules

ea7c31c

qn895 enabled auto-merge (squash) May 4, 2022 17:10

qn895 merged commit a632484 into elastic:main May 4, 2022

kibanamachine added the backport:skip This PR does not require backporting label May 4, 2022

qn895 deleted the ml-dv-register-recognized-modules branch June 8, 2022 20:09

qn895 mentioned this pull request Jun 21, 2023

[ML] Fixes time range in link to data visualizer after file upload #160189

Merged

Conversation

qn895 commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Risk Matrix

For maintainers

Uh oh!

elasticmachine commented May 2, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

qn895 commented May 4, 2022

Uh oh!

kibana-ci commented May 4, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Any counts in public APIs

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

qn895 commented May 2, 2022 •

edited

Loading

jgowdyelastic May 4, 2022 •

edited

Loading