search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.35k stars 7.98k forks source link

[ML] Refactors Field statistics table from an embeddable to a registered React component #181502

Closed qn895 closed 1 week ago

qn895 commented 3 weeks ago

Summary

Addresses https://github.com/elastic/kibana/issues/174965. This PR refactors Field statistics table from an embeddable to a async React component.

Context Initially when embeddable was implemented, we could not register dataVisualizer as an optional plugin due to the cyclical dependencies and enforced package import rules (from x-pack to discover). However, due to changes in the infrastructure in the last few years, we can now register x-pack plugins as a dependency. This eliminates the need for an embeddable as a wrapper around the field statistics table.

Reviewer's note

https://github.com/elastic/kibana/assets/43350163/7800291a-ad8c-447e-9bfc-e4f279b05d6a

https://github.com/elastic/kibana/assets/43350163/d7422e11-a0c7-4a8b-8f47-783e453f02a8

Follow up after this PR:

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

mgiota commented 3 weeks ago

@qn895 I am linking your PR with this one. For now, each embeddable that migrates to the new architecture, needs to add the data-shared-item attribute, so that Kibana reporting screenshot tool doesn't complain with timeout error.

qn895 commented 3 weeks ago

/ci

qn895 commented 3 weeks ago

/ci

qn895 commented 3 weeks ago

/ci

qn895 commented 3 weeks ago

/ci

elasticmachine commented 3 weeks ago

Pinging @elastic/ml-ui (:ml)

qn895 commented 2 weeks ago

/ci

qn895 commented 2 weeks ago

/ci

qn895 commented 2 weeks ago

/ci

qn895 commented 2 weeks ago

/ci

qn895 commented 2 weeks ago

Thanks @davismcphee and @peteharverson for testing. Per @ThomThomson's recommendation, I have migrated the field statistics table off the embeddable architecture and instead simply things by exposing it as a React component that Discover can consume. I've updated the PR description with the additional context to the change.

I've also fixed the issue with filters now. With the update, I think it works correctly now with the navigation via browser, but some additional testing would be appreciated. Also one thing to note is that for text fields (like agent field in Kibana sample data logs), it can only sample 1000 documents maximum. So if the dataset returns 1000+ hits, it will still only show 1000.

https://github.com/elastic/kibana/assets/43350163/c55d6a3f-1919-4b54-93ed-23d2181d3f34

https://github.com/elastic/kibana/assets/43350163/4f014826-ebbc-4a50-92bf-be7d0ad1de95

https://github.com/elastic/kibana/assets/43350163/da0bd647-0853-4a21-bf5d-4d2711580ac9

The saved search dashboard field statistics panel will also try to retain both the filters from the original saved search as well as the dashboard.

https://github.com/elastic/kibana/assets/43350163/eaf8ea58-302b-4235-adc1-a23d1483d182

peteharverson commented 2 weeks ago

I found this issue with filters and the browser history. If you add a filter, then edit it, hitting the 'back' button takes you to a state where you end up with the original and edited filter:

https://github.com/elastic/kibana/assets/7405507/9339da9e-0661-4235-91ac-82ad9ef9bc18

ThomThomson commented 2 weeks ago

I have migrated the field statistics table off the embeddable architecture and instead simply things by exposing it as a React component that Discover can consume.

This looks great! Thank you for doing this. One small nitpick is that some of the files / components still have embeddable in the name. If it isn't too much trouble I'd recommend removing that to avoid confusion.

Additionally, if you'd still like to place this on its own on a Dashboard in the future, feel free to reach out to @elastic/kibana-presentation - it should still be a straightforward process.

qn895 commented 2 weeks ago

Thanks Pete for catching that. I've fixed it here.

@ThomThomson Thanks for the guidance! I kept some of the naming/type as is cause we plan to add it as a dashboard panel in 8.15 so this will lessen some of the work we have to do then.

qn895 commented 2 weeks ago

@elasticmachine merge upstream

qn895 commented 1 week ago

@elasticmachine merge upstream

qn895 commented 1 week ago

@elasticmachine merge upstream

kibana-ci commented 1 week ago

:green_heart: Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataVisualizer 645 647 +2

Async chunks

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

id before after diff
dataVisualizer 656.1KB 671.8KB +15.7KB
discover 638.3KB 641.6KB +3.3KB
total +19.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
dataVisualizer 3 4 +1

Page load bundle

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

id before after diff
dataVisualizer 23.8KB 23.0KB -818.0B
discover 35.1KB 35.1KB +32.0B
total -786.0B
Unknown metric groups #### async chunk count | id | [before](https://github.com/elastic/kibana/commit/bc103c7016245901a04fc4921c1b213a4fbe2695) | [after](https://github.com/elastic/kibana/commit/3005634dd346e2699f59b5fe1170a2dac2da65cf) | diff | | --- | --- | --- | --- | | `dataVisualizer` | 8 | 12 | +4 | #### ESLint disabled line counts | id | [before](https://github.com/elastic/kibana/commit/bc103c7016245901a04fc4921c1b213a4fbe2695) | [after](https://github.com/elastic/kibana/commit/3005634dd346e2699f59b5fe1170a2dac2da65cf) | diff | | --- | --- | --- | --- | | `dataVisualizer` | 67 | 71 | +4 | | `discover` | 26 | 25 | -1 | | total | | | +3 | #### References to deprecated APIs | id | [before](https://github.com/elastic/kibana/commit/bc103c7016245901a04fc4921c1b213a4fbe2695) | [after](https://github.com/elastic/kibana/commit/3005634dd346e2699f59b5fe1170a2dac2da65cf) | diff | | --- | --- | --- | --- | | `dataVisualizer` | 10 | 8 | -2 | #### Total ESLint disabled count | id | [before](https://github.com/elastic/kibana/commit/bc103c7016245901a04fc4921c1b213a4fbe2695) | [after](https://github.com/elastic/kibana/commit/3005634dd346e2699f59b5fe1170a2dac2da65cf) | diff | | --- | --- | --- | --- | | `dataVisualizer` | 68 | 72 | +4 | | `discover` | 26 | 25 | -1 | | total | | | +3 |

History

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

cc @qn895

kibanamachine commented 1 week ago

💔 All backports failed

Status Branch Result
❌ 8.14 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 181502

Questions ?

Please refer to the Backport tool documentation