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

[Discover] Data selector enhancement #6571

Merged

Conversation

mengweieric
Copy link
Collaborator

@mengweieric mengweieric commented Apr 22, 2024

Description

Discover data selector enhancements includes:

  • Add a few missing i18n
  • Add id, improve data source metadata with fields addition for data sources
  • Improve data source filtering
  • Add in-memory cache for data selector list with refresh button
  • Improve data selector rendering logics
  • Removed hard coded data source type-to-display mapping from selector and read from metadata
  • Increased test coverage
  • Types and interfaces improvements

Issues Resolved

Screenshot

Screenshot 2024-04-29 at 12 03 46 PM

Testing the changes

Changelog

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

@mengweieric mengweieric changed the title Feature/data selector enhancement [Discover] Data selector enhancement Apr 22, 2024
Copy link
Contributor

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 6571.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.

Copy link
Contributor

❌ Changeset File Not Added Yet

Please ensure manual commit for changeset file 6571.yml under folder changelogs/fragments to complete this PR. File still missing.

@mengweieric mengweieric force-pushed the feature/data-selector-enhancement branch 2 times, most recently from 535f64d to 9045b7a Compare April 26, 2024 03:38
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 72.63158% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 67.74%. Comparing base (cfe7cd1) to head (c8753d9).
Report is 2 commits behind head on main.

Files Patch % Lines
..._sources/datasource_services/datasource_service.ts 59.25% 9 Missing and 2 partials ⚠️
...rces/datasource_selector/datasource_selectable.tsx 78.78% 3 Missing and 4 partials ⚠️
src/plugins/data/public/plugin.ts 0.00% 4 Missing ⚠️
...lugins/discover/public/__mock__/index.test.mock.ts 25.00% 3 Missing ⚠️
.../data_explorer/public/components/sidebar/index.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6571   +/-   ##
=======================================
  Coverage   67.73%   67.74%           
=======================================
  Files        3415     3417    +2     
  Lines       66851    66887   +36     
  Branches    10874    10878    +4     
=======================================
+ Hits        45282    45312   +30     
- Misses      18923    18926    +3     
- Partials     2646     2649    +3     
Flag Coverage Δ
Linux_1 33.19% <20.68%> (-0.01%) ⬇️
Linux_2 55.59% <ø> (ø)
Linux_3 ?
Linux_4 ?
Windows_1 33.24% <20.68%> (+0.01%) ⬆️
Windows_2 55.56% <ø> (ø)
Windows_3 45.27% <72.63%> (+0.02%) ⬆️
Windows_4 34.85% <20.68%> (-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.

@mengweieric mengweieric force-pushed the feature/data-selector-enhancement branch from 9045b7a to 03fae8b Compare April 26, 2024 19:43
@mengweieric mengweieric force-pushed the feature/data-selector-enhancement branch from 03fae8b to 9005834 Compare April 28, 2024 21:00
@mengweieric mengweieric marked this pull request as ready for review April 29, 2024 16:57
kavilla
kavilla previously approved these changes Apr 30, 2024
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Link is not failing because of this PR. All good to me

kavilla
kavilla previously approved these changes Apr 30, 2024
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Can you outline in the PR description what exactly this change is about? Currently it looks like this addresses all the feedback from the previous PR and closes the Data Sources issue. However i see in here that S3 and Spark data sources are still hard coded and the registration function is still not generic enough to handle future data sources.

CHANGELOG.md Outdated Show resolved Hide resolved
import { i18n } from '@osd/i18n';
import { DataSourceUIGroupType } from './datasource/types';

export const S3_GLUE_DATA_SOURCE_DISPLAY_NAME = 'Amazon S3';
Copy link
Member

Choose a reason for hiding this comment

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

Are we still hard coding these? Is there a plan to remove them?

data: T;
}

export enum DataSourceUIGroupType {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, do we have a plan on getting away from hardcoding datasource types in this service? These should be generic enough to allow different groups of datasources.

Signed-off-by: Eric <[email protected]>
@mengweieric
Copy link
Collaborator Author

Can you outline in the PR description what exactly this change is about? Currently it looks like this addresses all the feedback from the previous PR and closes the Data Sources issue. However i see in here that S3 and Spark data sources are still hard coded and the registration function is still not generic enough to handle future data sources.

Thanks for the review.

The removal of hardcoding in this PR is to remove previous hardcoded type-to-display-name mapping and instead reading from data source metadata. The s3 data sources are backend data sources where it doesn't have any UI specific definitions. Ideally these front end specific definitions should be retrieving from settings or front end data sources separated from backend data sources where user can configure them. Does this align with the direction you are pointing to?

As for registration function, are you point to the registerDataSources in data source service or the way data source owner register the data source?

Signed-off-by: Eric <[email protected]>
@kavilla
Copy link
Member

kavilla commented Apr 30, 2024

Can you outline in the PR description what exactly this change is about? Currently it looks like this addresses all the feedback from the previous PR and closes the Data Sources issue. However i see in here that S3 and Spark data sources are still hard coded and the registration function is still not generic enough to handle future data sources.

Thanks for the review.

The removal of hardcoding in this PR is to remove previous hardcoded type-to-display-name mapping and instead reading from data source metadata. The s3 data sources are backend data sources where it doesn't have any UI specific definitions. Ideally these front end specific definitions should be retrieving from settings or front end data sources separated from backend data sources where user can configure them. Does this align with the direction you are pointing to?

As for registration function, are you point to the registerDataSources in data source service or the way data source owner register the data source?

I think @ashwin-pc is saying the next iteration of this where if I am a cluster admin, I can install a plugin that adds the new data source. For example, if I want a S3 connection type I install a S3 connection plugin that wires that up. Or better yet, some type of catalogue.

@mengweieric
Copy link
Collaborator Author

Can you outline in the PR description what exactly this change is about? Currently it looks like this addresses all the feedback from the previous PR and closes the Data Sources issue. However i see in here that S3 and Spark data sources are still hard coded and the registration function is still not generic enough to handle future data sources.

Thanks for the review.
The removal of hardcoding in this PR is to remove previous hardcoded type-to-display-name mapping and instead reading from data source metadata. The s3 data sources are backend data sources where it doesn't have any UI specific definitions. Ideally these front end specific definitions should be retrieving from settings or front end data sources separated from backend data sources where user can configure them. Does this align with the direction you are pointing to?
As for registration function, are you point to the registerDataSources in data source service or the way data source owner register the data source?

I think @ashwin-pc is saying the next iteration of this where if I am a cluster admin, I can install a plugin that adds the new data source. For example, if I want a S3 connection type I install a S3 connection plugin that wires that up. Or better yet, some type of catalogue.

Yea understood that we will want this in future iterations, this PR addressed the step one mapping (updated the description), eventually we will not want those to be defined in code.

@ashwin-pc ashwin-pc merged commit 42166d0 into opensearch-project:main Apr 30, 2024
75 of 76 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 30, 2024
* datasource and service refactoring

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

* datasource enhancement and refactoring

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

* datasource selectable consolidation and refactoring

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

* add in memory cache with refresh

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

* move refresh to right side

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

* renaming

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

* update default datasource tests

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

* added more tests for default datasource

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

* update selector tests

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

* update changelog

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

* fix data source service tests

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

* add and update tests for datasource service

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

* add more data source service tests

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

* fix sidebar tests

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

* add to change log yml

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

* address comments along with more tests

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

* add test subject

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

* reference from correct type path

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

* correct text

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

* minor change - remove yet used displayOrder

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

* remove from changelog as having fragments already

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

* use expanded name

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

* fix one test

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

---------

Signed-off-by: Eric <[email protected]>
Signed-off-by: Eric Wei <[email protected]>
(cherry picked from commit 42166d0)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@ashwin-pc
Copy link
Member

@mengweieric yes my goal is to have no service specific names hardcoded in the framework. If any other company wanted to extend the datasources to support their version of the service like Azure for example. They should not need to make a code change in OSD to show up as a new Datasource as long as they adhere to the schema.

e.g.

registerDataSource({
  name: 'Postgress`
  schema: any
  ...
})

today, if i wanted to add postgress or any other datasource type to this list, i will need to make a code change in OSD to show up as an option in the DataSources menu. That should not be necessary. That should all be abstracted away in the API.

kavilla pushed a commit that referenced this pull request Apr 30, 2024
* datasource and service refactoring



* datasource enhancement and refactoring



* datasource selectable consolidation and refactoring



* add in memory cache with refresh



* move refresh to right side



* renaming



* update default datasource tests



* added more tests for default datasource



* update selector tests



* update changelog



* fix data source service tests



* add and update tests for datasource service



* add more data source service tests



* fix sidebar tests



* add to change log yml



* address comments along with more tests



* add test subject



* reference from correct type path



* correct text



* minor change - remove yet used displayOrder



* remove from changelog as having fragments already



* use expanded name



* fix one test



---------



(cherry picked from commit 42166d0)

Signed-off-by: Eric <[email protected]>
Signed-off-by: Eric Wei <[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>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 30, 2024
* datasource and service refactoring

* datasource enhancement and refactoring

* datasource selectable consolidation and refactoring

* add in memory cache with refresh

* move refresh to right side

* renaming

* update default datasource tests

* added more tests for default datasource

* update selector tests

* update changelog

* fix data source service tests

* add and update tests for datasource service

* add more data source service tests

* fix sidebar tests

* add to change log yml

* address comments along with more tests

* add test subject

* reference from correct type path

* correct text

* minor change - remove yet used displayOrder

* remove from changelog as having fragments already

* use expanded name

* fix one test

---------

(cherry picked from commit 42166d0)

Signed-off-by: Eric <[email protected]>
Signed-off-by: Eric Wei <[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>
(cherry picked from commit 4d14b60)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ananzh pushed a commit that referenced this pull request Apr 30, 2024
* datasource and service refactoring

* datasource enhancement and refactoring

* datasource selectable consolidation and refactoring

* add in memory cache with refresh

* move refresh to right side

* renaming

* update default datasource tests

* added more tests for default datasource

* update selector tests

* update changelog

* fix data source service tests

* add and update tests for datasource service

* add more data source service tests

* fix sidebar tests

* add to change log yml

* address comments along with more tests

* add test subject

* reference from correct type path

* correct text

* minor change - remove yet used displayOrder

* remove from changelog as having fragments already

* use expanded name

* fix one test

---------

(cherry picked from commit 42166d0)





(cherry picked from commit 4d14b60)

Signed-off-by: Eric <[email protected]>
Signed-off-by: Eric Wei <[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>
BionIT pushed a commit to BionIT/OpenSearch-Dashboards that referenced this pull request May 3, 2024
…earch-project#6691) (opensearch-project#6692)

* datasource and service refactoring

* datasource enhancement and refactoring

* datasource selectable consolidation and refactoring

* add in memory cache with refresh

* move refresh to right side

* renaming

* update default datasource tests

* added more tests for default datasource

* update selector tests

* update changelog

* fix data source service tests

* add and update tests for datasource service

* add more data source service tests

* fix sidebar tests

* add to change log yml

* address comments along with more tests

* add test subject

* reference from correct type path

* correct text

* minor change - remove yet used displayOrder

* remove from changelog as having fragments already

* use expanded name

* fix one test

---------

(cherry picked from commit 42166d0)





(cherry picked from commit 4d14b60)

Signed-off-by: Eric <[email protected]>
Signed-off-by: Eric Wei <[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>
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 3, 2024
* datasource and service refactoring

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

* datasource enhancement and refactoring

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

* datasource selectable consolidation and refactoring

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

* add in memory cache with refresh

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

* move refresh to right side

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

* renaming

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

* update default datasource tests

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

* added more tests for default datasource

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

* update selector tests

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

* update changelog

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

* fix data source service tests

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

* add and update tests for datasource service

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

* add more data source service tests

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

* fix sidebar tests

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

* add to change log yml

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

* address comments along with more tests

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

* add test subject

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

* reference from correct type path

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

* correct text

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

* minor change - remove yet used displayOrder

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

* remove from changelog as having fragments already

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

* use expanded name

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

* fix one test

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

---------

Signed-off-by: Eric <[email protected]>
Signed-off-by: Eric Wei <[email protected]>
@zhyuanqi zhyuanqi added enhancement New feature or request refactor Tech debt related tasks that need refactoring bug Something isn't working and removed bug Something isn't working enhancement New feature or request labels Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x data explorer Issues related to the Data Explorer project refactor Tech debt related tasks that need refactoring repeat-contributor v2.14.0
Projects
None yet
6 participants