[Discover] Adding Recent Datasets #8133

sejli · 2024-09-11T02:57:29Z

Description

Adds recent datasets to dataset selector. Also fixes an issue where we were able to select an index without specifying a time field option.

Issues Resolved

Screenshot

Testing the changes

Changelog

feat: Adds recently selected data to data selector

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-09-11T02:58:01Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

codecov · 2024-09-11T03:11:53Z

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.98%. Comparing base (d1caa92) to head (18d458d).

Files with missing lines	Patch %	Lines
...ry/query_string/dataset_service/dataset_service.ts	66.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8133      +/-   ##
==========================================
- Coverage   60.98%   60.98%   -0.01%     
==========================================
  Files        3743     3743              
  Lines       88857    88863       +6     
  Branches    13859    13861       +2     
==========================================
+ Hits        54188    54190       +2     
- Misses      31314    31315       +1     
- Partials     3355     3358       +3

Flag	Coverage Δ
Linux_1	`28.85% <0.00%> (-0.01%)`	⬇️
Linux_2	`56.34% <ø> (ø)`
Linux_3	`?`
Linux_4	`29.93% <0.00%> (-0.01%)`	⬇️
Windows_1	`28.86% <0.00%> (-0.01%)`	⬇️
Windows_2	`56.29% <ø> (ø)`
Windows_3	`37.79% <66.66%> (+<0.01%)`	⬆️
Windows_4	`29.93% <0.00%> (-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.

ashwin-pc

Nice! Can we add some tests for this feature so that we can be sure that these dont break as we fix more things?

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

src/plugins/data/public/ui/dataset_selector/dataset_selector.tsx

ashwin-pc · 2024-09-11T03:44:48Z

How are you limiting the recents?

src/plugins/data/public/ui/dataset_selector/dataset_selector.tsx

src/plugins/data/public/ui/dataset_selector/index.tsx

kavilla · 2024-09-12T21:33:06Z

src/plugins/data/public/ui/dataset_selector/index.tsx

@@ -34,6 +35,7 @@ const ConnectedDatasetSelector = ({ onSubmit }: ConnectedDatasetSelectorProps) =
      const query = queryString.getInitialQueryByDataset(dataset);
      queryString.setQuery(query);
      onSubmit!(queryString.getQuery());
+      services.data.query.queryString.getDatasetService().addRecentDataset(dataset);


queryString.getDatasetService().addRecentDataset(initialDataset);

@ashwin-pc do you think we should be just handling this is the query string manager? like updating the recents based on the query?

Yes that would make it less prone to issues like when datasets are selected using ways that arent the dataset selector. Wouldnt block on that being a requirement, but that is a cleaner approach

@sejli maybe could be an open issue

kavilla · 2024-09-17T21:25:09Z

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

@@ -31,6 +33,9 @@ export class DatasetService {
    if (this.uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED)) {
      this.registerDefaultTypes();
    }
+    this.recentDatasets = new LRUCache({
+      max: this.uiSettings.get(UI_SETTINGS.SEARCH_MAX_RECENT_DATASETS) ?? 4,


nit: could this actually be undefined?

While running tests, uiSettings has everything set to be jest.fn(), which is the only place where it would be undefined. Could override the original uiSettings mock such that uiSettings.get() returns 4, would that be better practice?

yes, dont change the UI implementation to meet a test infrastructure gap if possible.

kavilla · 2024-09-18T23:07:05Z

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

+    return Array.from(this.recentDatasets.values());
+  }
+
+  public addRecentDataset(dataset: Dataset | undefined): void {


do you think instead of relying on the UI component for setting the dataset the recent dataset is the one that is used? so like nobody could add recent dataset they can only access the recent data sets?

paulstn · 2024-09-23T17:45:56Z

src/plugins/data/public/ui/dataset_selector/configurator.tsx

@@ -165,6 +165,7 @@ export const Configurator = ({
            onConfirm(dataset);
          }}
          fill
+          disabled={timeFields && timeFields.length > 0 && timeFieldName === undefined}


nit: but couldn't you combine the first two expressions down to timeFields?.length > 0.
do you think that the way you did it would be better style?

joshuali925 · 2024-09-23T22:22:51Z

src/plugins/data/public/ui/dataset_selector/dataset_selector.tsx

+      setSelectedDataset(
+        allDatasets.filter((ds) => ds.recent)[0].dataset ??
+          allDatasets.filter((ds) => !ds.recent)[0].dataset
+      );


is the second filter needed? since if there's no recent datasets, then every dataset should satisfy !ds.recent

also recent datasets are added at first anyways, this is just allDatasets[0].dataset?

joshuali925 · 2024-09-23T22:24:44Z

src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts

@@ -23,6 +24,7 @@ export class DatasetService {
  private indexPatterns?: IndexPatternsContract;
  private defaultDataset?: Dataset;
  private typesRegistry: Map<string, DatasetTypeConfig> = new Map();
+  private recentDatasets: LRUCache<string, Dataset>;


is this just in memory or stored in some storage?

Currently, we'll just store it in memory.

joshuali925 · 2024-09-23T22:29:05Z

src/plugins/data/public/ui/dataset_selector/dataset_selector.tsx

+      (dataset) => !recentDatasets.some((ds) => ds.id === dataset.id)
+    );
+    const allDatasets = [
+      ...recentDatasets.map((ds) => ({ dataset: ds, recent: true })),


would there be invalid/outdated datasets? should we check and make sure every recent dataset appears in the fetchedDatasets?

Signed-off-by: Sean Li <lnse@amazon.com>

sejli added backport 2.x discover for discover reinvent labels Sep 11, 2024

github-actions bot added the seasoned-contributor label Sep 11, 2024

github-actions bot added the failed changeset label Sep 11, 2024

ashwin-pc reviewed Sep 11, 2024

View reviewed changes

kavilla reviewed Sep 12, 2024

View reviewed changes

src/plugins/data/public/ui/dataset_selector/dataset_selector.tsx Outdated Show resolved Hide resolved

kavilla reviewed Sep 12, 2024

View reviewed changes

src/plugins/data/public/ui/dataset_selector/index.tsx Outdated Show resolved Hide resolved

kavilla reviewed Sep 12, 2024

View reviewed changes

kavilla added the v2.18.0 label Sep 12, 2024

kavilla reviewed Sep 17, 2024

View reviewed changes

ashwin-pc added 2.17.1 and removed v2.18.0 labels Sep 17, 2024

kavilla reviewed Sep 18, 2024

View reviewed changes

sejli added the backport 2.17 label Sep 21, 2024

github-actions bot removed the failed changeset label Sep 21, 2024

opensearch-changeset-bot bot added a commit to sejli/OpenSearch-Dashboards that referenced this pull request Sep 21, 2024

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

7b188aa

kavilla previously approved these changes Sep 23, 2024

View reviewed changes

paulstn reviewed Sep 23, 2024

View reviewed changes

joshuali925 reviewed Sep 23, 2024

View reviewed changes

kavilla added the backport 2.17-next label Sep 26, 2024

sejli added 6 commits September 26, 2024 14:34

Adding recent datasets to dataset selector

e0a7a66

Signed-off-by: Sean Li <lnse@amazon.com>

addressing comments, adding tests

dfa9c47

Signed-off-by: Sean Li <lnse@amazon.com>

moving recent datasets to fetched datasets

17deb55

Signed-off-by: Sean Li <lnse@amazon.com>

mocking UI settings for test

47ad3ce

Signed-off-by: Sean Li <lnse@amazon.com>

fixing connect_to_query_state.test

633f666

Signed-off-by: Sean Li <lnse@amazon.com>

fixing sync_state_with_url test

18d458d

Signed-off-by: Sean Li <lnse@amazon.com>

sejli dismissed kavilla’s stale review via 18d458d September 26, 2024 21:36

sejli force-pushed the recent-datasets branch from 7b188aa to 18d458d Compare September 26, 2024 21:36

sejli removed backport 2.17 backport 2.17-next labels Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Adding Recent Datasets #8133

[Discover] Adding Recent Datasets #8133

sejli commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

ashwin-pc left a comment

ashwin-pc commented Sep 11, 2024

kavilla Sep 12, 2024

kavilla Sep 12, 2024

ashwin-pc Sep 16, 2024

kavilla Sep 23, 2024

kavilla Sep 17, 2024

sejli Sep 17, 2024

ashwin-pc Sep 17, 2024

kavilla Sep 18, 2024

paulstn Sep 23, 2024

joshuali925 Sep 23, 2024

joshuali925 Sep 23, 2024

sejli Sep 26, 2024

joshuali925 Sep 23, 2024

[Discover] Adding Recent Datasets #8133

Are you sure you want to change the base?

[Discover] Adding Recent Datasets #8133

Conversation

sejli commented Sep 11, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Sep 11, 2024

❌ Empty Changelog Section

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sejli commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading