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] Persisted state and syncing, shared context, linking state to component + smaller changes #4652

Closed

Conversation

ashwin-pc
Copy link
Member

Description

This PR might look big but its actually just 3 changes and a few cleanup tasks that simplify the app structure.

Changes:

  1. Adds persistent storage using the URL for the following and sets them up to sync
    1. global state _g
    2. app state _a
    3. query state _q
  2. Adds a shared context in Data Explorer's view service to share context between the panel and canvas view components. Discover uses this to share the data fetching observable.
  3. Wired up the doc table, sidenav and the querybar to the application state.

Other minor changes include:

  1. renaming docViewExpand to expandedHit to make it clearer what it contains
  2. minor styling fixes for the sidebar
  3. Simplified the Doc Table structure by eliminating similar components
  4. Added back the DocViewLinks that was deleted in an older commit. (This is still WIP but added to keep the API intact)
  5. Adds the initial logic to handle routing changes from legacy discover to data explorer. Currently this only migrates older discover state to data explorer state and is limited to src/plugins/discover/public/migrate_state.ts and where ever its referenced.
  6. Disabling functional test for now until merge to main since they will anyways fail and will be reactivated before a merge to main.

Note: The commit history has changes that have already been merged but the diff is accurate.

Issues Resolved

closes #4225
closes #4227
closes #4228

Screenshot

Ignore the layout issues, this will be fixed later

Screen.Recording.2023-08-01.at.4.15.59.AM.mov

Testing the changes

To test the state syncing:

  1. for global state, modify the time range and refresh to see that the state persists. The data in the doc table should also update
  2. for app state, add colums and see the URL update. Again this should persist on refresh and the doc table should change
  3. For query state, update the search query and do the same as the above

To test the shared context, updating the columns should update the table and vice versa and only one network request should fire for an update in the netwprk tab to the opensearch route

Check List

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

ashwin-pc added 16 commits July 13, 2023 06:52
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
…uting

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
@ashwin-pc ashwin-pc added discover for discover reinvent de-angular de-angularize work data explorer Issues related to the Data Explorer project labels Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #4652 (c825ddb) into feature/data-explorer (521f306) will decrease coverage by 0.06%.
The diff coverage is 36.53%.

❗ Current head c825ddb differs from pull request most recent head 3376dae. Consider uploading reports for the commit 3376dae to get more accurate results

@@                    Coverage Diff                    @@
##           feature/data-explorer    #4652      +/-   ##
=========================================================
- Coverage                  66.50%   66.45%   -0.06%     
=========================================================
  Files                       3289     3291       +2     
  Lines                      62821    62854      +33     
  Branches                    9788     9792       +4     
=========================================================
- Hits                       41782    41769      -13     
- Misses                     18658    18707      +49     
+ Partials                    2381     2378       -3     
Flag Coverage Δ
Windows 66.45% <36.53%> (-0.06%) ⬇️

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

Files Changed Coverage Δ
.../data_explorer/public/components/app_container.tsx 40.00% <0.00%> (ø)
.../data_explorer/public/components/sidebar/index.tsx 3.44% <0.00%> (ø)
...tion/components/sidebar/discover_field_details.tsx 80.00% <ø> (ø)
...over/public/application/components/table/table.tsx 84.61% <ø> (ø)
src/plugins/discover/public/build_services.ts 0.00% <ø> (ø)
src/plugins/discover/public/migrate_state.ts 0.00% <0.00%> (ø)
src/plugins/discover/public/plugin.ts 0.00% <0.00%> (ø)
...ta_explorer/public/utils/state_management/store.ts 20.68% <20.00%> (+0.68%) ⬆️
...pplication/components/sidebar/lib/group_fields.tsx 84.61% <25.00%> (-10.84%) ⬇️
...pplication/components/sidebar/discover_sidebar.tsx 64.28% <33.33%> (-14.10%) ⬇️
... and 7 more

... and 3 files with indirect coverage changes

@ashwin-pc ashwin-pc changed the title Routing [Data Explorer] Persisted state and syncing, shared context, linking state to component + smaller changes Aug 1, 2023
on:
push:
branches: ['**', '!backport/**']
branches: ['**', '!backport/**', '!feature/**']
Copy link
Collaborator

Choose a reason for hiding this comment

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

When merging this into main, we should watch out not to bring this over.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep :)

@@ -39,36 +40,63 @@ export interface DiscoverRootState extends RootState {
discover: DiscoverState;
}

const initialState = {} as DiscoverState;
const initialState: DiscoverState = {
columns: [],
Copy link
Member

Choose a reason for hiding this comment

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

We should add _source to initial columns state

const initialState: DiscoverState = {
  columns: ['_source'],
  sort: [],
};

Otherwise we will see only time column when first render discover table
Screenshot 2023-08-03 at 11 05 48

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
@ashwin-pc
Copy link
Member Author

Duplicate of #4678

@ashwin-pc ashwin-pc marked this as a duplicate of #4678 Aug 4, 2023
@ashwin-pc ashwin-pc closed this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data explorer Issues related to the Data Explorer project de-angular de-angularize work discover for discover reinvent distinguished-contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants