[Snapshot and Restore] Server side snapshots pagination

[yuliacech]

Summary

This PR removes the limit of maximum 1000 snapshots that can be displayed in the Snapshots tab of the Snapshots & Restore app. The limit was introduced in #103331 to prevent performance issues for clusters with a large number of snapshots. The issue would happen because the UI used to load all existing snapshots into the browser. Now only the current page is loaded with a possibility to load further snapshots when navigating between pages in the table.

UI changes

Even though this PR intended to keep most of the table functionality in terms of pagination, sorting and search, some UI changes were introduced:

1. Previously it was possible to query with several clauses in the search bar, for example -test cloud. Now only 1 clause is allowed.

2. Previously the search bar didn't provide any feedback when a wrong field was used in the query. Now the search bar displays an error if any field other than snapshot, repository or policyName is used in the query.

3. Previously it was possible to search not only by snapshot, repository or policyName but also by startTimeInMillis and durationInMillis followed by a time value in ms, for example startTimeInMillis>1000000000000 or durationInMillis>10000. I believe that both queries are barely used because of the low discoverability of field names and hard to use values. Field names are not displayed anywhere in the UI and the time values are displayed already formatted.

Code changes

A lot of the code changes are from moving the code from SnapshotList to separate components.
This PR leverages changes added to ES API Get snapshot endpoint and refactors the components for the Snapshots tab.

• Added parameters for pagination and sorting to the route /snapshots
• Added parameters for search by snapshot name, repository name and policy name to the route /snapshots
• Refactored the snapshot table to use EuiBasicTable
• Extracted components from SnapshotList to separate files: RepositoryEmptyPrompt, RepositoryError, SnapshotEmptyPrompt, SnapshotSearchBar

How to test

1. start ES with a couple of different folders for repositories: yarn es snapshot --license=trial -E path.repo=/tmp/snaps,/tmp/snaps2 in a terminal window
2. start Kibana: yarn start in a different terminal window
3. Navigate to "Snapshot & Restore" and create repositories using the same folders as in 1.
4. use this forked from @alisonelizabeth gist to create a lot of snapshots at once, for example REPO="test" COUNT=50 SNAPSHOT="my_backup_test" sh ./create_es_snapshots.sh

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

Release Note

In Snapshot & Restore app, the performance of snapshots list is improved to handle a large number of snapshots. There is no more limit of how many snapshots can be displayed in the list.

[yuliacech]

@elasticmachine merge upstream

[yuliacech]

@elasticmachine merge upstream

[yuliacech]

@elasticmachine merge upstream

[yuliacech]

@elasticmachine merge upstream

[yuliacech]

@elasticmachine merge upstream

[cjcenizal]

Hi Yulia I was able to verify the partial match search behavior is now fixed. However, I'm still noticing that the search request isn't submitted until I hit ENTER, whereas before it was search-as-you-type. Can you implement the original search-as-you-type behavior?

I also had a few suggestions to make the tests easier to understand. I think they're getting there but I'm still tripping over them.

Lastly, I think it'd be easier to traverse these tests if we broke up snapshots.ts into multiple files for pagination, sorting, and search, since it's getting really long. Happy to see this implemented in a subsequent PR though!

[cjcenizal]

I'm not quite parsing "When searching for policy name, also add _none to find snapshots without a policy as well".

How does a user search for a policy name? I don't see any information regarding "Policy" in the snapshots UI. I must be missing some context.

What does "without a policy" mean? Assuming I can search by policy name, why do I also want to see snapshots that don't have policies?

[yuliacech]

The - wildcard is used for excluding search, for example: find all snapshots except those created by the SLM policy 'my_policy'. The results should include snapshots created by other SLM policies, as well as snapshots without an SLM policy. Without _none the API would only return snapshots created by other policies.

[cjcenizal]

Oh, I see! Thanks. That's a great example. It might help to include it in the comment.

[cjcenizal]

I don't see total included in elastic/elasticsearch-specification#845. Did you mean to add it, or maybe create a new issue? Either way, can we add the link here?

[cjcenizal]

What prompted the removal of these properties? I've been puzzling over this and can't quite figure it out.

[yuliacech]

This const is used directly with ES client, which needs policyName and name for snapshot name. And we also use this const with our Kibana API and there we use name for policy name and snapshotName.

[cjcenizal]

I think we can improve the readability of these tests by extracting the boilerplate into helper functions, increasing the differentiation points between each test. For example:

      describe('snapshot name', () => {
        const searchSnapshots = async (searchParams) => {
          const {
            body: { snapshots },
          } = await supertest
            .get(getApiPath({
              searchField: 'snapshot',
              ...searchParams
            ))
            .set('kbn-xsrf', 'xxx')
            .send();

          return snapshots;
        };

        it('exact match', async () => {
          // list snapshots with the name "another_snapshot_2"
          const snapshots = await searchSnapshots({
            searchValue: 'another_snapshot_2',
            searchMatch: 'must',
            searchOperator: 'exact',
          });

          expect(snapshots.length).to.eql(1);
          expect(snapshots[0].snapshot).to.eql('another_snapshot_2');
        });

        it('partial match', async () => {
          // list snapshots with the name containing with "another"
          const snapshots = await searchSnapshots({
            searchValue: 'another',
            searchMatch: 'must',
            searchOperator: 'eq',
          });

          // both batches created snapshots containing "another" in the name
          expect(snapshots.length).to.eql(BATCH_SIZE_1 + BATCH_SIZE_2);
          const snapshotNamesContainSearch = snapshots.every((snapshot: SnapshotDetails) =>
            snapshot.snapshot.includes('another')
          );
          expect(snapshotNamesContainSearch).to.eql(true);
        });

        it('excluding search with exact match', async () => {
          // list snapshots with the name not "another_snapshot_2"
          const snapshots = await searchSnapshots({
            searchValue: 'another_snapshot_2',
            searchMatch: 'must_not',
            searchOperator: 'exact',
          });

          expect(snapshots.length).to.eql(SNAPSHOT_COUNT - 1);
          const snapshotIsExcluded = snapshots.every(
            (snapshot: SnapshotDetails) => snapshot.snapshot !== 'another_snapshot_2'
          );
          expect(snapshotIsExcluded).to.eql(true);
        });

[cjcenizal]

I think we can further improve readability by creating variables to represent the relationships that we expect to be maintained. In this example we use searchValue to make the connection clear between what we're searching for (searchValue) and asserting against (searchValue again).

        it('excluding search with exact match', async () => {
          // list snapshots with the name not "another_snapshot_2"
          const searchValue = 'another_snapshot_2';
          const snapshots = await searchSnapshots({
            searchValue,
            searchMatch: 'must_not',
            searchOperator: 'exact',
          });

          expect(snapshots.length).to.eql(SNAPSHOT_COUNT - 1);
          const snapshotIsExcluded = snapshots.every(
            (snapshot: SnapshotDetails) => snapshot.snapshot !== searchValue
          );
          expect(snapshotIsExcluded).to.eql(true);
        });

In other tests where common fixtures have been used, I advocate for concrete assertions because the fixture is defined independently of the test, so the relationships become obscured to someone reading the test. But in this case the test defines the entities being created, so the relationships are clear (to me). Probably just boils down to personal preference though. :)

[cjcenizal]

TBH I still have a hard time reasoning about how we arrive at this number. 😅

[cjcenizal]

I think this test would be improved if the value we're asserting against wasn't the same as the value you get when you filter on a non-existent repo.

Were you planning on addressing this comment?

[cjcenizal]

These comments are really helpful! I think we can take them further to make them even more helpful.

I'm going out on a bit of a limb with this suggestion, but I think what I find challenging about these tests in general, is that we're asserting against strings, and strings are used in repo names, SLM policy names, and snapshot names. So it's hard to keep track of what's related to what. I suspect it might be easier to draw associations between repos, policies, and snapshots if we use names that are easier to compare and contrast.

Also, I think we can make it easier to read this comment if we use indentation to make the lists easier to scan. I've combined both these suggestions in the example below and I'd love to hear your thoughts.

      /*
       * This setup creates these repos, SLM policies, and snapshots:
       *   - "food_repo" with 5 snapshots
       *     - (1) "one_apple_policy_snapshot" (created by SLM policy "apple_policy")
       *     - (1) "one_banana_policy_snapshot" (created by SLM policy "banana_policy")
       *     - (3) "three_carrot_snapshots_[0-2]"
       *
       *   - "animal_repo" with 5 snapshots
       *     - (5) "five_alligator_snapshots_[0-5]"
       */

With the above naming, you can tell which snapshot belongs to which policy, which policy created which snapshot, and how many snapshots are expected to be in each "batch", all from names alone. WDYT? (BTW I removed the ellipsis from the names you were using... I wasn't sure what they meant. Did they have meaning?)

This will also let us create variable names to reflect these values, so we don't need to do any mental translation between abstract and concrete values. Note that I've regrouped these variables in a way that I find easier to read. I'd even suggest moving the comment to where these variables are defined, since the pattern of information is mirrored and I'd expect an engineer to want to edit these values to alter the test setup.

const FOOD_REPO = 'food_repo';
const FOOD_REPO_PATH = '/tmp/repo_1';
const APPLE_POLICY_NAME = 'apple_policy';
const APPLE_POLICY_SNAPSHOT_NAME = 'one_apple_policy_snapshot';
const APPLY_POLICY_SNAPSHOT_COUNT = 1;
const BANANA_POLICY_NAME = 'banana_policy';
const BANANA_POLICY_SNAPSHOT_NAME = 'one_banana_policy_snapshot';
const BANANA_POLICY_SNAPSHOT_COUNT = 1;
const CARROT_SNAPSHOT_NAME_PREFIX = 'three_carrot_snapshots';
const CARROT_SNAPSHOT_COUNT = 3;

const ANIMAL_REPO_NAME = 'animal_repo';
const ANIMAL_REPO_PATH = '/tmp/repo_2';
const ALLIGATOR_SNAPSHOT_NAME_PREFIX = 'five_alligator_snapshots';
const ALLIGATOR_SNAPSHOT_COUNT = 5;

const SNAPSHOT_COUNT = CARROT_SNAPSHOT_COUNT + ALLIGATOR_SNAPSHOT_COUNT + APPLY_POLICY_SNAPSHOT_COUNT + BANANA_POLICY_SNAPSHOT_COUNT;

[yuliacech]

@elasticmachine merge upstream

[cjcenizal]

Thanks for making the search incremental @yuliacech! I'd be happy to see this merged as-is (perhaps with my timing suggestion implemented), with the tests addressed in a subsequent PR (even after FF).

[cjcenizal]

Can we reduce this to 200? Feels a bit sluggish to me at 500.

[yuliacech]

Agree, 500ms debounce might be too long!

[yuliacech]

@elasticmachine merge upstream

[yuliacech]

Thank you so much for reviewing and re-reviewing this huge PR, @cjcenizal! I really liked your idea about making api integration tests more readable with some relatable snapshot names, so I opened this issue to track that. Also this issue that @alisonelizabeth opened tracks adding more CITs for Snapshot & Restore, including snapshots table that's been worked on in this PR.

[yuliacech]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 2d08704
• Storybooks Preview
• Documentation Changes

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
snapshotRestore	182	189	+7

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
snapshotRestore	256.9KB	259.0KB	+2.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
snapshotRestore	28.7KB	29.0KB	+351.0B

Unknown metric groups

API count

id	before	after	diff
snapshotRestore	23	22	-1

History

• 💔 Build #162090 failed e139f95
• 💚 Build #161961 succeeded 9ac27d3
• 💚 Build #161486 succeeded 467f47c
• 💚 Build #160859 succeeded 61d21d4
• 💚 Build #160804 succeeded cfd5f04

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.