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

[MDS] Support Vega Visualizations #5975

Merged
merged 11 commits into from
Mar 8, 2024
7 changes: 3 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### 🛡 Security

### 📈 Features/Enhancements
- [MD]Change cluster selector component name to data source selector ([#6042](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6042))
- [MD]Change cluster selector component name to data source selector ([#6042](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6042))
- [Multiple Datasource] Add interfaces to register add-on authentication method from plug-in module ([#5851](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5851))
- [Multiple Datasource] Able to Hide "Local Cluster" option from datasource DropDown ([#5827](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5827))
- [Multiple Datasource] Add api registry and allow it to be added into client config in data source plugin ([#5895](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5895))
Expand All @@ -23,8 +23,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Workspace] Optional workspaces params in repository ([#5949](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5949))
- [Multiple Datasource] Refactoring create and edit form to use authentication registry ([#6002](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6002))
- [Multiple Datasource] Expose a few properties for customize the appearance of the data source selector component ([#6057](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6057))


- [Multiple Datasource] Add Vega support to MDS by specifying a data source name in the Vega spec ([#5975](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5975))

### 🐛 Bug Fixes

Expand All @@ -38,7 +37,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [osd/std] Add additional recovery from false-positives in handling of long numerals ([#5956](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5956))
- [BUG][Multiple Datasource] Fix missing customApiRegistryPromise param for test connection ([#5944](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5944))
- [BUG][Multiple Datasource] Add a migration function for datasource to add migrationVersion field ([#6025](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6025))
- [BUG][MD]Expose picker using function in data source management plugin setup([#6030](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6030))
- [BUG][MD]Expose picker using function in data source management plugin setup([#6030](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6030))

### 🚞 Infrastructure

Expand Down
2 changes: 1 addition & 1 deletion src/plugins/vis_type_vega/opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"inspector",
"uiActions"
],
"optionalPlugins": ["home", "usageCollection"],
"optionalPlugins": ["home", "usageCollection", "dataSource"],
"requiredBundles": [
"opensearchDashboardsUtils",
"opensearchDashboardsReact",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ export class OpenSearchQueryParser {
name: getRequestName(r, index),
}));

const data$ = this._searchAPI.search(opensearchSearches);
const data$ = await this._searchAPI.search(opensearchSearches);

const results = await data$.toPromise();

Expand Down
159 changes: 159 additions & 0 deletions src/plugins/vis_type_vega/public/data_model/search_api.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { SavedObjectsClientContract, SavedObjectsFindOptions } from 'opensearch-dashboards/public';
import { SearchAPI, SearchAPIDependencies } from './search_api';
import { ISearchStart } from 'src/plugins/data/public';
import { IUiSettingsClient } from 'opensearch-dashboards/public';

jest.mock('rxjs', () => ({
combineLatest: jest.fn().mockImplementation((obj) => obj),
}));

jest.mock('../../../data/public', () => ({
getSearchParamsFromRequest: jest.fn().mockImplementation((obj, _) => obj),
}));

interface MockSearch {
params?: Record<string, unknown>;
dataSourceId?: string;
pipe: () => {};
}

describe('SearchAPI.search', () => {
// This will only test that searchApiParams were correctly set. As such, every other function can be mocked
const getSearchAPI = (dataSourceEnabled: boolean) => {
const savedObjectsClient = {} as SavedObjectsClientContract;

const searchStartMock = {} as ISearchStart;
searchStartMock.search = jest.fn().mockImplementation((obj, _) => {
const mockedSearchResults = {} as MockSearch;
mockedSearchResults.params = obj;
mockedSearchResults.pipe = jest.fn().mockReturnValue(mockedSearchResults.params);
return mockedSearchResults;
});

const uiSettings = {} as IUiSettingsClient;
uiSettings.get = jest.fn().mockReturnValue(0);
uiSettings.get.bind = jest.fn().mockReturnValue(0);

const dependencies = {
savedObjectsClient,
dataSourceEnabled,
search: searchStartMock,
uiSettings,
} as SearchAPIDependencies;
const searchAPI = new SearchAPI(dependencies);
searchAPI.findDataSourceIdbyName = jest.fn().mockImplementation((name) => {
if (!dataSourceEnabled) {
throw new Error();
}
if (name === 'exampleName') {
return Promise.resolve('some-id');
}
});

return searchAPI;
};

test('If MDS is disabled and there is no datasource, return params without datasource id', async () => {
const searchAPI = getSearchAPI(false);
const requests = [{ name: 'example-id' }];
const fetchParams = ((await searchAPI.search(requests)) as unknown) as MockSearch[];
expect(fetchParams[0].params).toBe(requests[0]);
expect(fetchParams[0].hasOwnProperty('dataSourceId')).toBe(false);
});

test('If MDS is disabled and there is a datasource, it should throw an errorr', () => {
const searchAPI = getSearchAPI(false);
const requests = [{ name: 'example-id', data_source_name: 'non-existent-datasource' }];
expect(searchAPI.search(requests)).rejects.toThrowError();
});

test('If MDS is enabled and there is no datasource, return params without datasource id', async () => {
const searchAPI = getSearchAPI(true);
const requests = [{ name: 'example-id' }];
const fetchParams = ((await searchAPI.search(requests)) as unknown) as MockSearch[];
expect(fetchParams[0].params).toBe(requests[0]);
expect(fetchParams[0].hasOwnProperty('dataSourceId')).toBe(false);
});

test('If MDS is enabled and there is a datasource, return params with datasource id', async () => {
const searchAPI = getSearchAPI(true);
const requests = [{ name: 'example-id', data_source_name: 'exampleName' }];
const fetchParams = ((await searchAPI.search(requests)) as unknown) as MockSearch[];
expect(fetchParams[0].hasOwnProperty('params')).toBe(true);
expect(fetchParams[0].dataSourceId).toBe('some-id');
});
});

describe('SearchAPI.findDataSourceIdbyName', () => {
const savedObjectsClient = {} as SavedObjectsClientContract;
savedObjectsClient.find = jest.fn().mockImplementation((query: SavedObjectsFindOptions) => {
if (query.search === `"uniqueDataSource"`) {
return Promise.resolve({
total: 1,
savedObjects: [{ id: 'some-datasource-id', attributes: { title: 'uniqueDataSource' } }],
});
} else if (query.search === `"duplicateDataSource"`) {
return Promise.resolve({
total: 2,
savedObjects: [
{ id: 'some-datasource-id', attributes: { title: 'duplicateDataSource' } },
{ id: 'some-other-datasource-id', attributes: { title: 'duplicateDataSource' } },
],
});
} else if (query.search === `"DataSource"`) {
return Promise.resolve({
total: 2,
savedObjects: [
{ id: 'some-datasource-id', attributes: { title: 'DataSource' } },
{ id: 'some-other-datasource-id', attributes: { title: 'DataSource Copy' } },
],
});
} else {
return Promise.resolve({
total: 0,
savedObjects: [],
});
}
});

const getSearchAPI = (dataSourceEnabled: boolean) => {
const dependencies = { savedObjectsClient, dataSourceEnabled } as SearchAPIDependencies;
return new SearchAPI(dependencies);
};

test('If dataSource is disabled, throw error', () => {
const searchAPI = getSearchAPI(false);
expect(searchAPI.findDataSourceIdbyName('nonexistentDataSource')).rejects.toThrowError(
'data_source_name cannot be used because data_source.enabled is false'
);
});

test('If dataSource is enabled but no matching dataSourceName, then throw error', () => {
const searchAPI = getSearchAPI(true);
expect(searchAPI.findDataSourceIdbyName('nonexistentDataSource')).rejects.toThrowError(
'Expected exactly 1 result for data_source_name "nonexistentDataSource" but got 0 results'
);
});

test('If dataSource is enabled but multiple dataSourceNames, then throw error', () => {
const searchAPI = getSearchAPI(true);
expect(searchAPI.findDataSourceIdbyName('duplicateDataSource')).rejects.toThrowError(
'Expected exactly 1 result for data_source_name "duplicateDataSource" but got 2 results'
);
});

test('If dataSource is enabled but only one dataSourceName, then return id', async () => {
const searchAPI = getSearchAPI(true);
expect(await searchAPI.findDataSourceIdbyName('uniqueDataSource')).toBe('some-datasource-id');
});

test('If dataSource is enabled and the dataSourceName is a prefix of another, ensure the prefix is only returned', async () => {
const searchAPI = getSearchAPI(true);
expect(await searchAPI.findDataSourceIdbyName('DataSource')).toBe('some-datasource-id');
});
});
88 changes: 68 additions & 20 deletions src/plugins/vis_type_vega/public/data_model/search_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import { combineLatest } from 'rxjs';
import { map, tap } from 'rxjs/operators';
import { CoreStart, IUiSettingsClient } from 'opensearch-dashboards/public';
import { SavedObjectsClientContract } from 'src/core/public';
import { DataSourceAttributes } from 'src/plugins/data_source/common/data_sources';
import {
getSearchParamsFromRequest,
SearchRequest,
Expand All @@ -45,6 +47,8 @@
uiSettings: IUiSettingsClient;
injectedMetadata: CoreStart['injectedMetadata'];
search: DataPublicPluginStart['search'];
dataSourceEnabled: boolean;
savedObjectsClient: SavedObjectsClientContract;
}

export class SearchAPI {
Expand All @@ -54,31 +58,75 @@
public readonly inspectorAdapters?: VegaInspectorAdapters
) {}

search(searchRequests: SearchRequest[]) {
async search(searchRequests: SearchRequest[]) {
const { search } = this.dependencies.search;
const requestResponders: any = {};

return combineLatest(
searchRequests.map((request) => {
const requestId = request.name;
const params = getSearchParamsFromRequest(request, {
getConfig: this.dependencies.uiSettings.get.bind(this.dependencies.uiSettings),
});

if (this.inspectorAdapters) {
requestResponders[requestId] = this.inspectorAdapters.requests.start(requestId, request);
requestResponders[requestId].json(params.body);
}

return search({ params }, { abortSignal: this.abortSignal }).pipe(
tap((data) => this.inspectSearchResult(data, requestResponders[requestId])),
map((data) => ({
name: requestId,
rawResponse: data.rawResponse,
}))
);
})
await Promise.all(
searchRequests.map(async (request) => {
const requestId = request.name;
const dataSourceId = !!request.data_source_name
? await this.findDataSourceIdbyName(request.data_source_name)
: undefined;

const params = getSearchParamsFromRequest(request, {
getConfig: this.dependencies.uiSettings.get.bind(this.dependencies.uiSettings),
});

if (this.inspectorAdapters) {
requestResponders[requestId] = this.inspectorAdapters.requests.start(

Check warning on line 78 in src/plugins/vis_type_vega/public/data_model/search_api.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_type_vega/public/data_model/search_api.ts#L78

Added line #L78 was not covered by tests
requestId,
request
);
requestResponders[requestId].json(params.body);

Check warning on line 82 in src/plugins/vis_type_vega/public/data_model/search_api.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_type_vega/public/data_model/search_api.ts#L82

Added line #L82 was not covered by tests
}

const searchApiParams =
dataSourceId && this.dependencies.dataSourceEnabled
? { params, dataSourceId }
: { params };

return search(searchApiParams, { abortSignal: this.abortSignal }).pipe(
tap((data) => this.inspectSearchResult(data, requestResponders[requestId])),
map((data) => ({

Check warning on line 92 in src/plugins/vis_type_vega/public/data_model/search_api.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/vis_type_vega/public/data_model/search_api.ts#L91-L92

Added lines #L91 - L92 were not covered by tests
name: requestId,
rawResponse: data.rawResponse,
}))
);
})
)
);
}

async findDataSourceIdbyName(dataSourceName: string) {
if (!this.dependencies.dataSourceEnabled) {
throw new Error('data_source_name cannot be used because data_source.enabled is false');
}
const dataSources = await this.dataSourceFindQuery(dataSourceName);

// In the case that data_source_name is a prefix of another name, match exact data_source_name
const possibleDataSourceIds = dataSources.savedObjects.filter(
(obj) => obj.attributes.title === dataSourceName
);

if (possibleDataSourceIds.length !== 1) {
throw new Error(
`Expected exactly 1 result for data_source_name "${dataSourceName}" but got ${possibleDataSourceIds.length} results`
);
}
Comment on lines +106 to +117
Copy link
Member

@xinruiba xinruiba Mar 8, 2024

Choose a reason for hiding this comment

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

This logic is to check no duplicate datasource names between all datasources.

But we set the paging size 10 here: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5975/files#diff-f1c265abe9e3b3c11a64d4110cf3e6a010dfe369cf6eed9a20ea532763489fbdR125

In that case, will the duplication check only applies to first 10 datasources?

Copy link
Member Author

Choose a reason for hiding this comment

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

So there are two cases where possibleDataSourceIds before filtering is > 1:

  1. The data_source_name is a duplicate of another (which means querying cannot be done)
  2. The name is a prefix of other data_source_names. For example, if the name were Vega Data Source and the other were Vega Data Source Copy, then the find query for Vega Data Source will return both these data source names. The filtering is done to ensure that Vega Data Source is chosen and not Vega Data Source Copy.

The paging limit of 10 helps keep the number of possible datasources low since this is client-side.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for the initial paging to be free of duplicates, but duplication happens when applied across all data sources?

Copy link
Member

@xinruiba xinruiba Mar 8, 2024

Choose a reason for hiding this comment

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

This comment is not a blocker of this PR.
I don't think we are able to create datasource with duplication name, so this logic should be fine.


return possibleDataSourceIds.pop()?.id;
}

async dataSourceFindQuery(dataSourceName: string) {
return await this.dependencies.savedObjectsClient.find<DataSourceAttributes>({
type: 'data-source',
perPage: 10,
search: `"${dataSourceName}"`,
BionIT marked this conversation as resolved.
Show resolved Hide resolved
searchFields: ['title'],
fields: ['id', 'title'],
});
}

public resetSearchStats() {
Expand Down
1 change: 1 addition & 0 deletions src/plugins/vis_type_vega/public/data_model/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ export interface UrlObject {
[CONSTANTS.TYPE]?: string;
name?: string;
index?: string;
data_source_name?: string;
body?: Body;
size?: number;
timeout?: string;
Expand Down
4 changes: 4 additions & 0 deletions src/plugins/vis_type_vega/public/default.spec.hjson
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@

// Which index to search
index: _all

// If "data_source.enabled: true", optionally set the data source name to query from (omit field if querying from local cluster)
// data_source_name: Example US Cluster

// Aggregate data by the time field into time buckets, counting the number of documents in each bucket.
body: {
aggs: {
Expand Down
15 changes: 14 additions & 1 deletion src/plugins/vis_type_vega/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
* under the License.
*/

import { DataSourcePluginSetup } from 'src/plugins/data_source/public';
import { PluginInitializerContext, CoreSetup, CoreStart, Plugin } from '../../../core/public';
import { Plugin as ExpressionsPublicPlugin } from '../../expressions/public';
import { DataPublicPluginSetup, DataPublicPluginStart } from '../../data/public';
Expand All @@ -41,6 +42,8 @@ import {
setUISettings,
setMapsLegacyConfig,
setInjectedMetadata,
setDataSourceEnabled,
setSavedObjectsClient,
} from './services';

import { createVegaFn } from './expressions/vega_fn';
Expand Down Expand Up @@ -69,6 +72,7 @@ export interface VegaPluginSetupDependencies {
visualizations: VisualizationsSetup;
inspector: InspectorSetup;
data: DataPublicPluginSetup;
dataSource?: DataSourcePluginSetup;
mapsLegacy: any;
}

Expand All @@ -88,14 +92,22 @@ export class VegaPlugin implements Plugin<Promise<void>, void> {

public async setup(
core: CoreSetup,
{ inspector, data, expressions, visualizations, mapsLegacy }: VegaPluginSetupDependencies
{
inspector,
data,
expressions,
visualizations,
mapsLegacy,
dataSource,
}: VegaPluginSetupDependencies
) {
setInjectedVars({
enableExternalUrls: this.initializerContext.config.get().enableExternalUrls,
emsTileLayerId: core.injectedMetadata.getInjectedVar('emsTileLayerId', true),
});
setUISettings(core.uiSettings);
setMapsLegacyConfig(mapsLegacy.config);
setDataSourceEnabled({ enabled: !!dataSource });

const visualizationDependencies: Readonly<VegaVisualizationDependencies> = {
core,
Expand All @@ -116,6 +128,7 @@ export class VegaPlugin implements Plugin<Promise<void>, void> {
public start(core: CoreStart, { data, uiActions }: VegaPluginStartDependencies) {
setNotifications(core.notifications);
setData(data);
setSavedObjectsClient(core.savedObjects);
setUiActions(uiActions);
setInjectedMetadata(core.injectedMetadata);
}
Expand Down
Loading
Loading