Improve the perceived performance of Discover #6599

AMoo-Miki · 2024-04-22T21:00:55Z

Description

Removed the simultaneous rendering of DefaultDiscoverTable and DataGrid when only one would be displayed.
Bumped DOMPurify for a boost in performance.

Issues Resolved

Screenshot

2.13 - 10s span

Click to start of network call: 2392ms
Data to end of render: 5384ms
Click to end of render: 8016ms (network call 256ms)

PR - 10s span

Click to start of network call: 1289ms - 46% improvement
Data to end of render: 1626ms - 70% improvement
Click to end of render: 3186ms (network call 261ms) - 62% improvement

Changelog

feat: Improve the perceived performance of Discover when using the default tabular renderer

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

github-actions · 2024-04-22T21:01:32Z

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

github-actions · 2024-04-22T21:05:11Z

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

codecov · 2024-04-22T21:20:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.72%. Comparing base (d2d410b) to head (82dd4c3).
Report is 23 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6599       +/-   ##
===========================================
+ Coverage   32.93%   67.72%   +34.79%     
===========================================
  Files        2260     3401     +1141     
  Lines       45769    66574    +20805     
  Branches     7200    10819     +3619     
===========================================
+ Hits        15075    45089    +30014     
+ Misses      29984    18850    -11134     
- Partials      710     2635     +1925

Flag	Coverage Δ
Linux_1	`33.06% <ø> (+0.13%)`	⬆️
Linux_2	`55.65% <ø> (?)`
Linux_3	`45.21% <ø> (?)`
Linux_4	`34.95% <ø> (?)`
Windows_1	`33.08% <ø> (?)`
Windows_2	`55.62% <ø> (?)`
Windows_3	`45.21% <ø> (?)`
Windows_4	`34.95% <ø> (?)`

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.

src/plugins/discover/public/application/components/data_grid/data_grid.tsx

ashwin-pc · 2024-04-23T16:26:05Z

@AMoo-Miki Can you add the changeset file?

Signed-off-by: Miki <miki@amazon.com>

ashwin-pc

I havent reviewed the data grid component because you plan on deprecating it anyways and is apparently the exact same code for now. The rest LGTM.

src/plugins/discover/public/application/components/data_grid/data_grid_table.tsx

virajsanghvi

Feel free not to block on my comments

virajsanghvi · 2024-04-24T00:02:33Z

src/plugins/discover/public/application/components/data_grid/data_grid.tsx

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import './_data_grid_table.scss';


Does the css get imported twice or is it deduplicated? (not sure if the dedupe option is turned on for the loader)

Given this will be removed, feel free to ignore if this ends up having no real impact.

You are right. The other one needs to go. Will add it to s subsequent PR.

virajsanghvi · 2024-04-24T00:12:12Z

...ins/discover/public/application/components/default_discover_table/default_discover_table.tsx

  const displayedColumns = getLegacyDisplayedColumns(
    columns,
    indexPattern,
    hideTimeColumn,
    isShortDots
  );
  const displayedColumnNames = displayedColumns.map((column) => column.name);
-  const pageSize = 50;
-  const [renderedRowCount, setRenderedRowCount] = useState(50); // Start with 50 rows
+  const pageSize = 10;


Is this an intended change in pageSize from previous behavior?

Yes. Being infinite-scrolled, 10 is nice balance between performance and initial view. In a future iteration, we will add lazy loading of the subsequent rows to make it even more unnoticeable.

virajsanghvi · 2024-04-24T00:17:29Z

test/functional/apps/dashboard/dashboard_time_picker.js

-      // Current data grid loads 100 rows per page by default with inspect button and time range
-      await dashboardExpect.savedSearchRowCountFromLegacyTable(100);
+      // DefaultDiscoverTable loads 10 rows initially
+      await dashboardExpect.rowCountFromDefaultDiscoverTable(10);


nit: does it makes sense to export pageSize as a constant and use that here to avoid future updates?

~~It does. I will add that.~~

On second thought, when lazy-loading is added, depending on when the test attempts to count, we could have pageSize or greater than that. I will make this test act accordingly, using an exported value, to be future-safe.

opensearch-trigger-bot · 2024-04-24T00:20:10Z

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-6599-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b2c4b9c1b6128c2a0a20a54bd1865c47e4e0705a
# Push it to GitHub
git push --set-upstream origin backport/backport-6599-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-6599-to-2.x.

virajsanghvi · 2024-04-24T00:21:52Z

One question regarding perf metrics: are those tracked somewhere? Just curious if there's a way of validating the expected gains hold in production builds.

ruanyl · 2024-04-24T09:21:07Z

@AMoo-Miki FYI, the auto backport failed, you may need to do it manually.

kavilla · 2024-04-24T16:48:31Z

@ruanyl yes that is something that we need.

AMoo-Miki · 2024-04-24T17:50:26Z

are those tracked somewhere?

No but I intend to add code coverage and performance metrics gathering to my test framework.

Just curious if there's a way of validating the expected gains hold in production builds.

The graphs I placed above were from prod builds.

Signed-off-by: Miki <miki@amazon.com> (cherry picked from commit b2c4b9c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

(cherry picked from commit b2c4b9c) Signed-off-by: Miki <miki@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Signed-off-by: Miki <miki@amazon.com>

AMoo-Miki requested review from ananzh, kavilla, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT and xinruiba as code owners April 22, 2024 21:00

github-actions bot added the distinguished-contributor label Apr 22, 2024

AMoo-Miki added backport 2.x v2.14.0 labels Apr 22, 2024

github-actions bot added the failed changeset label Apr 22, 2024

AMoo-Miki force-pushed the discover-performance branch from 903eb14 to 2e2ddde Compare April 22, 2024 23:42

ruanyl reviewed Apr 23, 2024

View reviewed changes

Improve the perceived performance of Discover

82dd4c3

Signed-off-by: Miki <miki@amazon.com>

AMoo-Miki force-pushed the discover-performance branch from 2e2ddde to 82dd4c3 Compare April 23, 2024 17:18

ashwin-pc approved these changes Apr 23, 2024

View reviewed changes

src/plugins/discover/public/application/components/data_grid/data_grid_table.tsx Show resolved Hide resolved

ruanyl approved these changes Apr 23, 2024

View reviewed changes

ashwin-pc merged commit b2c4b9c into opensearch-project:main Apr 24, 2024
67 checks passed

virajsanghvi reviewed Apr 24, 2024

View reviewed changes

opensearch-trigger-bot bot added the failed backport label Apr 24, 2024

AMoo-Miki mentioned this pull request Apr 24, 2024

[Backport 2.x] [Discover] Fix lazy loading #6622

Merged

AMoo-Miki added backport 2.x and removed backport 2.x failed backport labels Apr 25, 2024

opensearch-trigger-bot bot mentioned this pull request Apr 25, 2024

[Backport 2.x] Improve the perceived performance of Discover #6637

Merged

AMoo-Miki removed the failed changeset label Apr 30, 2024

LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 3, 2024

Improve the perceived performance of Discover (opensearch-project#6599)

c28f933

Signed-off-by: Miki <miki@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the perceived performance of Discover #6599

Improve the perceived performance of Discover #6599

AMoo-Miki commented Apr 22, 2024 •

edited

github-actions bot commented Apr 22, 2024

github-actions bot commented Apr 22, 2024

codecov bot commented Apr 22, 2024 •

edited

ashwin-pc commented Apr 23, 2024

ashwin-pc left a comment

virajsanghvi left a comment

virajsanghvi Apr 24, 2024

AMoo-Miki Apr 30, 2024

virajsanghvi Apr 24, 2024

AMoo-Miki Apr 24, 2024

virajsanghvi Apr 24, 2024

AMoo-Miki Apr 24, 2024 •

edited

opensearch-trigger-bot bot commented Apr 24, 2024

virajsanghvi commented Apr 24, 2024

ruanyl commented Apr 24, 2024

kavilla commented Apr 24, 2024

AMoo-Miki commented Apr 24, 2024

Improve the perceived performance of Discover #6599

Improve the perceived performance of Discover #6599

Conversation

AMoo-Miki commented Apr 22, 2024 • edited

Description

Issues Resolved

Screenshot

2.13 - 10s span

PR - 10s span

Changelog

Check List

github-actions bot commented Apr 22, 2024

❌ Invalid Prefix For Manual Changeset Creation

github-actions bot commented Apr 22, 2024

❌ Invalid Prefix For Manual Changeset Creation

codecov bot commented Apr 22, 2024 • edited

Codecov Report

ashwin-pc commented Apr 23, 2024

ashwin-pc left a comment

Choose a reason for hiding this comment

virajsanghvi left a comment

Choose a reason for hiding this comment

virajsanghvi Apr 24, 2024

Choose a reason for hiding this comment

AMoo-Miki Apr 30, 2024

Choose a reason for hiding this comment

virajsanghvi Apr 24, 2024

Choose a reason for hiding this comment

AMoo-Miki Apr 24, 2024

Choose a reason for hiding this comment

virajsanghvi Apr 24, 2024

Choose a reason for hiding this comment

AMoo-Miki Apr 24, 2024 • edited

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Apr 24, 2024

virajsanghvi commented Apr 24, 2024

ruanyl commented Apr 24, 2024

kavilla commented Apr 24, 2024

AMoo-Miki commented Apr 24, 2024

AMoo-Miki commented Apr 22, 2024 •

edited

codecov bot commented Apr 22, 2024 •

edited

AMoo-Miki Apr 24, 2024 •

edited