Support MDS on ListDetectors page #716
Conversation
Signed-off-by: Jackie Han <[email protected]>
Signed-off-by: Jackie Han <[email protected]>
Signed-off-by: Jackie Han <[email protected]>
jackiehanyang
requested review from
ohltyler,
kaituo,
amitgalitz,
sean-zheng-amazon,
dbwiddis,
owaiskazi19 and
joshpalis
as code owners
Signed-off-by: Jackie Han <[email protected]>
server/plugin.ts
Outdated
|
|
||
| export interface ADPluginSetupDependencies { | ||
| dataSourceManagement: ReturnType<DataSourceManagementPlugin['setup']>; | ||
| dataSource: DataSourcePluginSetup; |
There was a problem hiding this comment.
If dataSourceManagement and dataSource are optionalPlugins, these shouldn't be required.
There was a problem hiding this comment.
will update this part
There was a problem hiding this comment.
updated in the latest commit
Signed-off-by: Jackie Han <[email protected]>
Signed-off-by: Jackie Han <[email protected]>
|
Can we add some context and manual tests have been done for the PR? |
|
Approved but a couple of action items once this is merged:
|
This pr just add the data source component on the list page and render the page according to data source selector. I will provide a record to include this page on the other pr I opened |
noted |
| @@ -89,6 +89,7 @@ export type GetDetectorsQueryParams = { | |||
| indices?: string; | |||
| sortDirection: SORT_DIRECTION; | |||
| sortField: string; | |||
| dataSourceId?: string; | |||
There was a problem hiding this comment.
it isn't scalable or consistent to add this ID as part of this particular interface that's used just for the getDetectors() server-side call. We'll need it for all server-side fns, including all of the AD ones, alerting ones, and general cluster ones. Can you explore a common pattern that could be added to all routes, for example, maybe appending or prepending the data source ID in the path itself and parsing out similar to how detectorID is done for some of them is a possibility. All plugins will have the same problem of how to propagate the data source ID from public -> server-side, is there a common pattern being done by others?
There was a problem hiding this comment.
I'm adding the dataSourceId as port of this particular interface just for this list page. Because for list page, it uses a specific interface for its query params. Since dataSourceId also serves as a query param on this page, so I'm just adding it to this interface. For other pages, I'm appending the data source id the path itself. Please check the pr for detailed page - https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/pull/719/files#diff-cc594889ac44cb130233969adf3da57a18f360a2a77c57ee273aad5f39d6d733
There was a problem hiding this comment.
Understood, and cool, looks like some are adjusted that way there. Let's make all of them consistent through the node path the first time though. This will make it much cleaner and avoid making changes to this data model which doesn't logically fit.
|
Also, let's make sure unit tests are passing. +1 to @huyaboo comment as well, maybe we can have a meta issue to make sure we have 1/ updated unit tests and 2/ a few integ tests for sanity testing. @huyaboo / @BionIT is there integ test framework support for mocking multiple data sources and invoking multiple cluster clients? |
great suggestion. meta issue created - #721 |
Description
This pr adds MDS support on ListDetectors page. The default data source will be selected when MDS support is enabled.
Issues Resolved
[List any issues this PR will resolve]
Check List
--signoffBy 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.