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

[Data Explorer] Migrate VisBuilder to Data Explorer #5627

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Dec 18, 2023

Description

Add initial setup and migrate state management

This PR completes Task 1 and 2 in #5407.

  • Reconstruct and allow VisBuilder to be rendered from DataExplorer
  • Follow proposal task 2 option 1 to migrate state management to DataExplorer

Issue Resolve
#5492 #5493

Add context and implement side panel

  • add useVisBuilderContext
  • modify preloadedState in Data Explorer
  • implement side panel

Issue Resolve:
#5522

Combine components into VisBuilderCanvas

Issue Resolve:
#5523

Recording

[Recording 1]

  • start and run vis-builder
  • drag & drop
  • switch vis types
  • time range
  • save vis-builder
  • re-load vis-builder
record-1.mov

[Recording 2]

  • Line graph. Compared with playground.
Line.graph.mov

[Recording 3]

  • Legacy Urls work in Vis-Builder
http://localhost:5601/[app/vis-builder/edit/00d51500-a444-11ee-b295-337d2ac786b3#?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-18w,to:now))&_q=(filters:!(('$state':(store:appState),meta:(alias:!n,disabled:!f,index:ff959d40-b880-11e8-a6d9-e546fe2bba5f,key:category.keyword,negate:!f,params:(query:'Women!'s%20Clothing'),type:phrase),query:(match_phrase:(category.keyword:'Women!'s%20Clothing')))),query:(language:kuery,query:%22Friday%22))&_a=(metadata:(editor:(errors:(),state:clean)),style:(addLegend:!t,addTooltip:!t,legendPosition:right,type:histogram),ui:(),visualization:(activeVisualization:(aggConfigParams:!((enabled:!t,id:'1',params:(field:geoip.city_name),schema:metric,type:cardinality),(enabled:!t,id:'2',params:(field:day_of_week,missingBucket:!f,missingBucketLabel:Missing,order:desc,otherBucket:!f,otherBucketLabel:Other,size:5),schema:segment,type:terms)),name:histogram),indexPattern:ff959d40-b880-11e8-a6d9-e546fe2bba5f,searchField:''))
migrate_state.mov

[Recording 4]

  • Switch index pattern
switch-idx.mov

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: Patch coverage is 15.10791% with 236 lines in your changes are missing coverage. Please review.

Project coverage is 67.60%. Comparing base (85df662) to head (0c9e9f5).
Report is 9 commits behind head on main.

❗ Current head 0c9e9f5 differs from pull request most recent head eaad556. Consider uploading reports for the commit eaad556 to get more accurate results

Files Patch % Lines
src/plugins/vis_builder/public/plugin.ts 3.22% 30 Missing ⚠️
src/plugins/vis_builder/public/migrate_state.ts 0.00% 27 Missing ⚠️
.../public/application/utils/use/use_index_pattern.ts 4.54% 21 Missing ⚠️
...ta_explorer/public/utils/state_management/store.ts 6.25% 15 Missing ⚠️
..._explorer/public/utils/state_management/preload.ts 0.00% 14 Missing ⚠️
.../application/utils/helpers/index_pattern_helper.ts 0.00% 12 Missing ⚠️
...pplication/utils/state_management/prefix_helper.ts 30.76% 9 Missing ⚠️
...on/utils/state_management/handlers/editor_state.ts 11.11% 8 Missing ⚠️
...ion/view_components/utils/use_vis_builder_state.ts 11.11% 8 Missing ⚠️
src/plugins/vis_builder/public/extract_id.ts 0.00% 8 Missing ⚠️
... and 27 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5627       +/-   ##
===========================================
+ Coverage   55.58%   67.60%   +12.01%     
===========================================
  Files        1199     3357     +2158     
  Lines       24259    65235    +40976     
  Branches     4087    10520     +6433     
===========================================
+ Hits        13485    44100    +30615     
- Misses      10133    18565     +8432     
- Partials      641     2570     +1929     
Flag Coverage Δ
Linux_1 32.03% <13.66%> (?)
Linux_2 55.58% <100.00%> (?)
Linux_3 44.87% <10.25%> (?)
Linux_4 35.07% <0.00%> (?)
Windows_1 32.06% <13.66%> (?)
Windows_2 55.55% <100.00%> (-0.04%) ⬇️
Windows_3 44.89% <10.25%> (?)
Windows_4 35.07% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ananzh
Copy link
Member Author

ananzh commented Dec 21, 2023

Ongoing and Upcoming Tasks

Minor Bug Identification: This morning, I noticed a small issue. After saving a visualization in Vis Builder and then reloading it from the Recently Viewed section in the side navigation, the breadcrumb doesn’t display the title correctly. I believe moving chrome.setBreadcrumb from src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts to top_nav should resolve this.

Regarding Comment 1: About the suggestion to prefix the slice with the view ID to avoid name conflicts, I’m considering a modification. In the Data Explorer, we could add a prefix, but when retrieving the state, it would be necessary to remove this prefix for use. This means that each time we use a state in Vis Builder, we would run a helper function to remove the prefix. I'm still contemplating this approach.

Regarding Comment 2: I just remember we need to add migration state to make sure legacy URLs for Vis Builder continue working after this migration. I will implement a solution similar to what was used during the Discover migration.

Performance Improvement: I also noticed some potential improvements in reducing repeated rendering.

Next Steps

  • Fix the Breadcrumb Bug: Address the minor bug with the breadcrumb not displaying correctly.
  • Legacy URL Migration: Implement the migration state for legacy URLs.
  • Prefix Solution: Explore better solutions for handling the prefix issue.
  • Functional Tests: Fix functional testing.

@ananzh ananzh force-pushed the vb-migration-de branch 4 times, most recently from 57b6ae9 to aaa6a7e Compare December 27, 2023 23:13
@ananzh ananzh force-pushed the vb-migration-de branch 3 times, most recently from 0bf4045 to 327678b Compare April 8, 2024 20:30
@ananzh ananzh marked this pull request as ready for review April 8, 2024 20:35
ananzh added 9 commits April 12, 2024 01:55
This PR completes Task 1 and 2 in opensearch-project#5407.

* Reconstruct and allow VisBuilder to be rendered from DataExplorer
* Follow proposal task 2 option 1 to migrate state management to DataExplorer

Issue Resolve
opensearch-project#5492
opensearch-project#5493

[2][VisBuilder Migration] Add context and implement side panel

* add useVisBuilderContext
* modify preloadedState in Data Explorer
* implement side panel

Issue Resolve:
opensearch-project#5522

Signed-off-by: ananzh <ananzh@amazon.com>

[3][VisBuilder Migration] Combine components into VisBuilderCanvas

Signed-off-by: ananzh <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
This PR avoids re-rendering the entire AppContainer when data services update.
The re-render is caused by history object which is in AppMountParameters. This
history object is part of the application's routing context and can change frequently
as the url is updated by query, filter or time range. Each change in the history object
could potentially trigger a re-render of the AppContainer and its child components.

With this PR, the AppContainer will not be re-loaded. Instead each component, like Canvas
and Panel, will be updated.

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh marked this pull request as draft October 30, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants