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

Table performance paging #7399

Merged
merged 26 commits into from
Jan 26, 2024
Merged

Table performance paging #7399

merged 26 commits into from
Jan 26, 2024

Conversation

jvigliotta
Copy link
Contributor

@jvigliotta jvigliotta commented Jan 22, 2024

partial-close: #7268 (items 1)

Describe your changes:

  • added the ability to configure tables to be in "performance" mode (limited number of rows shown, defaults to 50, but configurable)
  • configurable persistability
  • check for exporting to CSV (must be in "unlimited" mode)
  • table row collections handle a majority of the work by limiting the rows to the configured or default amount and holding onto an additional amount (same as configured/default) to be used if an item is removed to fill in any missing rows
  • realtime and fixed time are supported, in realtime it's a rolling window of the specified row limit
  • at most will be 2x the limit of rows per endpoint

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Has this been smoke tested?
  • Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
  • Have you associated a milestone with this PR? Note: leave blank if unsure.
  • Is this a breaking change to be called out in the release notes?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?

@jvigliotta jvigliotta added type:feature Feature. Required intentional design notable_change A change which should be noted in the changelog labels Jan 22, 2024
@jvigliotta jvigliotta added this to the Target:4.0.0 milestone Jan 22, 2024
Copy link

deploysentinel bot commented Jan 22, 2024

Current Playwright Test Results Summary

✅ 138 Passing - ❌ 4 Failing - ⚠️ 2 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 01/23/2024 02:27:05am UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: 395a735

Started: 01/23/2024 02:18:58am UTC

❌ Failures

📄   functional/plugins/displayLayout/displayLayout.e2e.spec.js • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Display Layout independent time works with display layouts and its children
Retry 1Initial Attempt
Error: Console error detected: [error] Failed to load resource: the server responded wi...
Console error detected: [error] Failed to load resource: the server responded with a status of 404 () at (https://history.nasa.gov/alsj/a16/AS16-117-18747.jpg 0:0)

expect(received).not.toEqual(expected) // deep equality

Expected: not "error"

100% (7) 7 / 7 runs
failed over last 7 days
0% (0) 0 / 7 runs
flaked over last 7 days

📄   functional/plugins/imagery/exampleImagery.e2e.spec.js • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Example Imagery Object Can use Mouse Wheel to zoom in and out of latest image
Retry 1Initial Attempt
Error: Test timeout of 60000ms exceeded while running "beforeEach" hook.
Test timeout of 60000ms exceeded while running "beforeEach" hook.
100% (3) 3 / 3 runs
failed over last 7 days
0% (0) 0 / 3 runs
flaked over last 7 days

📄   functional/clearDataAction.e2e.spec.js • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Clear Data Action works as expected with Example Imagery
Retry 2Retry 1Initial Attempt
Error: Timed out 5000ms waiting for expect(locator).toBeVisible()...
Timed out 5000ms waiting for expect(locator).toBeVisible()

Locator: locator('.c-imagery__main-image__background-image')
Expected: visible
Received: hidden
Call log:
  - expect.toBeVisible with timeout 5000ms
  - waiting for locator('.c-imagery__main-image__background-image')
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"

100% (8) 8 / 8 runs
failed over last 7 days
0% (0) 0 / 8 runs
flaked over last 7 days

📄   functional/plugins/flexibleLayout/flexibleLayout.e2e.spec.js • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Flexible Layout independent time works with flexible layouts and its children
Retry 2Retry 1Initial Attempt
Error: Console error detected: [error] Failed to load resource: the server responded wi...
Console error detected: [error] Failed to load resource: the server responded with a status of 404 () at (https://history.nasa.gov/alsj/a16/AS16-117-18742.jpg 0:0)

expect(received).not.toEqual(expected) // deep equality

Expected: not "error"

100% (8) 8 / 8 runs
failed over last 7 days
0% (0) 0 / 8 runs
flaked over last 7 days

⚠️ Flakes

📄   functional/tooltips.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Verify tooltips display tooltip path for telemetry table names
Retry 1Initial Attempt
0% (0) 0 / 7 runs
failed over last 7 days
42.86% (3) 3 / 7 runs
flaked over last 7 days

📄   functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Restricted Notebook with a page locked and with an embed @addinit Allows embeds to be deleted if page unlocked @addinit
Retry 1Initial Attempt
0% (0) 0 / 8 runs
failed over last 7 days
75% (6) 6 / 8 runs
flaked over last 7 days

View Detailed Build Results


Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 60 lines in your changes are missing coverage. Please review.

Comparison is base (b985619) 55.98% compared to head (fdd390d) 55.90%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7399      +/-   ##
==========================================
- Coverage   55.98%   55.90%   -0.09%     
==========================================
  Files         662      662              
  Lines       26304    26394      +90     
  Branches     2551     2563      +12     
==========================================
+ Hits        14727    14756      +29     
- Misses      10868    10925      +57     
- Partials      709      713       +4     
Flag Coverage Δ *Carryforward flag
e2e-full 41.04% <ø> (-0.99%) ⬇️ Carriedforward from adc8ba8
e2e-stable 59.83% <58.92%> (-0.04%) ⬇️
unit 48.92% <34.00%> (-0.07%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
example/generator/GeneratorProvider.js 93.54% <100.00%> (+0.44%) ⬆️
src/plugins/telemetryTable/TelemetryTableType.js 100.00% <100.00%> (ø)
src/plugins/telemetryTable/TelemetryTableView.js 88.88% <ø> (ø)
...ugins/telemetryTable/TelemetryTableViewProvider.js 100.00% <100.00%> (ø)
src/plugins/telemetryTable/plugin.js 100.00% <100.00%> (ø)
src/api/telemetry/TelemetryCollection.js 84.69% <50.00%> (-1.10%) ⬇️
...s/telemetryTable/collections/TableRowCollection.js 56.46% <55.55%> (-0.06%) ⬇️
src/plugins/telemetryTable/TelemetryTable.js 80.09% <64.28%> (-1.29%) ⬇️
...gins/telemetryTable/TelemetryTableConfiguration.js 63.15% <38.88%> (-7.54%) ⬇️
...telemetryTable/components/TableFooterIndicator.vue 24.59% <6.66%> (-0.87%) ⬇️
... and 1 more

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b985619...ff0d393. Read the comment docs.

…e can go back to the most recent rows for all remaining items and repopulate the table if necessary
@jvigliotta jvigliotta marked this pull request as ready for review January 23, 2024 02:37
- Layout and style sanding and polishing.
- Added title to button.
- More direct button labeling.
Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

Added a bit of bondo and sanded:

  • Fixed alignment and button classing.
  • Changed button labeling.
  • Added button title.
  • Footer now always displayed, has more compact layout.

LGTM!

charlesh88 and others added 4 commits January 24, 2024 09:23
Partially closes #7147
- Removed footer hover behavior: table footer now always visible.
- Tweaks to style, margin etc. to make footer more compact.
…llections, table row collections will only limit what they return in getRows, handling sorting when in different modes
const removeCount = this.boundedTelemetry.length - size;
const removed = this.boundedTelemetry.splice(0, removeCount);

this.emit('remove', removed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, this is a simple way of enforcing size with a small change

return;
}

this.telemetryMode = mode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Spacing is a little weird here. Did prettier demand it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea on autosave

Copy link
Contributor

Choose a reason for hiding this comment

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

thats how i would have wrote it... hehe

@unlikelyzero unlikelyzero merged commit b9df97e into master Jan 26, 2024
4 of 9 checks passed
@unlikelyzero unlikelyzero deleted the table-performance-paging branch January 26, 2024 21:24
@akhenry
Copy link
Contributor

akhenry commented Jan 26, 2024

Created #7419 to followup on e2e tests next sprint.

@jvigliotta jvigliotta mentioned this pull request Jan 29, 2024
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable_change A change which should be noted in the changelog type:feature Feature. Required intentional design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve telemetry table performance
5 participants