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

[8.x] [Lens] Embeddable react refactor (#186642) #201739

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Nov 26, 2024

Backport

This will backport the following commits from main to 8.x:

I've skipped a flaky test here to carry on with the merge.
The same test is flaky in main too: #201744 .
Will push a test fix asap.

Questions ?

Please refer to the Backport tool documentation

## Summary

This PR contains the refactor of the Lens embeddable with the new React
architecture.

fix elastic#174957
fixes elastic#180672

**Current status**:
✅ Ready to review

### Notes for testing and reviewers

Other than reworking the Lens embeddable with the new architecture this
PR contains the following major changes.

#### Edit flow
The `Edit` flow has changed to in-line first using the new `Edit` API
provided by the new system
* The impact of this change can be noticed in the code on the `Canvas`
case where the Custom Lens component is instructed to avoid the inline
editing. In all the other cases in-line editing is enabled by default
now.
* Another side effect of this has been the replacement of the special
`INLINE_EDIT` action id into the regular `EDIT` action. Some tests have
been affected by this replacing the `clickEdit` function with the
`openEditorFromFlyout` one.
* The Inline editing codebase **as been reworked entirely** so make sure
to stress test this side of things.

#### Attribute service

Another important aspect changed in this PR is the `attributeService`:
this was tied to the previous Embeddable system and it is now completely
skipped. The Lens wrapper around that has been reworked to be thinner
and directly call the CM services.
* Please make sure to test thoroughly save/load SO flows

#### Transformation API (by-value <=> by-reference flow)

The new system adopts the new Transformation API (who prevents the panel
to fully reload on change).
* Please make sure to test thoroughly Visualize library <=> by value
flows
* In particular moving from one type and another should change how the
Panel Settings interpret "default" values to reset

#### Message system

Also this part of the code was partially rewritten to be more manageable
ont he embeddable surface, maintaining the core functionalities.
* Please make sure to test thoroughly error messages, warnings and info
messages
  * Some scenarios to test includes
* multi-layer errors (i.e. use a broken KQL query for an
annotation/multi-layers). Check that the panel recovers correctly from
it when resolved
    * Missing references
    * Missing dataViews
    * Wrong formatted SO
* Configuration mistakes - check that a broken config is not saveable

### Other areas to check

* Change filters in dashboard/viz and check that are correctly handled
* Check drilldowns
* Check that `Unsaved changes` are correctly detected
* Check that the panel updates correctly on `View` mode change

## Main type changes

This PR contains also some important `type` changes, here's listed:
* the `query` property now explicitly supports ES|QL query type.
  * in `main` it used to work without type support
* `LensEmbeddableInput`/`LensEmbeddableOutput` types have changed, but
the type names remained the same.

## Follow ups already planned:

Some enhancements have been already collected and will be addressed in a
follow up [here](elastic#195355)

### Tasks
<details>

<summary>Detailed list of tasks for the refactor</summary>

* New embeddable factory
  * [x] Define visualization context
  * [x] Define observables to track
  * [x] Basic panel settings
  * [x] Basic edit api
  * [x] inspector api
  * [x] Library services
  * [x] Unified search api
  * [x] Basic integrations api
  * [x] State management api for inline editing
  * Publish correct observables
    * [x] `dataViews`
    * [x] `query`
    * [x] `filters`
    * [x] `dataLoading`
    * [x] `savedObjectId`
  * Actions
    * [x] View underlying data api
 * Custom renderer
   * [x] Basic implementation
   * [x] Support callbacks
   * [x] Support custom styling/paddings
 * Expose
* [x] Handle searchSession
* Edit
  * [x] Open panel in Lens editor
  * Inline editing
    * [x] rework references logic
      * elastic#180726
* integrate the logic to extract filters dataViews from filters as for
the first bug in elastic#188545
    * DSL flyout
      * [x] open flyout
      * [x] save
    * ES|QL
      * [x] open flyout on creation
      * [x] open flyout on editing
      * [x] save
* [x] revisit mounting logic to avoid detach if possible (not possible
yet)
* [x] explore the integration with the new `onEdit` api method used for
the inline editing~~
      * [x] created panel management module and sorted it out
    * [x] open in Editor
      * [x] fix the save on return to dashboard
* ~~migrate by ref to by value on inline editing~~ will do it in a
follow up PR
* Add from library issues
  * [x] Fix missing title and tags
* Data loading
  * [x] Compute all required data params for rendering
* Render the panel
  * [x] hook up user messaging system
  * [x] Merge search context
  * [x] Expression variables
  * [x] panel settings
    * [x] per panel time range
    * [x] per panel filter
    * test with both DSL and ES|QL mode
  * Reload
    * [x] on unified search updates
    * [x] on config changes
    * [x] on drilldown changes?
    * [x] on view mode change
 * Attributes service
   * [x] load from library
   * [x] save to library

</details>

### Pending issues:
<details>

<summary>Detailed list of issues</summary>

* [x] Unified histogram does not render in Discover
* [x] Saving to library from context menu in dashboard doesn't save the
title
* [x] When adding a vis from the library the new panel has no title
* [x] Vis disappears when opening inline editor and cancel
  * Create a viz, save and return to dashboard, then edit it and cancel.
* Saving an edit inline doesn't apply the changes (i.e. changing the
chart type)
  * [x] Changing the chart type on the layer panel leads to a crash
* [x] Changing the chart type won't update the visualization (via both
config panel or suggestions)
* [x] Edit a dimension will stretch the panel to overflow the fly-out
* [x] duplicating a dimension in the inline editor by drag and drop
works buggy visually
* When duplicating a panel, the new panel gets the same title rather
than “title (copy)”
  * [x] by-value panels
  * [x] by-reference panels
* [x] brushing throughout the timerange doesn’t work
* [x] filtering when clicking on value doesn’t work
* [x] filtering from legend doesn’t work
* [x] for lens table, the sort ascending/descending actions don’t have
an effect
* [x] filtering doesn’t display on table either
* Discover related issues
* thanks to @davismcphee investigation the source of the issue seems to
be related to the way the `abortController` is managed in the new
embeddable implementation as Discover is relying on that.
* [x] needs to investigate for a fix that restores the previous
behaviour of the `abortController` management
  * [x] the hits total count is not in sync with the chart/table now
* [x] Change chart type via suggestion panel when inline editing in
Discover doesn't update the chart
* [x] Dirty panel issue (see @nickofthyme 's
[comment](elastic#186642 (comment))
)
* [x] `Unsaved changes` issue (see @mbondyra
[comment](elastic#186642 (comment)))
* [x] Multiple errors not rendered correctly in panel when blocking
(i.e. missing field - `lens-message-list-trigger` related)
  * [x] recover from a blocker error required 2 renders
* Missing SO error should not be handled for the custom render component
(legacy behaviour) but should be correctly handled for dashboard (will
be handled in a follow up PR given that is broken on `main` too)
* [x] Too many requests on Unified Histogram when in Discover (3 vs 2)
* [x] Too many request on slow queries for Unified Histogram (2 vs 1)
* [x] Annotations preview issues (chart rendering with height `0px`)
* [x] `uuid` not propagated correctly
* [x] another flavour of this was `id` not propagated correctly into the
`data-test-embeddable-id` attribute
* [x] Dispatch correctly the `render` events
* [x] refresh interval does not propagate thru the Lens custom component
in Discover (thanks to @jughosta to sort this out )
</details>

---------

Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Marco Vettorello <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Bhavya RM <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
(cherry picked from commit 61d0320)

# Conflicts:
#	x-pack/plugins/observability_solution/exploratory_view/public/components/shared/exploratory_view/configurations/lens_attributes.ts
@dej611 dej611 enabled auto-merge (squash) November 26, 2024 10:05
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:Obs AI Assistant Observability AI Assistant Team:obs-ux-management Observability Management User Experience Team labels Nov 26, 2024
@dej611 dej611 mentioned this pull request Nov 26, 2024
74 tasks
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ai-assistant (Team:Obs AI Assistant)

Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@kibanamachine
Copy link
Contributor

kibanamachine commented Nov 27, 2024

This runner was configured with the test still unskipped. Since the test demonstrated to be flaky it has been skipped in this PR.

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7504

[❌] test/functional/apps/management/config.ts: 20/50 tests passed.

see run history

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 27, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: f2e0175
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-201739-f2e01755e41f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1492 1518 +26

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
lens 591 548 -43

Async chunks

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

id before after diff
canvas 1.1MB 1.1MB +17.0B
cases 492.5KB 492.7KB +167.0B
eventAnnotationListing 229.5KB 229.5KB -23.0B
exploratoryView 182.8KB 182.8KB +13.0B
lens 1.5MB 1.5MB +19.2KB
ml 4.7MB 4.7MB +354.0B
observabilityAIAssistantApp 239.2KB 239.2KB -5.0B
securitySolution 13.3MB 13.3MB +924.0B
unifiedHistogram 71.3KB 71.1KB -228.0B
total +20.3KB

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
lens 62 60 -2

Page load bundle

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

id before after diff
lens 50.8KB 49.9KB -933.0B
unifiedHistogram 10.3KB 10.3KB +26.0B
total -907.0B
Unknown metric groups

API count

id before after diff
lens 693 647 -46

async chunk count

id before after diff
lens 23 26 +3

ESLint disabled line counts

id before after diff
lens 17 21 +4

References to deprecated APIs

id before after diff
lens 84 85 +1

Total ESLint disabled count

id before after diff
lens 17 21 +4

Unreferenced deprecated APIs

id before after diff
embeddable 0 2 +2

History

Copy link
Contributor

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

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

Not my domain, but trying a blind approve to see if we can unblock it

@dej611 dej611 merged commit 556deb0 into elastic:8.x Nov 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:Obs AI Assistant Observability AI Assistant Team:obs-ux-management Observability Management User Experience Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants