Support MDS on DetectorDetails page #719

jackiehanyang · 2024-04-07T15:58:38Z

Description

This PR adds MDS support to the DetectorDetails page. The page's functionality and appearance remain the same, with the exception of the following changes:

a dataSourceId will be appended to the url for both /results, /historical, and /configurations tab
A read-only data source view will be rendered in the top right corner. Currently, it displays a placeholder label as the Neo team is implementing the feature to allow the data source view to take only a dataSourceId instead of requiring both an id and a label.

Please note:

This PR branch was created from the neoListPage branch, which has another open PR. We will aim to merge that PR first to provide a clearer view for this PR.
Efforts were made to decouple page interactions to allow for incremental PRs. However, hard-coded values were used in some places to ensure functionality. These hard-coded values will be removed in future updates when working on adding MDS support on those specific pages.

OpenSearch.Dashboards.-.8.April.2024.mp4

-fbe5-4621-97dd-2a5b663cb2bf)

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
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: Jackie Han <[email protected]>

ohltyler · 2024-04-09T00:32:25Z

server/routes/ad.ts

Can we consistently have this set for all routes as part of a single change? That way we can ensure no APIs are missed. Same goes for alerting and opensearch routes pages. If it causes problems (rendering a non-integrated page causes crashes), then can you help open a tracking issue to include in a MDS meta issue that all routes are covered and tested?

I was planning to update all the routes in a single change, but then I found that it created barriers when testing my change. Some pages and calls were crashing, which prevented me from accessing certain pages and tabs. As a result, I switched to updating the routes on a per-page basis.

Opened this meta issue that can help track every pages/redux store/route updates - #721 Will do a sanity test/bug bash after code complete as well.

ohltyler · 2024-04-09T00:34:45Z

However, hard-coded values were used in some places to ensure functionality. These hard-coded values will be removed in future updates when working on adding MDS support on those specific pages.

Same goes for this. Please add details in tracking issues for all workarounds / hardcoded values / etc. so nothing is missed in the end. Especially since this is being committed directly to main and no feature branch.

Signed-off-by: Jackie Han <[email protected]>

jackiehanyang · 2024-04-09T17:11:43Z

However, hard-coded values were used in some places to ensure functionality. These hard-coded values will be removed in future updates when working on adding MDS support on those specific pages.

Same goes for this. Please add details in tracking issues for all workarounds / hardcoded values / etc. so nothing is missed in the end. Especially since this is being committed directly to main and no feature branch.

Added a section in the meta issue to track hardcoded value that was added while sending out incremental prs - #721

Signed-off-by: Jackie Han <[email protected]>

huyaboo

A couple overall comments:

+1 to @ohltyler comment. We have to ensure that all relevant changes to the routes are consistent and functioning.
For a lot of the routes, dataSourceId is part of the route but how are Local Cluster cases handled? Double slashes // should be avoided. In the same vein, we should ensure that dataSourceId is handled consistently. In some cases dataSourceId is an optional field whereas in others it is required.

huyaboo · 2024-04-09T20:54:50Z

public/pages/CreateDetectorSteps/hooks/useFetchDetectorInfo.ts

@@ -43,7 +43,8 @@ export const useFetchDetectorInfo = (
  useEffect(() => {
    const fetchDetector = async () => {
      if (!detector) {
-        await dispatch(getDetector(detectorId));
+        // hardcoding the datasource id for now, will update it later when working on create page


Can we have a tracker for all the TODOs so we don't forget what needs to be done?

tracked here - #721

huyaboo · 2024-04-09T20:58:23Z

public/pages/DetectorDetail/containers/DetectorDetail.tsx

+            setMenuMountPoint={props.setActionMenu}
+            componentType={'DataSourceView'}
+            componentConfig={{
+              activeOption: [{ id: dataSourceId}],


Non-blocking: Can label be inserted here as well if the context/parent props has found a label earlier? It's not required but it can help with reducing network calls for DataSourceMenu component.

huyaboo · 2024-04-09T21:33:35Z

public/pages/DetectorsList/containers/List/List.tsx

@@ -552,6 +574,16 @@ export const DetectorList = (props: ListProps) => {
    });
  };

+  const handleDataSourceChange = (e) => {
+    const dataConnectionId = e[0] ? e[0].id : undefined;


If dataConnectionId = undefined, this is the invalid state returned from the component. Could you please handle this as an error state before calling setState()? If an undefined data source id is propagated, there is a chance of unpredictable behavior happening.

yes I had this change in the list page pr, too many open prs.... I will include that change here

huyaboo · 2024-04-09T21:39:31Z

public/pages/main/Main.tsx

+                    <DetectorList
+                      dataSourceEnabled={dataSourceEnabled}
+                      setActionMenu={setHeaderActionMenu}
+                      {...props}


Since props already contains dataSourceEnabled and setHeaderActionMenu within it as required args, I think {...props} is sufficient.

I don't think props already contains dataSourceEnabled and setHeaderActionMenu?

huyaboo · 2024-04-09T21:41:32Z

public/pages/main/Main.tsx

+                    <DetectorDetail 
+                      dataSourceEnabled={dataSourceEnabled}
+                      setActionMenu={setHeaderActionMenu}
+                      {...props} />


Same deal as the DetectorList. {...props} should be sufficient.

same reply as above. I don't think props has these two things in it. We still need to pass them in

huyaboo · 2024-04-09T21:54:22Z

public/redux/reducers/ad.ts

  type: START_DETECTOR,
  request: (client: HttpSetup) =>
-    client.post(`..${AD_NODE_API.DETECTOR}/${detectorId}/start`),
+    client.post(`..${AD_NODE_API.DETECTOR}/${detectorId}/${dataSourceId}/start`),


How are Local Cluster queries handled here? If Local Clusters are "", there is a possibility that the post request will contain the endpoint /detector-id//start

Provided a suggested change below:

Suggested change

client.post(`..${AD_NODE_API.DETECTOR}/${detectorId}/${dataSourceId}/start`),

client.post(`..${AD_NODE_API.DETECTOR}/${detectorId}${dataSourceId ? '/' + dataSourceId : ''}/start`),

I will make a change to define two separate routes. one includes dataSourceId, one doesn’t. This can make the API structure clearer and avoids ambiguity. Something like this

apiRouter.get('/detectors/{id}/results/{isHistorical}', adService.getAnomalyResultsWithoutDataSource); apiRouter.get('/detectors/{id}/{dataSourceId}/results/{isHistorical}', adService.getAnomalyResultsWithDataSource);

That example points to 2 different functions - can we make multiple routes point to the same fn and have shared logic at the top of each fn to parse the optional data source ID?

yes, I gave a bad example above. Here's an example of existing code that has multiple routes point to the same post function searchResults:

apiRouter.post('/detectors/_search', adService.searchDetector); apiRouter.post('/detectors/results/_search/', adService.searchResults); apiRouter.post('/detectors/results/{dataSourceId}/_search', adService.searchResults); apiRouter.post( '/detectors/results/{dataSourceId}/_search/{resultIndex}/{onlyQueryCustomResultIndex}', adService.searchResults );

Planning to parse out data source id like this

const { dataSourceId = "" } = request.params as { dataSourceId?: string };

So when it doesn't exist in the request, just assign empty value to it so it can fallback to local cluster client

public/redux/reducers/ad.ts

public/redux/reducers/anomalyResults.ts

BionIT · 2024-04-07T23:36:07Z

public/pages/CreateDetectorSteps/hooks/useFetchDetectorInfo.ts

@@ -43,7 +43,8 @@ export const useFetchDetectorInfo = (
  useEffect(() => {
    const fetchDetector = async () => {
      if (!detector) {
-        await dispatch(getDetector(detectorId));
+        // hardcoding the datasource id for now, will update it later when working on create page
+        await dispatch(getDetector(detectorId, '4585f560-d1ef-11ee-aa63-2181676cc573'));


Can we add a TODO in the comment so it is clear

BionIT · 2024-04-07T23:37:08Z

public/pages/DetectorDetail/containers/DetectorDetail.tsx

+
+  const location = useLocation();
+  const queryParams = new URLSearchParams(location.search);
+  const dataSourceId = queryParams.get('dataSourceId') as string;


does dataSourceId always exist in the query param?

BionIT · 2024-04-07T23:38:22Z

public/pages/DetectorDetail/containers/DetectorDetail.tsx

+            setMenuMountPoint={props.setActionMenu}
+            componentType={'DataSourceView'}
+            componentConfig={{
+              // give a placeholder label for now, will update it once neo team allows empty label field


Probably adding a TODO: give a place holder ... here so it is easier to find the location that needs rework

BionIT · 2024-04-07T23:39:33Z

public/pages/DetectorsList/containers/List/List.tsx

+
+    setState({
+      ...state,
+      page: 0,


what does page: 0 state do here?

BionIT · 2024-04-15T05:09:59Z

public/anomaly_detection_app.tsx

+export function renderApp(
+  coreStart: CoreStart,
+  params: AppMountParameters,
+  dataSource: DataSourcePluginSetup


looks like the dataSource object is not used below

BionIT · 2024-04-15T05:10:39Z

public/models/interfaces.ts

@@ -218,6 +219,7 @@ export type DetectorListItem = {
  lastUpdateTime: number;
  enabledTime?: number;
  detectorType?: string;
+  dataSourceId: string;


Is dataSourceId optional?

jackiehanyang added 6 commits February 14, 2024 14:12

update snapshots

d3de573

Signed-off-by: Jackie Han <[email protected]>

add snpshot

8258d5c

Signed-off-by: Jackie Han <[email protected]>

Merge branch 'opensearch-project:main' into main

c21d6c7

neo List Detectors page

e0333ec

Signed-off-by: Jackie Han <[email protected]>

add opensearch_dashboards.json file

8a94a9b

Signed-off-by: Jackie Han <[email protected]>

Support MDS on DetectorDetails page

10c730b

Signed-off-by: Jackie Han <[email protected]>

jackiehanyang requested a review from BionIT April 7, 2024 15:58

jackiehanyang requested review from ohltyler, kaituo, amitgalitz, sean-zheng-amazon, dbwiddis, owaiskazi19 and joshpalis as code owners April 7, 2024 15:58

opensearch-trigger-bot bot added the backport 2.x label Apr 7, 2024

update read-only data source view

6aab2f4

Signed-off-by: Jackie Han <[email protected]>

ohltyler reviewed Apr 9, 2024

View reviewed changes

create getter and setter for dataSourceManagement plugin

6d8bd5b

Signed-off-by: Jackie Han <[email protected]>

add type to context variable in server helper file

5057c07

Signed-off-by: Jackie Han <[email protected]>

jackiehanyang mentioned this pull request Apr 9, 2024

[META] MDS Support For AD #721

Closed

25 tasks

huyaboo reviewed Apr 9, 2024

View reviewed changes

jackiehanyang mentioned this pull request Apr 12, 2024

Support MDS on List, Detail, Dashboard, Overview pages #722

Merged

4 tasks

BionIT reviewed Apr 15, 2024

View reviewed changes

jackiehanyang closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MDS on DetectorDetails page #719

Support MDS on DetectorDetails page #719

jackiehanyang commented Apr 7, 2024 •

edited

Loading

ohltyler Apr 9, 2024 •

edited

Loading

jackiehanyang Apr 9, 2024

ohltyler commented Apr 9, 2024

jackiehanyang commented Apr 9, 2024

huyaboo left a comment

huyaboo Apr 9, 2024

jackiehanyang Apr 9, 2024

huyaboo Apr 9, 2024

huyaboo Apr 9, 2024

jackiehanyang Apr 9, 2024

huyaboo Apr 9, 2024 •

edited

Loading

jackiehanyang Apr 9, 2024

huyaboo Apr 9, 2024

jackiehanyang Apr 9, 2024

huyaboo Apr 9, 2024

jackiehanyang Apr 9, 2024

ohltyler Apr 9, 2024 •

edited

Loading

jackiehanyang Apr 10, 2024

BionIT Apr 7, 2024

BionIT Apr 7, 2024

BionIT Apr 7, 2024

BionIT Apr 7, 2024

BionIT Apr 15, 2024

BionIT Apr 15, 2024

	client.post(`..${AD_NODE_API.DETECTOR}/${detectorId}/${dataSourceId}/start`),
	client.post(`..${AD_NODE_API.DETECTOR}/${detectorId}${dataSourceId ? '/' + dataSourceId : ''}/start`),

Support MDS on DetectorDetails page #719

Support MDS on DetectorDetails page #719

Conversation

jackiehanyang commented Apr 7, 2024 • edited Loading

Description

Issues Resolved

Check List

ohltyler Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler commented Apr 9, 2024

jackiehanyang commented Apr 9, 2024

huyaboo 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

huyaboo Apr 9, 2024 • edited Loading

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

ohltyler Apr 9, 2024 • edited Loading

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

jackiehanyang commented Apr 7, 2024 •

edited

Loading

ohltyler Apr 9, 2024 •

edited

Loading

huyaboo Apr 9, 2024 •

edited

Loading

ohltyler Apr 9, 2024 •

edited

Loading