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

Add data source service #191

Merged
merged 8 commits into from
May 29, 2024

Conversation

wanglam
Copy link
Collaborator

@wanglam wanglam commented May 27, 2024

Description

This PR is a part of multi data source support, add a new data source service to handle all the data source update logic.
This data source service will implement below functions:
1.Provide a setDataSourceId in setup and start return values. So other plugins can change data source id by this function.
2.Provide a init method, so chat bot can init data source id when dialog open. This init data source id function will check if multi data source enabled. It will try to get selection data source id from dsm or default data source from ui settings.
3.Provide a clearDataSourceId method, it reset data source id to null. It will be called when chatbot dialog been closed.
4.Provide a subscribeDataSourceId method, so chatbot can observe this subject to interact with different data source.
5.Provide a getDataSourceQuery method, so chatbot can send API request with this data source id query object. It will handle the local cluster automaticlly.

Issues Resolved

A part of #192

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test.
  • New functionality has user manual doc added.
  • Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Lin Wang <wonglam@amazon.com>
@wanglam wanglam added backport 2.x Trigger the backport flow to 2.x Skip-Changelog labels May 27, 2024
@wanglam wanglam marked this pull request as ready for review May 27, 2024 10:03
Signed-off-by: Lin Wang <wonglam@amazon.com>
@@ -131,6 +140,7 @@ export class AssistantPlugin
}

return {
...dataSourceSetupResult,
Copy link
Member

@SuZhou-Joe SuZhou-Joe May 28, 2024

Choose a reason for hiding this comment

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

Suggested change
...dataSourceSetupResult,
dataSourceService: dataSourceSetupResult

Is there any place that will use dataSourceSetupResult? And if so, can we make it a single entry to keep the export interface clean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about just export a setDataSourceId method? For now, this method not been used in any place.

Copy link
Member

Choose a reason for hiding this comment

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

Agree that.

@@ -160,8 +170,12 @@ export class AssistantPlugin
setChrome(core.chrome);
setNotifications(core.notifications);

return {};
return {
...this.dataSourceService.start(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
...this.dataSourceService.start(),
dataSourceServiceStart: this.dataSourceService.start(),

Same here, I'd vote for a single clear entry.

Customized,
}

const getDSMDataSourceSelectionOption = (dataSourceSelection: Map<string, DataSourceOption[]>) => {
Copy link
Member

Choose a reason for hiding this comment

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

The function name seems not comply with what it is doing. What about getSingleSelectedDataSourceOption?

Comment on lines 104 to 108
this.uiSettings.get$('defaultDataSource', null).subscribe((newDataSourceId) => {
if (this.dataSourceIdFrom === DataSourceIdFrom.UiSettings) {
this.setDataSourceId(newDataSourceId, DataSourceIdFrom.UiSettings);
}
});
Copy link
Member

@SuZhou-Joe SuZhou-Joe May 28, 2024

Choose a reason for hiding this comment

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

Do we need to be aware of the defaultDataSource? I thought it should be transparent to plugins and it looks like a logic coupling.

What about we initialize a hidden data source selector if none picker is mounted to the page? And that picker will help to tell us what the defaultDataSource is.

Copy link
Collaborator Author

@wanglam wanglam May 28, 2024

Choose a reason for hiding this comment

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

This's a good idea. But there are three limitations after change to a transparent data source picker:

  1. onSelectedDataSource in data source picker need to wait all data source fetched, it will take more time to determine the default data source.
  2. The onSelectedDataSource won't be fired after default data source changed, the data source id in data source service will still use the old one.
  3. Since we are depend on data source selection, the transparent data source picker will add selection value to the data source selection. We need to add some logic to filter out data source selection from this component. @raintygao may need to modify the OSD PR.
    I can change to use a transparent data source picker if 2 & 3 have been addressed or skipped. But I think it would be better if we can add some defaultDataSource functions to subscribe the default data source changes in DSM plugin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

DSM hasn't provided a way to get default data source. As it's stored in ui settings and DSM also gets this value from uisettings, so i think we don't need to depend on DSM to get this additionally. Im open to be aware of defaultDataSource in external plugin in this stage.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we add an interface getDefaultDataSource in DSM? I just do not feel it reasonable that we know the implementation detail of the default data source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. @raintygao already raised a PR to add these methods in the DSM plugin. Let's keep current uiSettings implementation. Will raised another PR to fix this default data source after @raintygao 's PR merged.

Signed-off-by: Lin Wang <wonglam@amazon.com>

this.uiSettings.get$('defaultDataSource', null).subscribe((newDataSourceId) => {
if (this.dataSourceIdFrom === DataSourceIdFrom.UiSettings) {
this.setDataSourceId(newDataSourceId, DataSourceIdFrom.UiSettings);
Copy link
Member

Choose a reason for hiding this comment

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

Q: why do we need to subscribe to the change of defaultDataSource? My understanding is this.setDataSourceId should be setting the current selected data source, it should not take defaultDataSource into consideration.

I guess what we are trying to implement here is when this.dataSourceId$ is empty, assistant should use defaultDataSource, maybe we should incorporate this logic in getDataSourceId and subscribeDataSourceId?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default data source can be changed in the data source management application. Add these code here to update the data source id if default data source changed if it's set by default data source.


constructor() {}

initDefaultDataSourceIdIfNeed() {
Copy link
Member

Choose a reason for hiding this comment

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

When will this function to be called?

Copy link
Member

Choose a reason for hiding this comment

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

so chat bot can init data source id when dialog open.

Found in the PR description, but do chatbot really need to call initDefaultDataSourceIdIfNeed every time it's opened? I feel it more intuitive if simply subscribeDataSourceId or getDataSourceId can do it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The motivation of init everytime when chatbot open is get latest default data source id. Since the default data source id may changed in another tabs, add the init code to chatbot open to avoid using the expired default data source id.

Signed-off-by: Lin Wang <wonglam@amazon.com>
Copy link

codecov bot commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.30%. Comparing base (3991de2) to head (1249852).
Report is 1 commits behind head on main.

Current head 1249852 differs from pull request most recent head 9caec03

Please upload reports for the commit 9caec03 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
+ Coverage   90.02%   90.30%   +0.28%     
==========================================
  Files          54       55       +1     
  Lines        1433     1496      +63     
  Branches      347      359      +12     
==========================================
+ Hits         1290     1351      +61     
- Misses        141      143       +2     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Lin Wang <wonglam@amazon.com>
Copy link
Member

@ruanyl ruanyl left a comment

Choose a reason for hiding this comment

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

I'm trying to describe what I'm thinking via suggested changes, maybe it's more straight-forward to just look at the code.

The key points are:

  1. use dataSourceId$ to store computed value from data source selector
  2. derive the data source to be used in chatbot via getDataSourceId$()
    And chatbot can simply subscribe to getDataSourceId$() to get which data source to use. Does it make sense?

Comment on lines 47 to 66
init() {
if (!this.isMDSEnabled()) {
return;
}
const dataSourceSelectionMap = this.dataSourceManagement?.dataSourceSelection
.getSelection$()
.getValue();
const dataSourceSelectionOption = dataSourceSelectionMap
? getSingleSelectedDataSourceOption(dataSourceSelectionMap)
: null;
if (dataSourceSelectionOption) {
this.setDataSourceId(dataSourceSelectionOption.id, DataSourceIdFrom.DataSourceSelection);
return;
}
const defaultDataSourceId = this.uiSettings?.get('defaultDataSource', null);
if (!defaultDataSourceId) {
return;
}
this.setDataSourceId(defaultDataSourceId, DataSourceIdFrom.UiSettings);
}
Copy link
Member

@ruanyl ruanyl May 28, 2024

Choose a reason for hiding this comment

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

Suggested change
init() {
if (!this.isMDSEnabled()) {
return;
}
const dataSourceSelectionMap = this.dataSourceManagement?.dataSourceSelection
.getSelection$()
.getValue();
const dataSourceSelectionOption = dataSourceSelectionMap
? getSingleSelectedDataSourceOption(dataSourceSelectionMap)
: null;
if (dataSourceSelectionOption) {
this.setDataSourceId(dataSourceSelectionOption.id, DataSourceIdFrom.DataSourceSelection);
return;
}
const defaultDataSourceId = this.uiSettings?.get('defaultDataSource', null);
if (!defaultDataSourceId) {
return;
}
this.setDataSourceId(defaultDataSourceId, DataSourceIdFrom.UiSettings);
}
init() {
if (!this.isMDSEnabled()) {
return;
}
if (!this.dataSourceManagement) {
return;
}
this.dataSourceManagement.dataSourceSelection
.getSelection$()
.pipe(map(getSingleSelectedDataSourceOption))
.subscribe((v) => v ? this.dataSourceId$.next(v.id) : this.dataSourceId$.next(null));
}

Comment on lines 98 to 104
getDataSourceId() {
return this.dataSourceId$.getValue();
}

subscribeDataSourceId(observer?: PartialObserver<string | null>) {
return this.dataSourceId$.subscribe(observer);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
getDataSourceId() {
return this.dataSourceId$.getValue();
}
subscribeDataSourceId(observer?: PartialObserver<string | null>) {
return this.dataSourceId$.subscribe(observer);
}
getDataSourceId$() {
return combineLatest([
this.dataSourceId$,
this.uiSettings?.get$('defaultDataSource', '') ?? of(''),
]).pipe(
map(([selectedDataSourceId, defaultDataSourceId]) => {
if (selectedDataSourceId) {
return selectedDataSourceId;
}
return defaultDataSourceId;
})
);
}

Comment on lines 116 to 126
this.dataSourceSelectionSubscription = this.dataSourceManagement?.dataSourceSelection
?.getSelection$()
.subscribe(() => {
this.init();
});

this.uiSettings.get$('defaultDataSource', null).subscribe((newDataSourceId) => {
if (this.dataSourceIdFrom === DataSourceIdFrom.UiSettings) {
this.setDataSourceId(newDataSourceId, DataSourceIdFrom.UiSettings);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.dataSourceSelectionSubscription = this.dataSourceManagement?.dataSourceSelection
?.getSelection$()
.subscribe(() => {
this.init();
});
this.uiSettings.get$('defaultDataSource', null).subscribe((newDataSourceId) => {
if (this.dataSourceIdFrom === DataSourceIdFrom.UiSettings) {
this.setDataSourceId(newDataSourceId, DataSourceIdFrom.UiSettings);
}
});
this.init()

Signed-off-by: Lin Wang <wonglam@amazon.com>
@wanglam
Copy link
Collaborator Author

wanglam commented May 28, 2024

Hi @ruanyl , thanks for your great suggestions. I've updated my code based them.
There are some code changes in my implementation:
This line is for allowing local cluster be passed: https://github.com/opensearch-project/dashboards-assistant/pull/191/files#diff-3d472851e03fbf6c2619534f8dcb63d0d742f398d092846188bd5f624bc8d094R89

This line is for allowing local cluster be passed: https://github.com/opensearch-project/dashboards-assistant/pull/191/files#diff-3d472851e03fbf6c2619534f8dcb63d0d742f398d092846188bd5f624bc8d094R89
This line is for avoid same data source id fire data source id change: https://github.com/opensearch-project/dashboards-assistant/pull/191/files#diff-3d472851e03fbf6c2619534f8dcb63d0d742f398d092846188bd5f624bc8d094R53

Feel free to help me review it. Thank you.

Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
@wanglam
Copy link
Collaborator Author

wanglam commented May 29, 2024

@ruanyl @raintygao I've update the dataSourceSelection to optional in DataSourceManagementPluginSetup. It's a minor change. Feel free to help me take a look. Thank you.

@wanglam wanglam merged commit c2eab86 into opensearch-project:main May 29, 2024
9 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 29, 2024
* Add data source service

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Add changelog for data source service

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Rename to getSingleSelectedDataSourceOption

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Update data source init logic

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Move setup and start result under dataSource

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Refactor data source with getDataSourceId$

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Return default data source for multi and empty data selection

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Update dataSourceSelection to optional

Signed-off-by: Lin Wang <wonglam@amazon.com>

---------

Signed-off-by: Lin Wang <wonglam@amazon.com>
(cherry picked from commit c2eab86)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
wanglam pushed a commit that referenced this pull request Jun 3, 2024
* Add data source service

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Add changelog for data source service

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Rename to getSingleSelectedDataSourceOption

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Update data source init logic

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Move setup and start result under dataSource

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Refactor data source with getDataSourceId$

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Return default data source for multi and empty data selection

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Update dataSourceSelection to optional

Signed-off-by: Lin Wang <wonglam@amazon.com>

---------

Signed-off-by: Lin Wang <wonglam@amazon.com>
(cherry picked from commit c2eab86)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Trigger the backport flow to 2.x v2.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants