Add MDS to msearch #9361

wanglam · 2025-02-10T08:39:01Z

Description

This PR is for adding MDS for _internal/msearch. A data_source_id can be passed to the msearch API to determine which data source should be used to execute msearch with search requests.

Issues Resolved

#2174

Screenshot

No UI changes

Testing the changes

Clone branch code and run yarn osd bootstrap --single-version loose
Add below configs in config/opensearch_dashboards.yml

data_source.enabled: true
uiSettings:
  overrides:
    'courier:batchSearches': true

Run yarn start --no-base-path
Login with admin user
Visit /app/management/opensearch-dashboards/dataSources
Add an OpenSearch data source connection
Visit /app/home#/tutorial_directory
Click "Add data" on the "Sample eCommerce orders" card
Click "View data" on the "Sample eCommerce orders" card
Dashboards data in external data source should be showed

Changelog

feat: Add MDS to msearch

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

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

codecov · 2025-02-10T08:54:55Z

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.72%. Comparing base (6cf56f0) to head (2eef6da).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/plugins/data/server/search/routes/msearch.ts	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9361   +/-   ##
=======================================
  Coverage   61.71%   61.72%           
=======================================
  Files        3817     3817           
  Lines       91862    91868    +6     
  Branches    14552    14557    +5     
=======================================
+ Hits        56697    56702    +5     
  Misses      31509    31509           
- Partials     3656     3657    +1

Flag	Coverage Δ
Linux_1	`28.99% <14.28%> (-0.01%)`	⬇️
Linux_2	`56.45% <15.00%> (-0.02%)`	⬇️
Linux_3	`39.20% <95.23%> (+<0.01%)`	⬆️
Linux_4	`28.89% <14.28%> (-0.01%)`	⬇️
Windows_1	`29.01% <14.28%> (-0.01%)`	⬇️
Windows_2	`56.40% <15.00%> (-0.02%)`	⬇️
Windows_3	`39.20% <95.23%> (+<0.01%)`	⬆️
Windows_4	`28.89% <14.28%> (-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.

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

SuZhou-Joe · 2025-02-11T09:01:24Z

src/plugins/data/common/search/search_source/legacy/fetch_soon.ts

+  const key = dataSourceId ?? '';

-  const i = requestsToFetch.length;
-  requestsToFetch = [...requestsToFetch, request];
-  requestOptions = [...requestOptions, options];
+  const i = requestsToFetch[key]?.length ?? 0;
+  requestsToFetch[key] = [...(requestsToFetch[key] ?? []), restRequest];
+  requestOptions[key] = [...(requestOptions[key] ?? []), options];


Nit: better to add some comments here to elaborate that we are grouping request by data source.

SuZhou-Joe · 2025-02-11T09:07:35Z

src/plugins/data/server/search/routes/msearch.ts

+        query: schema.object({
+          data_source_id: schema.maybe(schema.string()),
+        }),


Not very sure if the validation will give error if the request does not have any query.

I've tested with data source disabled case. The msearch API works as expected without the data source id query.

SuZhou-Joe · 2025-02-11T09:13:05Z

src/plugins/data/common/search/search_source/legacy/fetch_soon.ts

@@ -83,20 +83,27 @@ async function delayedFetch(
  if (ms === 0) {
    return callClient([request], [options], fetchHandlers)[0];
  }
+  const { dataSourceId, ...restRequest } = request;


Not sure if it is straightforward to add unit test for this function but it will be great that we have one, the logic is kind of complex.

SuZhou-Joe · 2025-02-11T09:26:27Z

I thought it would be a simple several lines fix. Thanks for this amazing PR.

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

kavilla · 2025-02-12T00:52:30Z

src/plugins/data/server/search/routes/msearch.ts

@@ -67,16 +67,23 @@ export function registerMsearchRoute(router: IRouter, deps: SearchRouteDependenc
            })
          ),
        }),
+        query: schema.object({
+          data_source_id: schema.maybe(schema.string()),


Haven't tested this yet, but I see a potential issue:

Scenario:

Index pattern foo* saved on datasourceA

Matches: foo-1, foo-2, etc.

Index pattern bar* also saved on datasourceA

Matches: bar-1, bar-2, etc.

bar* indices actually exist on datasourceB

Won't this lead to 404s when querying bar* since we're looking in datasourceA but the indices are in datasourceB?

To me i almost think multisearch and multiple data sources is almost fundamentally conflicting ideas. How do we fetch the data from multiple data sources with a single query. this essentially feels like cross cluster searching. unless im missing something

You're right, trying to search for an index pattern in a different data source than where it exists will cause a 404 error.

To solve this, we separate the search requests by data source on the client-side before sending them. The fetchSoon function groups the requests based on their data source, creating multiple msearch requests, one for each data source.

This way, each msearch request is sent to the correct data source, avoiding 404 errors for indices that are in a different data source.

The internal msearch API needs to call msearch in different OpenSearch clients. They are also expensive when combined in the same request. That's why we only support one data source per request.

zhongnansu

I have opened an issue for this before. Could you take a look and see if your PR would address the problem there? Thanks #2174 @wanglam

wanglam · 2025-02-17T02:51:30Z

I have opened an issue for this before. Could you take a look and see if your PR would address the problem there? Thanks #2174 @wanglam

Hi @zhongnansu , I've looked at the issue you mentioned (#2174). This PR can solve the problem described there. We will add a new parameter called "data source id" to the internal msearch API.

The way we manage queued search requests will also be improved in this PR. We will store the search requests, parameters, and ongoing promises separately for each data source.

Feel free to review this PR. If anything needs to be changed or improved, I will update the code accordingly.

kavilla · 2025-02-19T02:45:06Z

src/plugins/data/common/search/search_source/legacy/call_client.ts

@@ -36,7 +36,8 @@ import { ISearchOptions } from '../../index';
 export function callClient(
  searchRequests: SearchRequest[],


Does the search request have the index pattern ? Or data source info?

The search requests interface is not being updated. The data source ID is only present on the client side. It was added at line 359 and removed at line 86. The msearch API's search requests interface remains unchanged. The only modification to the internal msearch API was the addition of a new data_source_id query parameter. This parameter will trigger a msearch call on the specified data source.

kavilla · 2025-02-28T10:05:17Z

src/plugins/data/public/search/legacy/call_msearch.ts

@@ -39,9 +39,10 @@ import { LegacyFetchHandlers } from '../../../common/search/search_source';
 * @internal
 */
 export function getCallMsearch({ http }: { http: HttpStart }): LegacyFetchHandlers['callMsearch'] {
-  return async ({ body, signal }) => {
+  return async ({ body, signal, dataSourceId }) => {


would this require modifying the msearch search params? if we have some?

kavilla · 2025-02-28T10:21:12Z

src/plugins/data/server/search/routes/msearch.ts

@@ -67,16 +67,23 @@ export function registerMsearchRoute(router: IRouter, deps: SearchRouteDependenc
            })
          ),
        }),
+        query: schema.object({


could we actually just shove the data source id into each of the searches.body? and parse out that value to get the client?

kavilla

Overall it looks good to me.

A couple thoughts:

I'm a bit hesitant about modifying the existing callMsearch API. Maybe we could stuff the dataSourceId into each search.body within searches instead and parse it out on the server side? This could mitigate modifying existing APIs.
Alternatively, what about adding a new _enhanced msearch route instead of changing the original? That could keep things more isolated.

But I can be convinced that this ok to pass by query param. Make sure we have solid testing coverage, especially for both MDS and non-MDS scenarios. Leveraging the existing Cypress tests from @ananzh and @angle943 could be a good starting point.

Otherwise, nice work! Let me know what you think about those suggestions. Looking forward to getting this merged.

Add MDS to msearch

09e0e70

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

github-actions bot added the distinguished-contributor label Feb 10, 2025

Changeset file for PR opensearch-project#9361 created/updated

6dc02b4

wanglam added 2 commits February 11, 2025 12:17

Move inside try section

4f2a6db

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

Merge branch 'main' into feat-msearch-multi-data-source

67e4ceb

wanglam marked this pull request as ready for review February 11, 2025 06:19

wanglam requested a review from huyaboo as a code owner February 11, 2025 06:19

SuZhou-Joe reviewed Feb 11, 2025

View reviewed changes

SuZhou-Joe previously approved these changes Feb 11, 2025

View reviewed changes

wanglam added 2 commits February 11, 2025 18:04

Add UT for fetchSoon to cover different data sources

c70c8de

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

Add annotation for grouping requests, options and in-progress objects

e365ecb

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

wanglam dismissed SuZhou-Joe’s stale review via e365ecb February 11, 2025 10:04

kavilla reviewed Feb 12, 2025

View reviewed changes

zhongnansu reviewed Feb 12, 2025

View reviewed changes

Merge branch 'main' into feat-msearch-multi-data-source

0605a80

kavilla reviewed Feb 19, 2025

View reviewed changes

Merge branch 'main' into feat-msearch-multi-data-source

2eef6da

SuZhou-Joe approved these changes Feb 26, 2025

View reviewed changes

SuZhou-Joe added the multiple datasource multiple datasource project label Feb 26, 2025

kavilla reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MDS to msearch #9361

Add MDS to msearch #9361

wanglam commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

SuZhou-Joe Feb 11, 2025

SuZhou-Joe Feb 11, 2025

wanglam Feb 11, 2025

SuZhou-Joe Feb 11, 2025

SuZhou-Joe commented Feb 11, 2025

kavilla Feb 12, 2025

wanglam Feb 17, 2025

zhongnansu left a comment

wanglam commented Feb 17, 2025

kavilla Feb 19, 2025

wanglam Feb 19, 2025

kavilla Feb 28, 2025

kavilla Feb 28, 2025

kavilla left a comment

		@@ -36,7 +36,8 @@ import { ISearchOptions } from '../../index';
		export function callClient(
		searchRequests: SearchRequest[],

Add MDS to msearch #9361

Are you sure you want to change the base?

Add MDS to msearch #9361

Conversation

wanglam commented Feb 10, 2025 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

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

SuZhou-Joe commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongnansu left a comment

Choose a reason for hiding this comment

wanglam commented Feb 17, 2025

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

kavilla left a comment

Choose a reason for hiding this comment

wanglam commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading