[discover-next][bug] add max height to dataset navigator and use memoization #7540

kavilla · 2024-07-27T03:37:12Z

Description

For a lot of indices I give it a max height on the popover. Also using memoization helps a lot with the performance and rerendering.

Issues Resolved

n/a

Screenshot

Testing the changes

locally

Changelog

fix: dataset explorer performance and handling overflow

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

codecov · 2024-07-27T03:52:19Z

Codecov Report

Attention: Patch coverage is 4.68750% with 122 lines in your changes missing coverage. Please review.

Project coverage is 63.64%. Comparing base (2d8c743) to head (335978d).
Report is 245 commits behind head on main.

Files with missing lines	Patch %	Lines
.../public/ui/dataset_navigator/dataset_navigator.tsx	0.97%	102 Missing ⚠️
.../public/query/state_sync/connect_to_query_state.ts	16.66%	8 Missing and 2 partials ⚠️
...ta/public/query/dataset_manager/dataset_manager.ts	12.50%	6 Missing and 1 partial ⚠️
...ata/public/antlr/opensearch_sql/code_completion.ts	0.00%	1 Missing ⚠️
...query/state_sync/create_global_query_observable.ts	0.00%	1 Missing ⚠️
...ublic/query_assist/components/query_assist_bar.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7540      +/-   ##
==========================================
- Coverage   63.65%   63.64%   -0.01%     
==========================================
  Files        3629     3629              
  Lines       79519    79520       +1     
  Branches    12602    12601       -1     
==========================================
- Hits        50618    50613       -5     
- Misses      25834    25840       +6     
  Partials     3067     3067

Flag	Coverage Δ
Linux_1	`30.76% <0.80%> (-0.01%)`	⬇️
Linux_2	`55.58% <ø> (ø)`
Linux_3	`40.23% <3.20%> (-0.01%)`	⬇️
Linux_4	`31.57% <2.34%> (-0.01%)`	⬇️
Windows_1	`30.77% <0.80%> (-0.01%)`	⬇️
Windows_2	`55.53% <ø> (ø)`
Windows_3	`40.23% <3.20%> (-0.02%)`	⬇️
Windows_4	`31.57% <2.34%> (-0.01%)`	⬇️

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.

sejli

Changes look good and much needed 🙌 . Test failures look unrelated though.

     │1)    doc views
     │       custom doc views with datagrid table
     │         should render react doc view:
     │
     │      Error: expected 'logstash-2015.09.20' to equal 'logstash-2015.09.22'
     │       at Assertion.assert (packages\osd-expect\expect.js:111:11)
     │       at Assertion.be.Assertion.equal (packages\osd-expect\expect.js:238:8)
     │       at Assertion.be (packages\osd-expect\expect.js:80:22)
     │       at Context.<anonymous> (test\plugin_functional\test_suites\doc_views\/doc_views.ts:58:54)
     │       at processTicksAndRejections (node:internal/process/task_queues:95:5)
     │       at Object.apply (packages\osd-test\src\functional_test_runner\lib\mocha\wrap_function.js:95:16)

Signed-off-by: Kawika Avilla <kavilla414@gmail.com> almost working pretty nicely Signed-off-by: Kawika Avilla <kavilla414@gmail.com> a little bit better Signed-off-by: Kawika Avilla <kavilla414@gmail.com> its ok Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

ashwin-pc

Has a few fast follows that are needed, but LGTM

ashwin-pc · 2024-07-29T22:24:09Z

src/plugins/data/public/query/dataset_manager/dataset_manager.ts

-    return this.defaultDataSet;
+  };
+
+  public initWithIndexPattern = (indexPattern: IndexPattern | null) => {


This can just be setDataset

ashwin-pc · 2024-07-29T22:25:21Z

src/plugins/data/public/ui/dataset_navigator/_dataset_navigator.scss

+  &:not(:hover)::-webkit-scrollbar {
+    width: 0;
+    background: transparent;
+  }
+
+  &::-webkit-scrollbar {
+    width: 4px;
+  }
+
+  &::-webkit-scrollbar-track {
+    background: $euiColorLightestShade;
+  }
+
+  &::-webkit-scrollbar-thumb {
+    background-color: $euiColorMediumShade;
+    border-radius: 4px;
+  }


We should just use the EUI Scrollbar mixin here

ashwin-pc · 2024-07-29T22:25:44Z

src/plugins/data/public/ui/dataset_navigator/_dataset_navigator.scss

+  }
+
+  &__menu {
+    width: 365px;


Do we want a fixed width?

ashwin-pc · 2024-07-29T22:28:52Z

src/plugins/query_enhancements/public/plugin.tsx

+    core.uiSettings.getUpdate$().subscribe(({ key, newValue }) => {
+      if (key === UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED) {
+        if (newValue) {
+          core.uiSettings.set(UI_SETTINGS.STATE_STORE_IN_SESSION_STORAGE, true);
+        }
+      }
+      if (key === UI_SETTINGS.STATE_STORE_IN_SESSION_STORAGE) {
+        if (!newValue) {
+          core.uiSettings.set(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED, false);
+        }
+      }


We should get rid of this limitation

opensearch-trigger-bot · 2024-07-29T22:57:47Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7540-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 611f56bc63b56546437b028ae43f76719118aed8
# Push it to GitHub
git push --set-upstream origin backport/backport-7540-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7540-to-2.x.

opensearch-trigger-bot · 2024-07-29T22:57:48Z

The backport to 2.16 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.16 2.16
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.16
# Create a new branch
git switch --create backport/backport-7540-to-2.16
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 611f56bc63b56546437b028ae43f76719118aed8
# Push it to GitHub
git push --set-upstream origin backport/backport-7540-to-2.16
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.16

Then, create a pull request where the base branch is 2.16 and the compare/head branch is backport/backport-7540-to-2.16.

After the following PRs: #7492, #7546, #7540 this commit added skip(1) back to dataset manager observable: fef6156, we need to revert changes done in fix(query assist): update reading data source id from dataset manager #7464 (comment) revert dataset manager observable usage in query assist to support skip(1) revert dataset manager tests [Discover Next] Fixes Discover styles #7546 removed query editor header div, this PR adds it back to enable query editor extensions Signed-off-by: Joshua Li <joshuali925@gmail.com>

…ization (opensearch-project#7540) * add max heigh. use memoization Signed-off-by: Kawika Avilla <kavilla414@gmail.com> almost working pretty nicely Signed-off-by: Kawika Avilla <kavilla414@gmail.com> a little bit better Signed-off-by: Kawika Avilla <kavilla414@gmail.com> its ok Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update mock Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update another mock Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix mock for extension Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * rebase fixes Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update script Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix initial load Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

After the following PRs: opensearch-project#7492, opensearch-project#7546, opensearch-project#7540 this commit added skip(1) back to dataset manager observable: fef6156, we need to revert changes done in fix(query assist): update reading data source id from dataset manager opensearch-project#7464 (comment) revert dataset manager observable usage in query assist to support skip(1) revert dataset manager tests [Discover Next] Fixes Discover styles opensearch-project#7546 removed query editor header div, this PR adds it back to enable query editor extensions Signed-off-by: Joshua Li <joshuali925@gmail.com>

* [Discover 2.0] Updating fetch functions to include local cluster (#7542) * Update datasources fetch function to include local cluster * Check for duplicates when fetching external datasources (in the case local cluster is added as a datasource) * Clean up types in DataSetNavigator so items are displayed properly --------- Signed-off-by: Sean Li <lnse@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> * [discover-next][bug] add max height to dataset navigator and use memoization (#7540) * add max heigh. use memoization Signed-off-by: Kawika Avilla <kavilla414@gmail.com> almost working pretty nicely Signed-off-by: Kawika Avilla <kavilla414@gmail.com> a little bit better Signed-off-by: Kawika Avilla <kavilla414@gmail.com> its ok Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update mock Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update another mock Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix mock for extension Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * rebase fixes Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update script Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix initial load Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Fix query assist for query editor (#7552) After the following PRs: #7492, #7546, #7540 this commit added skip(1) back to dataset manager observable: fef6156, we need to revert changes done in fix(query assist): update reading data source id from dataset manager #7464 (comment) revert dataset manager observable usage in query assist to support skip(1) revert dataset manager tests [Discover Next] Fixes Discover styles #7546 removed query editor header div, this PR adds it back to enable query editor extensions Signed-off-by: Joshua Li <joshuali925@gmail.com> * [Discover next] Fixes dataset navigator menu styling & search error toast (#7566) Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * [Discover 2.0] Loading fix for databases (#7567) * add back in useeffect for loading databases Signed-off-by: Sean Li <lnse@amazon.com> * Changeset file for PR #7567 created/updated --------- Signed-off-by: Sean Li <lnse@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> --------- Signed-off-by: Sean Li <lnse@amazon.com> Signed-off-by: Kawika Avilla <kavilla414@gmail.com> Signed-off-by: Joshua Li <joshuali925@gmail.com> Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Sean Li <lnse@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Kawika Avilla <kavilla414@gmail.com> Co-authored-by: Joshua Li <joshuali925@gmail.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com>

…ization (opensearch-project#7540) * add max heigh. use memoization Signed-off-by: Kawika Avilla <kavilla414@gmail.com> almost working pretty nicely Signed-off-by: Kawika Avilla <kavilla414@gmail.com> a little bit better Signed-off-by: Kawika Avilla <kavilla414@gmail.com> its ok Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update mock Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update another mock Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix mock for extension Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * rebase fixes Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update script Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix initial load Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

After the following PRs: opensearch-project#7492, opensearch-project#7546, opensearch-project#7540 this commit added skip(1) back to dataset manager observable: fef6156, we need to revert changes done in fix(query assist): update reading data source id from dataset manager opensearch-project#7464 (comment) revert dataset manager observable usage in query assist to support skip(1) revert dataset manager tests [Discover Next] Fixes Discover styles opensearch-project#7546 removed query editor header div, this PR adds it back to enable query editor extensions Signed-off-by: Joshua Li <joshuali925@gmail.com>

AMoo-Miki · 2024-08-22T04:37:47Z

Backported with #7574

kavilla requested review from ananzh, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi, mengweieric, LDrago27, virajsanghvi and sejli as code owners July 27, 2024 03:37

github-actions bot added the distinguished-contributor label Jul 27, 2024

kavilla added backport 2.x v2.16.0 discover-next backport 2.16 and removed distinguished-contributor labels Jul 27, 2024

opensearch-changeset-bot bot added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jul 27, 2024

Changeset file for PR opensearch-project#7540 created/updated

7e55fcc

sejli previously approved these changes Jul 27, 2024

View reviewed changes

kavilla dismissed stale reviews from ashwin-pc and sejli via 9d00607 July 28, 2024 09:34

kavilla force-pushed the kavilla/improvementsdenav branch 2 times, most recently from 9d00607 to a122934 Compare July 29, 2024 02:44

ashwin-pc previously approved these changes Jul 29, 2024

View reviewed changes

kavilla added 5 commits July 29, 2024 17:48

update mock

f230a23

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

update another mock

b5e628f

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

fix mock for extension

4f72440

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

rebase fixes

aadbe4d

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

kavilla dismissed ashwin-pc’s stale review via aadbe4d July 29, 2024 20:50

kavilla force-pushed the kavilla/improvementsdenav branch from 1035ae2 to aadbe4d Compare July 29, 2024 20:50

kavilla added 2 commits July 29, 2024 20:52

update script

e47af9c

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

fix initial load

335978d

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

ashwin-pc approved these changes Jul 29, 2024

View reviewed changes

sejli approved these changes Jul 29, 2024

View reviewed changes

ananzh merged commit 611f56b into opensearch-project:main Jul 29, 2024
66 of 67 checks passed

opensearch-trigger-bot bot added the failed backport label Jul 29, 2024

ananzh mentioned this pull request Jul 30, 2024

[2.x manual backport] BP for #7542, #7540, #7552, #7566, #7567 #7574

Merged

7 tasks

AMoo-Miki removed the failed backport label Aug 22, 2024

AMoo-Miki added the in-next label Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[discover-next][bug] add max height to dataset navigator and use memoization #7540

[discover-next][bug] add max height to dataset navigator and use memoization #7540

kavilla commented Jul 27, 2024

codecov bot commented Jul 27, 2024 •

edited

Loading

sejli left a comment

ashwin-pc left a comment

ashwin-pc Jul 29, 2024

ashwin-pc Jul 29, 2024

ashwin-pc Jul 29, 2024

ashwin-pc Jul 29, 2024

opensearch-trigger-bot bot commented Jul 29, 2024

opensearch-trigger-bot bot commented Jul 29, 2024

AMoo-Miki commented Aug 22, 2024

[discover-next][bug] add max height to dataset navigator and use memoization #7540

[discover-next][bug] add max height to dataset navigator and use memoization #7540

Conversation

kavilla commented Jul 27, 2024

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Jul 27, 2024 • edited Loading

Codecov Report

sejli left a comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc Jul 29, 2024

Choose a reason for hiding this comment

ashwin-pc Jul 29, 2024

Choose a reason for hiding this comment

ashwin-pc Jul 29, 2024

Choose a reason for hiding this comment

ashwin-pc Jul 29, 2024

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jul 29, 2024

opensearch-trigger-bot bot commented Jul 29, 2024

AMoo-Miki commented Aug 22, 2024

codecov bot commented Jul 27, 2024 •

edited

Loading