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

[Discover] Filter out OpenSearch Serverless Collections #8758

Merged
merged 6 commits into from
Oct 31, 2024
Merged

[Discover] Filter out OpenSearch Serverless Collections #8758

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4623841
Filtering out serverless collections
sejli Oct 30, 2024
ff7cad6
rerunning linter
sejli Oct 30, 2024
ecfba30
adding test for s3 type
sejli Oct 30, 2024
dc6cb28
updating filter
sejli Oct 30, 2024
848bcdf
string check
sejli Oct 30, 2024
58efea4
update test
sejli Oct 30, 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
14 changes: 9 additions & 5 deletions src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,15 @@
perPage: 10000,
});
const dataSources: DataStructure[] = [DEFAULT_DATA.STRUCTURES.LOCAL_DATASOURCE].concat(
response.savedObjects.map((savedObject) => ({
id: savedObject.id,
title: savedObject.attributes.title,
type: 'DATA_SOURCE',
}))
response.savedObjects
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

but it can be another PR

.filter(
(savedObject) => savedObject.attributes.dataSourceEngineType !== 'OpenSearch Serverless'

Check warning on line 125 in src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts#L125 

Added line #L125 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@AMoo-Miki AMoo-Miki Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree this should ideally be capabilities based

Copy link
Member

Choose a reason for hiding this comment

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

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'

Copy link
Member Author

Choose a reason for hiding this comment

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

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;

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :/

)
.map((savedObject) => ({

Check warning on line 127 in src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts#L127 

Added line #L127 was not covered by tests
id: savedObject.id,
title: savedObject.attributes.title,
type: 'DATA_SOURCE',
}))
);

return injectMetaToDataStructures(dataSources);
Expand Down
26 changes: 15 additions & 11 deletions src/plugins/query_enhancements/public/datasets/s3_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,17 +198,21 @@ const fetchDataSources = async (client: SavedObjectsClientContract): Promise<Dat
});
const dataSources: DataStructure[] = [DEFAULT_DATA.STRUCTURES.LOCAL_DATASOURCE];
return dataSources.concat(
resp.savedObjects.map((savedObject) => ({
id: savedObject.id,
title: savedObject.attributes.title,
type: 'DATA_SOURCE',
meta: {
query: {
id: savedObject.id,
},
type: DATA_STRUCTURE_META_TYPES.CUSTOM,
} as DataStructureCustomMeta,
}))
resp.savedObjects
.filter(
(savedObject) => savedObject.attributes.dataSourceEngineType !== 'OpenSearch Serverless'
sejli marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is attributes guaranteed to be defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

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;

)
.map((savedObject) => ({
id: savedObject.id,
title: savedObject.attributes.title,
type: 'DATA_SOURCE',
meta: {
query: {
id: savedObject.id,
},
type: DATA_STRUCTURE_META_TYPES.CUSTOM,
} as DataStructureCustomMeta,
}))
);
};

Expand Down
Loading