fix error that local cluster cannot get version #606

kamahuan · 2025-02-07T08:17:44Z

Description

Currently in the new work flow page, when user click local data source, it shows a loading state and does not render any presets. This PR fix this issue by getting version from the datasource and render the presets based on the data source version.

Issues Resolved

None

UI changes after code change

for version 2.19

for version 2.17

Check List

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.

ohltyler

If adding a new API, make sure it is integrated into the rest of the redux store and our route service framework. For example, see #330 which onboards the search connectors API.

We don't want to make direct calls from client-side, as it can be unstable, cause issues with tests, doesn't integrate with our error handling propagation, and could lead to NPEs.

ohltyler · 2025-02-07T18:46:25Z

server/routes/opensearch_routes_service.ts

+  router.get(
+    {
+      path: `${BASE_NODE_API_PATH}/version`,
+      validate: {
+        params: schema.object({
+          pattern: schema.string(),
+        }),
+      },
+    },
+    opensearchRoutesService.catIndices
+  );


This route is not being used. If adding a server-side call, let's properly integrate with the redux store for consistency.

Yes this part is not used. Has removed the unused code.

ohltyler · 2025-02-07T18:47:04Z

server/routes/opensearch_routes_service.ts

+      const response = await callWithRequest('cat.indices', {
+        index: pattern,
+        format: 'json',
+      });
+
+      // re-formatting the index results to match Index
+      const cleanedIndices = response.map((index: any) => ({
+        name: index.index,
+        health: index.health,
+      })) as Index[];
+
+      return res.ok({ body: cleanedIndices });
+    } catch (err: any) {
+      return generateCustomError(res, err);
+    }
+  };


I'm confused about this call - why is cat indices needed?

removed this part.

ohltyler · 2025-02-07T18:47:44Z

public/pages/workflows/new_workflow/new_workflow.tsx

@@ -71,7 +78,7 @@ const filterPresetsByVersion = async (
    return workflows;
  }

-  if (!dataSourceId) {
+  if (dataSourceId === undefined) {


I remember some discrepancy on how empty string vs. undefined is handled for data sources. @saimedhi please confirm if this change makes sense.

For the scenario of local cluster, data source id would be an empty string ''. !dataSourceId would be true here so it will return empty presets for local cluster.

This is not the behavior we want so I changed the condition to only return empty if data source id is undefined.

@kamahuan , I agree with your change.

When local cluster is used, dataSourceId = ''

When no compatible datasource is used, dataSourceId is undefined. Implies dataSourceId not present in page Url.

Thank you for fixing the code added in "filter presets and processors based on backend version" PR.

Lets connect and test all the possible scenarios before merging this PR.

can we check the mds feature flag to determine if mds is enabled instead of checking if dataSourceId is present? Checking the feature flag should be the best practice and less error-prone

something like this - something like this - https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/blob/main/public/pages/DetectorsList/containers/List/List.tsx#L147

Hi Jackie, I am not sure if I completely understand your comment. Line 76 and 77 checks if MDS is disabled, if so, we are showing all the presets assuming version >= 2.19. The other 2 cases checks 2 difference scenarios:

if datasourceId === '', that means we are using local clusters

if datasourceId === 'undefined', that means no compatible data source is used.
Are you suggesting removing line 81 to check the scenario where datasource is enabled? I am unsure semantically, if enabling datasource consists of the 2 above scenarios. May I know if @ohltyler @saimedhi have any ideas about this.

Line 76 and 77 checks if MDS is disabled

Don't use if(dataSourceId === undefined) to check if MDS is disabled, read the feature flag from config file and check the feature flag to determine if MDS is enabled. Something like this if(MDSEnabled) - https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/blob/main/public/services.ts#L65

ohltyler · 2025-02-07T18:48:42Z

public/pages/workflows/new_workflow/new_workflow.tsx

+    if (dataSourceId === '') {
+      const response = await getCore().http.post('/api/console/proxy', {
+        query: { path: '/', method: 'GET', dataSourceId: '' },
+      });
+      return response.version.number;
+    }
+


Let's integrate this properly into redux and our server-side route services.

Done. Add check local cluster version check into route.service file and redux into presetRedux file

saimedhi · 2025-02-10T14:46:16Z

public/pages/workflows/new_workflow/new_workflow.tsx

+    //     query: { path: '/', method: 'GET', dataSourceId: '' },
+    //   });
+    //   return response.version.number;
+    // }


@kamahuan , could you remove this commented lines.

saimedhi · 2025-02-10T14:52:13Z

public/pages/workflows/new_workflow/new_workflow.tsx

    (state: AppState) => state.presets
  );
  const [allWorkflows, setAllWorkflows] = useState<WorkflowTemplate[]>([]);
  const [filteredWorkflows, setFilteredWorkflows] = useState<
    WorkflowTemplate[]
  >([]);
-  const [isVersionLoading, setIsVersionLoading] = useState(false);
+  // const [isVersionLoading, setIsVersionLoading] = useState(false);


Please remove commented lines.

saimedhi

@kamahuan, please clean up the commented code. Thank you for the fixes.

ohltyler · 2025-02-10T16:33:07Z

public/store/reducers/presets_reducer.ts

In general, I'm ok with the change, but it should go to a more logical reducer - suggest to move to opensearch_reducer. Also, after moving, suggest to reuse the existing loading state var there; a new one shouldn't be needed.

public/pages/workflows/new_workflow/new_workflow.tsx

public/store/reducers/presets_reducer.ts

ohltyler

LGTM - please resolve the merge conflicts, and work with Sai to ensure everything is tested and working as expected. Thanks!

Signed-off-by: Kama Huang <kamahuan@amazon.com> refactor get local cluster version logic into service route and redux Signed-off-by: Kama Huang <kamahuan@amazon.com> refactor getVersion logic into opensearch reducer Signed-off-by: Kama Huang <kamahuan@amazon.com> adding loading state back to presetReducer Signed-off-by: Kama Huang <kamahuan@amazon.com> fixing unit tests Signed-off-by: Kama Huang <kamahuan@amazon.com>

public/pages/workflows/new_workflow/new_workflow.tsx

Signed-off-by: Kama Huang <kamahuan@amazon.com>

Signed-off-by: Kama Huang <kamahuan@amazon.com> (cherry picked from commit 39ed7aa) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

(cherry picked from commit 39ed7aa) Signed-off-by: Kama Huang <kamahuan@amazon.com> 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>

kamahuan requested review from dbwiddis, owaiskazi19, joshpalis, ohltyler, amitgalitz, jackiehanyang, minalsha and saimedhi as code owners February 7, 2025 08:17

opensearch-trigger-bot bot added the backport 2.x label Feb 7, 2025

ohltyler requested changes Feb 7, 2025

View reviewed changes

saimedhi reviewed Feb 10, 2025

View reviewed changes

ohltyler requested changes Feb 10, 2025

View reviewed changes

kamahuan force-pushed the feature1 branch 3 times, most recently from 17cac7e to 96dffc0 Compare February 10, 2025 19:56

ohltyler requested changes Feb 10, 2025

View reviewed changes

public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved

public/store/reducers/presets_reducer.ts Outdated Show resolved Hide resolved

ohltyler reviewed Feb 11, 2025

View reviewed changes

kamahuan force-pushed the feature1 branch from d5d8a34 to 21910ac Compare February 11, 2025 19:31

saimedhi approved these changes Feb 13, 2025

View reviewed changes

ohltyler approved these changes Feb 13, 2025

View reviewed changes

kamahuan force-pushed the feature1 branch from 21910ac to b4b691e Compare February 17, 2025 02:58

ohltyler requested changes Feb 17, 2025

View reviewed changes

public/pages/workflows/new_workflow/new_workflow.tsx Show resolved Hide resolved

adding comments about fetching local cluster version

77561b3

Signed-off-by: Kama Huang <kamahuan@amazon.com>

ohltyler approved these changes Feb 17, 2025

View reviewed changes

ohltyler merged commit 39ed7aa into opensearch-project:main Feb 17, 2025
7 checks passed

opensearch-trigger-bot bot mentioned this pull request Feb 17, 2025

[Backport 2.x] fix error that local cluster cannot get version #623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix error that local cluster cannot get version #606

fix error that local cluster cannot get version #606

kamahuan commented Feb 7, 2025

ohltyler left a comment

ohltyler Feb 7, 2025

kamahuan Feb 10, 2025

ohltyler Feb 7, 2025

kamahuan Feb 10, 2025

ohltyler Feb 7, 2025

kamahuan Feb 10, 2025

saimedhi Feb 10, 2025 •

edited

Loading

saimedhi Feb 10, 2025

jackiehanyang Feb 10, 2025

kamahuan Feb 11, 2025

jackiehanyang Feb 12, 2025

ohltyler Feb 7, 2025

kamahuan Feb 10, 2025

saimedhi Feb 10, 2025

saimedhi Feb 10, 2025

kamahuan Feb 10, 2025

saimedhi left a comment

ohltyler Feb 10, 2025

ohltyler left a comment

fix error that local cluster cannot get version #606

fix error that local cluster cannot get version #606

Conversation

kamahuan commented Feb 7, 2025

Description

Issues Resolved

UI changes after code change

for version 2.19

for version 2.17

Check List

ohltyler 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

saimedhi Feb 10, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saimedhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler left a comment

Choose a reason for hiding this comment

saimedhi Feb 10, 2025 •

edited

Loading