Add an unified storage class for data plugin #7701

abbyhu2000 · 2024-08-13T23:48:19Z

Description

Created a more advanced storage class for data plugin
Refactor logics from previous storage usages
Add Query History class for future recent query use

Screenshot

Testing the changes

Changelog

feat: Add a unified storage class for data plugin

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-08-13T23:48:51Z

❌ 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-08-14T00:04:01Z

Codecov Report

Attention: Patch coverage is 38.33333% with 37 lines in your changes missing coverage. Please review.

Project coverage is 63.81%. Comparing base (21b8a2f) to head (179c199).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...ns/data/public/query/query_string/query_history.ts	25.00%	11 Missing and 1 partial ⚠️
src/plugins/data/public/ui/settings/settings.ts	0.00%	8 Missing ⚠️
src/plugins/data/common/storage/storage.ts	70.58%	2 Missing and 3 partials ⚠️
.../public/query/query_string/query_string_manager.ts	37.50%	5 Missing ⚠️
...c/plugins/data/public/ui/search_bar/search_bar.tsx	40.00%	3 Missing ⚠️
src/plugins/data/public/plugin.ts	0.00%	1 Missing ⚠️
...s/data/public/query/persisted_log/persisted_log.ts	0.00%	1 Missing ⚠️
...public/ui/query_string_input/query_bar_top_row.tsx	0.00%	1 Missing ⚠️
...ublic/ui/query_string_input/query_string_input.tsx	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7701      +/-   ##
==========================================
- Coverage   63.82%   63.81%   -0.02%     
==========================================
  Files        3651     3653       +2     
  Lines       80951    80995      +44     
  Branches    12885    12892       +7     
==========================================
+ Hits        51666    51683      +17     
- Misses      26122    26144      +22     
- Partials     3163     3168       +5

Flag	Coverage Δ
Linux_2	`55.81% <0.00%> (-0.04%)`	⬇️
Linux_3	`?`
Linux_4	`31.27% <0.00%> (-0.03%)`	⬇️
Windows_1	`30.01% <0.00%> (-0.02%)`	⬇️
Windows_2	`55.76% <0.00%> (-0.04%)`	⬇️
Windows_3	`40.41% <38.33%> (-0.01%)`	⬇️
Windows_4	`31.27% <0.00%> (-0.03%)`	⬇️

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/data/common/storage/index.ts

src/plugins/data/common/storage/storage.ts

kavilla · 2024-08-15T09:25:26Z

src/plugins/data/common/storage/storage.ts

+}
+
+type IStorageEngine = typeof window.localStorage;
+export class DataStorage {


i do like this structure. it makes it easier to not use opensearchDashboards. and just pass the key

kavilla

Awesome. Thank you and some nice clean ups.

Just comments but nothing blocking. If potentially can add some tests where possible.

src/plugins/data/public/ui/query_string_input/no_data_popover.tsx

kavilla · 2024-08-15T10:28:48Z

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

@@ -381,13 +381,13 @@ export default class QueryStringInputUI extends Component<Props, State> {
      'field' in suggestion &&
      suggestion.field.subType &&
      suggestion.field.subType.nested &&
-      !this.services.storage.get('opensearchDashboards.DQLNestedQuerySyntaxInfoOptOut')


this is really nice! thank you

kavilla · 2024-08-15T10:32:43Z

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

+  getHistoryKeys() {
+    return this.storage
+      .keys()
+      .filter((key: string) => key.indexOf('query_') === 0)


could you explain the decision to go with query_ it makes sense to me but do we have to make it more specific to avoid potential collisions? did we look into the decision of the name of the key ssense

yea not sure about the meaning of "ssense" , but i think "_query" makes sense for saving recent query here? They full key is opensearch_dashboards.query_1723753938175, the numbers followed are the timetamp, i think it will avoid collisions?

kavilla · 2024-08-15T10:33:54Z

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

+import { BehaviorSubject } from 'rxjs';
+import { Query, TimeRange } from '../..';
+
+// Todo: Implement a more advanced QueryHistory class when needed for recent query history


seems pretty advanced hah nice. and only has functionality for recents. or do you mean configurable

kavilla · 2024-08-15T10:35:35Z

src/plugins/data/public/ui/settings/settings.ts

  }

  setUserQueryLanguageBlocklist(languages: string[]) {
    this.storage.set(
-      'opensearchDashboards.userQueryLanguageBlocklist',
+      'userQueryLanguageBlocklist',
      languages.map((language) => language.toLowerCase())
    );
    return true;
  }

  getUserQueryLanguage() {


to be honest i think we can start cleaning this up and centralizing the language stuff with the language manager. what do you think? or is this useful here still?

Yea i plan to include these language related storage stuff in the language manager, and get rid of the settings class.

kavilla · 2024-08-15T10:35:51Z

src/plugins/data/public/ui/settings/settings.ts

  }

  setUserQueryLanguage(language: string) {
    if (language !== this.getUserQueryLanguage()) {
      this.search.df.clear();
    }
-    this.storage.set('opensearchDashboards.userQueryLanguage', language);


same for the rest

kavilla · 2024-08-15T10:39:50Z

src/plugins/data/public/ui/query_string_input/query_bar_top_row.tsx

@@ -379,7 +379,7 @@ export default function QueryBarTopRow(props: QueryBarTopRowProps) {

  function onLuceneSyntaxWarningOptOut(toast: Toast) {
    if (!storage) return;
-    storage.set('opensearchDashboards.luceneSyntaxWarningOptOut', true);
+    storage.set('luceneSyntaxWarningOptOut', true);


nothing blocking this PR, but should we consider making apart of the language manager a generic function that will be invoked when the language is switched. developers can register a language with a function like onLanguageOptIn and onLanguageOptOut and we will call for that to do as they please. Like potentially updating a UI metric saved object.

kavilla · 2024-08-15T10:48:42Z

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

+    return () => subscription.unsubscribe();
+  }
+
+  addQueryToHistory(dataSet: string, query: Query, dateRange?: TimeRange) {


do you think we could use the SimpleDataSet object so that when I refactor to match the desired state it will be easier to catch.

Or do you see any issue with using the dataset object here.

Here is diagram:

assets/127f96cd-2e40-4f80-8e48-94bf7da84b6d">

SimpleDataSet todaywill have this information that will be important for requery.

Sure modified this prop to be simple data set.

kavilla · 2024-08-15T10:53:30Z

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

+      dataSet,
+      time: timestamp,
+      query,
+      dateRange,


here is the current saved query saved object.

I think we should follow the same format and property names as the saved query.

you have query, but i would rename dataRange to timeFilter and i would include filters as well.

time could probaby could be submitted_at

should we consider workspaces @ashwin-pc ?

so inside the onQueryBarSubmit function, we only get time range as props. But time filter includes both time range and refresh interval. So do you think we need to include refresh interval and the filters here?

public onQueryBarSubmit = (queryAndDateRange: { dateRange?: TimeRange; query?: Query }) => {

kavilla · 2024-08-15T11:01:39Z

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

@@ -371,6 +373,14 @@ class SearchBarUI extends Component<SearchBarProps, State> {
        }
      }
    );
+    const dataSet = this.dataSetService.getDataSet();


I think no other plugin utilizes datasets besides Discover. However, should we consider this while designing this recent query and history. Specifically the app where the query was sent. so that if the current app path is not the same app path then we do not display the query. in case one app supports different languages but another doesn't

Signed-off-by: abbyhu2000 <[email protected]>

* add an unified storage class Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]> (cherry picked from commit 9a84202) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* add an unified storage class Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]>

* add an unified storage class --------- (cherry picked from commit 9a84202) Signed-off-by: abbyhu2000 <[email protected]> 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>

abbyhu2000 marked this pull request as ready for review August 13, 2024 23:48

github-actions bot added the distinguished-contributor label Aug 13, 2024

github-actions bot added the failed changeset label Aug 13, 2024

abbyhu2000 added discover-next v2.17.0 labels Aug 13, 2024

github-actions bot removed the failed changeset label Aug 13, 2024

opensearch-changeset-bot bot added a commit to abbyhu2000/OpenSearch-Dashboards that referenced this pull request Aug 13, 2024

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

f932775

kavilla reviewed Aug 15, 2024

View reviewed changes

src/plugins/data/common/storage/index.ts Outdated Show resolved Hide resolved

kavilla reviewed Aug 15, 2024

View reviewed changes

src/plugins/data/common/storage/storage.ts Show resolved Hide resolved

kavilla reviewed Aug 15, 2024

View reviewed changes

kavilla previously approved these changes Aug 15, 2024

View reviewed changes

abbyhu2000 dismissed kavilla’s stale review via f4dcc75 August 15, 2024 19:09

abbyhu2000 force-pushed the query_storage_new branch 2 times, most recently from c9a7eda to 179c199 Compare August 15, 2024 21:48

abbyhu2000 added 7 commits August 16, 2024 21:54

add an unified storage class

5750155

Signed-off-by: abbyhu2000 <[email protected]>

fix license headers

87ab64c

Signed-off-by: abbyhu2000 <[email protected]>

modify type to simple dataset

3b27524

Signed-off-by: abbyhu2000 <[email protected]>

fix unit test failure

29f90c5

Signed-off-by: abbyhu2000 <[email protected]>

fix another unit test failure

90ebecd

Signed-off-by: abbyhu2000 <[email protected]>

fix unit test

54b8fb2

Signed-off-by: abbyhu2000 <[email protected]>

fix unit test

36ed793

Signed-off-by: abbyhu2000 <[email protected]>

abbyhu2000 force-pushed the query_storage_new branch from 0955e9f to 36ed793 Compare August 16, 2024 23:06

kavilla approved these changes Aug 19, 2024

View reviewed changes

sejli approved these changes Aug 19, 2024

View reviewed changes

abbyhu2000 merged commit 9a84202 into opensearch-project:main Aug 19, 2024
65 checks passed

abbyhu2000 added the backport 2.x label Aug 19, 2024

opensearch-trigger-bot bot mentioned this pull request Aug 19, 2024

[Backport 2.x] Add an unified storage class for data plugin #7755

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an unified storage class for data plugin #7701

Add an unified storage class for data plugin #7701

abbyhu2000 commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024

codecov bot commented Aug 14, 2024 •

edited

Loading

kavilla Aug 15, 2024

kavilla left a comment

kavilla Aug 15, 2024

kavilla Aug 15, 2024

abbyhu2000 Aug 15, 2024

kavilla Aug 15, 2024

kavilla Aug 15, 2024

abbyhu2000 Aug 15, 2024

kavilla Aug 15, 2024

kavilla Aug 15, 2024

kavilla Aug 15, 2024

abbyhu2000 Aug 15, 2024

kavilla Aug 15, 2024

abbyhu2000 Aug 15, 2024

kavilla Aug 15, 2024

Add an unified storage class for data plugin #7701

Add an unified storage class for data plugin #7701

Conversation

abbyhu2000 commented Aug 13, 2024 • edited Loading

Description

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Aug 13, 2024

❌ Empty Changelog Section

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

kavilla left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abbyhu2000 commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading