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.

Sign up for GitHub

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

Jump to bottom

[Multiple Datasource][Version Decoupling] Add data source engine type to data source saved object #7026

Merged
merged 3 commits into from
Jun 14, 2024
Merged

[Multiple Datasource][Version Decoupling] Add data source engine type to data source saved object #7026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09ac45c
[Multiple Datasource][Version Decoupling] Add data source engine type…
ZilongX Jun 13, 2024
649c161
Changeset file for PR #7026 created/updated
opensearch-changeset-bot[bot] Jun 13, 2024
26c64e4
Merge branch 'main' into main
ZilongX Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/7026.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- [MDS] Add data source engine type to data source saved object ([#7026](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7026))
8 changes: 8 additions & 0 deletions src/plugins/data_source/common/data_sources/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export interface DataSourceAttributes extends SavedObjectAttributes {
description?: string;
endpoint: string;
dataSourceVersion?: string;
Copy link
Member

Choose a reason for hiding this comment

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

nit: prolly keeping in line with version which makes sense too.

but might be redundant to put dataSource in the field name since it's apart of the DataSource attributes. but i also understand the concept of data sources has different meanings even within this repo so might make sense to specify.

Copy link
Collaborator Author

@ZilongX ZilongX Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah you got the point, the dataSource prefix was added intentionally just to distinguish with other data source terms

dataSourceEngineType?: DataSourceEngineType;
Copy link
Member

Choose a reason for hiding this comment

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

do we need a migration? i dont think so right

OpenSearch-Dashboards/src/plugins/data_source/server/saved_objects/data_source.ts

Line 44 in a4aa682

'2.4.0': flow(migrateDataSource), // 2.4.0 is the version that introduces the datasource

it's optional field. so think it doesn't need it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's why this field is marked optional yes

installedPlugins?: string[];
auth: {
type: AuthType | string;
Expand Down Expand Up @@ -52,3 +53,10 @@ export enum SigV4ServiceName {
}

export { DataSourceError } from './error';

export enum DataSourceEngineType {
OpenSearch = 'OpenSearch',
Copy link
Member

Choose a reason for hiding this comment

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

How come not default?

If the data source then is undefined then it's assumed default. One thing I noticed related to the flint stuff passing around types with spaces causes some issues if the global url states.

vs using default which is the implied OpenSearch and is BWC if we assume default if no type explicitly defined.

I know in the repo it's a mixed casing with enums but not sure if using default will cause some issues lowercase so maybe

export enum DATA_SOURCE_ENGINE_TYPE {
  DEFAULT = 'default',
  SERVERLESS = 'serverless',
  LEGACY = 'legacy',
  NONE = 'none'
}

the serverless banking off that we assume it's OpenSearch. The legacy because internally we reference it as legacy internally [example]

and NONE because it's matches the pattern to other enums in the repo [example]

Copy link
Collaborator Author

@ZilongX ZilongX Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why is default needed?

This engine type is retrieved when the data source object is pulled together and assuming a default just makes no sense to the users where the default just mean OS cluster ? This info is not leveraged in global url states but maybe share the flint issues link if any so we can double check. (is flint even open sourced ?)

Per the legacy it's pretty vague where it could be meaning either Elasticsearch or Legacy OpenSearch (say when we have OpenSearch 3.0 out and OpenSearch 1.x retired which become Legacy OpenSearch). For supported data source types there are three now explicitly so given three exact type rather than giving general terms looks more accurate.

Copy link
Member

@kavilla kavilla Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(is flint even open sourced ?)

but i dont think elasticsearch is either which is why internally codewise we refer to elasticsearch references as legacy

a default just makes no sense to the users where the default just mean OS cluster

do we display this info to the user in functional purposes? have we verified this with legal? previous requirements were no trademark displays from that legacy engine. so if we plan on displaying this info could be an issue. if not then i would consider that internally codewise since forking we reference elasticsearch as legacy. so it would be an antipattern

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this info is not displayed to user, would like to follow up on this legal verification mentioned here, do you have any previous ref or poc from legal ? have no problem to switch all reference of elasticsearch to legacy

OpenSearchServerless = 'OpenSearch Serverless',
Elasticsearch = 'Elasticsearch',
NA = 'No Engine Type Available',
}
28 changes: 17 additions & 11 deletions src/plugins/data_source/server/routes/data_source_connection_validator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,23 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => {
expect(validateDataSourcesResponse.statusCode).toBe(200);
});

test('fetchDataSourceVersion - Success: opensearch client response code is 200 and response body have version number', async () => {
test('fetchDataSourceInfo - Success: opensearch client response code is 200 and response body have version number and distribution', async () => {
const opensearchClient = opensearchServiceMock.createOpenSearchClient();
opensearchClient.info.mockResolvedValue(
opensearchServiceMock.createApiResponse({
statusCode: 200,
body: {
version: {
number: '2.11.0',
distribution: 'opensearch',
},
},
})
);
const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {});
const fetchDataSourcesVersionResponse = await dataSourceValidator.fetchDataSourceVersion();
expect(fetchDataSourcesVersionResponse).toBe('2.11.0');
const fetchDataSourcesVersionResponse = await dataSourceValidator.fetchDataSourceInfo();
expect(fetchDataSourcesVersionResponse.dataSourceVersion).toBe('2.11.0');
expect(fetchDataSourcesVersionResponse.dataSourceEngineType).toBe('OpenSearch');
});

test('fetchInstalledPlugins - Success: opensearch client response code is 200 and response body have installed plugin list', async () => {
Expand Down Expand Up @@ -92,8 +94,8 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => {
}
});

// In case fetchDataSourceVersion call succeeded yet did not return version number, return an empty version instead of raising exceptions
test('fetchDataSourceVersion - Success:opensearch client response code is 200 but response body does not have version number', async () => {
// In case fetchDataSourceInfo call succeeded yet did not return version number and distribution, return an empty info instead of raising exceptions
test('fetchDataSourceInfo - Success:opensearch client response code is 200 but response body does not have version number', async () => {
const opensearchClient = opensearchServiceMock.createOpenSearchClient();
opensearchClient.info.mockResolvedValue(
opensearchServiceMock.createApiResponse({
Expand All @@ -104,8 +106,9 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => {
})
);
const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {});
const fetchDataSourcesVersionResponse = await dataSourceValidator.fetchDataSourceVersion();
expect(fetchDataSourcesVersionResponse).toBe('');
const fetchDataSourcesVersionResponse = await dataSourceValidator.fetchDataSourceInfo();
expect(fetchDataSourcesVersionResponse.dataSourceVersion).toBe('');
expect(fetchDataSourcesVersionResponse.dataSourceEngineType).toBe('No Engine Type Available');
});

test('failure: opensearch client response code is other than 200', async () => {
Expand All @@ -130,8 +133,8 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => {
});
});

// In case fetchDataSourceVersion call failed, return an empty version instead of raising exceptions
test('fetchDataSourceVersion - Failure: opensearch client response code is other than 200', async () => {
// In case fetchDataSourceInfo call failed, return an empty info instead of raising exceptions
test('fetchDataSourceInfo - Failure: opensearch client response code is other than 200', async () => {
const statusCodeList = [100, 202, 300, 400, 500];
statusCodeList.forEach(async function (code) {
const opensearchClient = opensearchServiceMock.createOpenSearchClient();
Expand All @@ -144,8 +147,11 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => {
})
);
const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {});
const fetchDataSourcesVersionResponse = await dataSourceValidator.fetchDataSourceVersion();
expect(fetchDataSourcesVersionResponse).toBe('');
const fetchDataSourcesVersionResponse = await dataSourceValidator.fetchDataSourceInfo();
expect(fetchDataSourcesVersionResponse.dataSourceVersion).toBe('');
expect(fetchDataSourcesVersionResponse.dataSourceEngineType).toBe(
'No Engine Type Available'
);
});
});
});
Expand Down
34 changes: 26 additions & 8 deletions src/plugins/data_source/server/routes/data_source_connection_validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

import { OpenSearchClient } from 'opensearch-dashboards/server';
import { createDataSourceError } from '../lib/error';
import { SigV4ServiceName } from '../../common/data_sources';
import { DataSourceEngineType, SigV4ServiceName } from '../../common/data_sources';
import { DataSourceInfo } from '../types';

export class DataSourceConnectionValidator {
constructor(
private readonly callDataCluster: OpenSearchClient,
Expand Down Expand Up @@ -36,26 +38,42 @@
}
}

async fetchDataSourceVersion() {
let dataSourceVersion = '';
async fetchDataSourceInfo() {
const dataSourceInfo: DataSourceInfo = {
dataSourceVersion: '',
dataSourceEngineType: DataSourceEngineType.NA,
};

try {
// OpenSearch Serverless does not have version concept
if (
this.dataSourceAttr.auth?.credentials?.service === SigV4ServiceName.OpenSearchServerless
) {
return dataSourceVersion;
dataSourceInfo.dataSourceEngineType = DataSourceEngineType.OpenSearchServerless;
return dataSourceInfo;

Check warning on line 53 in src/plugins/data_source/server/routes/data_source_connection_validator.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_source/server/routes/data_source_connection_validator.ts#L52-L53 

Added lines #L52 - L53 were not covered by tests
}

await this.callDataCluster
.info()
.then((response) => response.body)
.then((body) => {
dataSourceVersion = body.version.number;
dataSourceInfo.dataSourceVersion = body.version.number;

if (
body.version.distribution !== null &&
body.version.distribution !== undefined &&
body.version.distribution === 'opensearch'
Copy link
Member

Choose a reason for hiding this comment

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

nit:
body.version.distribution === 'opensearch' should be enough here.
But no hurts to add !== null and !== undefined explicitly.

Comment on lines +62 to +65
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (
body.version.distribution !== null &&
body.version.distribution !== undefined &&
body.version.distribution === 'opensearch'
if (
body.version.distribution === 'opensearch'

This can be simplified

) {
dataSourceInfo.dataSourceEngineType = DataSourceEngineType.OpenSearch;
} else {
dataSourceInfo.dataSourceEngineType = DataSourceEngineType.Elasticsearch;

Check warning on line 69 in src/plugins/data_source/server/routes/data_source_connection_validator.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_source/server/routes/data_source_connection_validator.ts#L69 

Added line #L69 was not covered by tests
}
});

return dataSourceVersion;
return dataSourceInfo;
} catch (e) {
// return empty dataSource version instead of throwing exception in case info() api call fails
return dataSourceVersion;
// return default dataSourceInfo instead of throwing exception in case info() api call fails
return dataSourceInfo;
}
}

Expand Down
10 changes: 9 additions & 1 deletion src/plugins/data_source/server/routes/fetch_data_source_metadata.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,9 @@ describe(`Fetch DataSource MetaData ${URL}`, () => {

const router = httpSetup.createRouter('');
dataSourceClient.info.mockImplementationOnce(() =>
opensearchClientMock.createSuccessTransportRequestPromise({ version: { number: '2.11.0' } })
opensearchClientMock.createSuccessTransportRequestPromise({
version: { number: '2.11.0', distribution: 'opensearch' },
})
);

const installedPlugins = [
Expand Down Expand Up @@ -201,6 +203,7 @@ describe(`Fetch DataSource MetaData ${URL}`, () => {
.expect(200);
expect(result.body).toEqual({
dataSourceVersion: '2.11.0',
dataSourceEngineType: 'OpenSearch',
installedPlugins: ['opensearch-ml', 'opensearch-sql'],
});
expect(dataSourceServiceSetupMock.getDataSourceClient).toHaveBeenCalledWith(
Expand All @@ -224,6 +227,7 @@ describe(`Fetch DataSource MetaData ${URL}`, () => {
.expect(200);
expect(result.body).toEqual({
dataSourceVersion: '2.11.0',
dataSourceEngineType: 'OpenSearch',
installedPlugins: ['opensearch-ml', 'opensearch-sql'],
});
});
Expand Down Expand Up @@ -324,6 +328,7 @@ describe(`Fetch DataSource MetaData ${URL}`, () => {
.expect(200);
expect(result.body).toEqual({
dataSourceVersion: '2.11.0',
dataSourceEngineType: 'OpenSearch',
installedPlugins: ['opensearch-ml', 'opensearch-sql'],
});
});
Expand All @@ -338,6 +343,7 @@ describe(`Fetch DataSource MetaData ${URL}`, () => {
.expect(200);
expect(result.body).toEqual({
dataSourceVersion: '2.11.0',
dataSourceEngineType: 'OpenSearch',
installedPlugins: ['opensearch-ml', 'opensearch-sql'],
});
});
Expand All @@ -352,6 +358,7 @@ describe(`Fetch DataSource MetaData ${URL}`, () => {
.expect(200);
expect(result.body).toEqual({
dataSourceVersion: '2.11.0',
dataSourceEngineType: 'OpenSearch',
installedPlugins: ['opensearch-ml', 'opensearch-sql'],
});
});
Expand All @@ -366,6 +373,7 @@ describe(`Fetch DataSource MetaData ${URL}`, () => {
.expect(200);
expect(result.body).toEqual({
dataSourceVersion: '2.11.0',
dataSourceEngineType: 'OpenSearch',
installedPlugins: ['opensearch-ml', 'opensearch-sql'],
Comment on lines 374 to 377
Copy link
Member

Choose a reason for hiding this comment

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

Is this is the only place where we are comparing result.body during testing? What if we are testing result.body at other places like functional, integration? It will break, right? We will need to include dataSourceEngineType everyehwhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes as dataSourceEngineType is marked as optional to ensure existing test won't fail

});
});
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/data_source/server/routes/fetch_data_source_metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,15 @@ export const registerFetchDataSourceMetaDataRoute = async (
dataSourceAttr
);

const dataSourceVersion = await dataSourceValidator.fetchDataSourceVersion();
const dataSourceInfo = await dataSourceValidator.fetchDataSourceInfo();
const dataSourceVersion = dataSourceInfo.dataSourceVersion;
const dataSourceEngineType = dataSourceInfo.dataSourceEngineType;
const installedPlugins = Array.from(await dataSourceValidator.fetchInstalledPlugins());

return response.ok({
body: {
dataSourceVersion,
dataSourceEngineType,
installedPlugins,
},
});
Expand Down
6 changes: 6 additions & 0 deletions src/plugins/data_source/server/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from 'src/core/server';
import {
DataSourceAttributes,
DataSourceEngineType,
AuthType,
UsernamePasswordTypedContent,
SigV4Content,
Expand Down Expand Up @@ -98,3 +99,8 @@ export interface DataSourcePluginStart {
getAuthenticationMethodRegistry: () => IAuthenticationMethodRegistry;
getCustomApiSchemaRegistry: () => CustomApiSchemaRegistry;
}

export interface DataSourceInfo {
dataSourceVersion?: string;
dataSourceEngineType?: DataSourceEngineType;
}
1 change: 1 addition & 0 deletions ...urce_management/public/components/create_data_source_wizard/create_data_source_wizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export const CreateDataSourceWizard: React.FunctionComponent<CreateDataSourceWiz
// Fetch data source metadata from added OS/ES domain/cluster
const metadata = await fetchDataSourceMetaData(http, attributes);
attributes.dataSourceVersion = metadata.dataSourceVersion;
attributes.dataSourceEngineType = metadata.dataSourceEngineType;
attributes.installedPlugins = metadata.installedPlugins;
await createSingleDataSource(savedObjects.client, attributes);
// Set the first create data source as default data source.
Expand Down
Loading