[Discover] Filter out OpenSearch Serverless Collections

[sejli]

Description

Filters out OpenSearch Serverless Collections after fetching data sources

Issues Resolved

Screenshot

Testing the changes

Changelog

• skip

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

[ashwin-pc]

Lgtm

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.75%. Comparing base (48bfe2c) to head (58efea4).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...ery/query_string/dataset_service/lib/index_type.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8758   +/-   ##
=======================================
  Coverage   60.75%   60.75%           
=======================================
  Files        3798     3798           
  Lines       90684    90690    +6     
  Branches    14272    14277    +5     
=======================================
+ Hits        55091    55099    +8     
+ Misses      32094    32091    -3     
- Partials     3499     3500    +1     

Flag	Coverage Δ
Linux_1	29.05% <0.00%> (-0.01%)	⬇️
Linux_2	56.39% <ø> (ø)
Linux_3	37.60% <0.00%> (+<0.01%)	⬆️
Linux_4	29.82% <50.00%> (+<0.01%)	⬆️
Windows_1	29.07% <0.00%> (-0.01%)	⬇️
Windows_2	56.34% <ø> (ø)
Windows_3	37.61% <0.00%> (+<0.01%)	⬆️
Windows_4	29.82% <50.00%> (+<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.

[kavilla]

if we got the okay to filter out serverless then this looks good to me!

definitely not a blocker for this PR or more of a question: do we think it would be worth it for the dataset type config to have a supportedEngineType (following the naming pattern set by @manasvinibs) a while back?

a value we could respect is ALL and all the data source engine types work. that way if the engine type and i think it actually should follow the complete paradigm set by mana. where it actually just be supportedEngines which is a record of semver engines.

this will help future proof us like

supportedEngines: {
  'OpenSearch': "^1.0.0",
}

for scenarios like maybe a specific feature on a cluster.

cc: @virajsanghvi @AMoo-Miki any opinions on this one

[AMoo-Miki]

A. If AWS wants to filter something out, they are more than welcome to patch it in their fork. OpenSearch logic should not contain opinions for AWS. OSD is not the one who defined dataSourceEngineType: 'OpenSearch Serverless' so it shouldn't be the one who filters it out.
B. @kavilla, while i like the flexibility your proposal provides, without many users asking for this flexibility, I cannot convince myself that it is worth complicating the code.

[kavilla]

yeah that's true. something that we sometimes run into is like the query assist be available on only certain clusters and version.

but yeah that would be too complicated and i think we would should solve that more universally with capability APIs

[virajsanghvi]

Agree this should ideally be capabilities based

[joshuali925]

can this use the enum instead of string

OpenSearch-Dashboards/src/plugins/data_source/common/data_sources/types.ts

Line 60 in a70a7b8

OpenSearchServerless = 'OpenSearch Serverless',

OpenSearch logic should not contain opinions for AWS

If OSD allows users to add OpenSearch serverless as datasources to it, I think this is not an opinion for AWS, it's a limitation that this combination does not work in OSD?

OSD is not the one who defined dataSourceEngineType: 'OpenSearch Serverless'

q: who is defining the dataSourceEngineType: 'OpenSearch Serverless'

[sejli]

Should be the data_source plugin when a data source is created.

const handleSubmit = async (attributes: DataSourceAttributes) => {
    setIsLoading(true);
    try {
      // 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;

[AMoo-Miki]

If OSD allows users to add OpenSearch serverless as datasources to it, I think this is not an opinion for AWS, it's a limitation that this combination does not work in OSD?

Thanks for finding that. It should have never happened.

who is defining the dataSourceEngineType: 'OpenSearch Serverless'

Found it in the data_source:

OpenSearch-Dashboards/src/plugins/data_source/server/routes/data_source_connection_validator.ts

Lines 41 to 71 in 190dab0

	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
	) {
	dataSourceInfo.dataSourceEngineType = DataSourceEngineType.OpenSearchServerless;
	return dataSourceInfo;
	}
	await this.callDataCluster
	.info()
	.then((response) => response.body)
	.then((body) => {
	dataSourceInfo.dataSourceVersion = body.version.number;
	if (
	body.version.distribution !== null &&
	body.version.distribution !== undefined &&
	body.version.distribution === 'opensearch'
	) {
	dataSourceInfo.dataSourceEngineType = DataSourceEngineType.OpenSearch;
	} else {
	dataSourceInfo.dataSourceEngineType = DataSourceEngineType.Elasticsearch;
	}
	});

Apparently all of these was added to OSD :/

[ananzh]

We could a follow up on @kavilla's question

[virajsanghvi]

How did we test these changes?

[virajsanghvi]

Is attributes guaranteed to be defined?

[sejli]

Attributes is guaranteed to be defined, but not sure about dataSourceEngineType. This should be some of the data_source plugin work using the saved objects client. I'm not super familiar, but it looks like it should also be defined for data sources here.

.then((response) => {
      const objects = response?.savedObjects;
      if (objects) {
        return objects.map((source) => {
          const id = source.id;
          // Data connection doesn't have title for now, would use connectionId instead to display.
          const title = source.get('title') ?? source.get('connectionId') ?? '';
          const workspaces = source.workspaces ?? [];
          const auth = source.get('auth');
          const description = source.get('description');
          const dataSourceEngineType = source.get('dataSourceEngineType');
          const type = source.type;

[kavilla]

i think this file has tests as well we can add which should be copy pasta from the other one

[kavilla]

but it can be another PR

[kavilla]

i see test failures on the ci but i feel like github actions have been having some issue yesterday and today