Skip to content

Add Index-based adaptor for integrations#1381

Merged
Swiddis merged 20 commits intoopensearch-project:mainfrom
Swiddis:feature/index-adaptor
Feb 1, 2024
Merged

Add Index-based adaptor for integrations#1381
Swiddis merged 20 commits intoopensearch-project:mainfrom
Swiddis:feature/index-adaptor

Conversation

@Swiddis
Copy link
Copy Markdown
Collaborator

@Swiddis Swiddis commented Jan 24, 2024

Description

Index Adaptor is implemented as wrapper around the JSON reader. Solution seems to work fine for the current integrations, but may need to be refactored later for larger counts. The PR also includes refactoring to start using the adaptor as a source for integrations in the TemplateManager, and several smaller lint fixes as part of gradual work on #1363. When merged, users can add integrations by POSTing their serialized forms to .kibana. A Zip reader would still be helpful since integration serialization is specific to this code, but once the serialization is ported to another tool it should be straightforward to upload bundles like this. I expect this to be the method used by the Catalog repository to perform automatic uploading, instead of a zip file which is easier for manual uploads.

I ran into issues while testing the functionality with document length, many existing integrations are too large to be serialized directly. It turns out OS has a document length limit of 1 MB. As a short-term workaround, optimizing images and removing sample data is sufficient to get all current integrations under 0.5 MB (image optimization in general would help in a future PR). In the long term we plan to move images and sample data out of the integration data entirely, they'll be handled by the catalog.

This PR also makes integrations objects manageable in the Saved Objects table:
image

Issues Resolved

Resolves #1380

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (4bffb00) 53.87% compared to head (111f9a9) 53.97%.

Files Patch % Lines
server/routes/integrations/integrations_router.ts 16.66% 10 Missing ⚠️
...tors/integrations/repository/index_data_adaptor.ts 88.88% 3 Missing ⚠️
...rver/adaptors/integrations/integrations_manager.ts 85.71% 1 Missing ⚠️
...ver/adaptors/integrations/repository/repository.ts 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1381      +/-   ##
==========================================
+ Coverage   53.87%   53.97%   +0.10%     
==========================================
  Files         316      317       +1     
  Lines       11309    11349      +40     
  Branches     3014     3018       +4     
==========================================
+ Hits         6093     6126      +33     
- Misses       5167     5174       +7     
  Partials       49       49              
Flag Coverage Δ
dashboards-observability 53.97% <80.26%> (+0.10%) ⬆️

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: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis Swiddis added the enhancement New feature or request label Jan 26, 2024
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis Swiddis added the integrations Used to denote items related to the Integrations project label Jan 29, 2024
console.error(`Requested integration '${integPath}' does not exist`);
async getIntegration(integrationName: string): Promise<IntegrationReader | null> {
const maybeIntegrations = await Promise.all(
this.readers.map((reader) => this.getReaderIntegration(reader, integrationName))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not quite familiar with how the entire index adaptor works, but just wondering if there could have any performance issues as I saw there are recursive file traversals as well as file system operations. How did we perform performance tests and would this be a potential issue to the overall cluster health if the cluster is undergoing high traffic with less computing resource available.

Copy link
Copy Markdown
Collaborator Author

@Swiddis Swiddis Feb 1, 2024

Choose a reason for hiding this comment

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

Short answer is that it's not very performant: #1389. Among other things I want to try adding a cache in a future PR, which should fix the bulk of the issues, but I still need to figure out a framework for more thorough profiling to make sure I'm optimizing the right things.

Altogether there shouldn't be more than a couple dozen megabytes of integrations content at any point (the total of the 11 we have so far is 2.5MB and I don't imagine users adding 100+ integrations), so I don't see any issues with adding an in-memory cache, though I'm not sure how I'm going to handle cache invalidation.

@Swiddis Swiddis merged commit ee3ca58 into opensearch-project:main Feb 1, 2024
@Swiddis Swiddis deleted the feature/index-adaptor branch February 1, 2024 21:29
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 1, 2024
* Update comment for json adaptor construction

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Stub index data adaptor class

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Add initial impl for findIntegrationVersions

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Fill in simple getDirectoryType implementation

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Implement index adaptor as wrapper for json adaptor

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Add integration template type for index

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Fix lints for server/routes

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Fix integrations_manager lints

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Refactor template manager to support multiple readers at once

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Rename FileSystemCatalogDataAdaptor -> FileSystemDataAdaptor

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Add IndexReader to existing Manager logic

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Fix plugin label type

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Add tests for index adaptor

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Add object management to integration objects

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Fix bug with version parsing for numeric integration names

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Prioritize dynamic integrations over defaults

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

---------

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
(cherry picked from commit ee3ca58)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 2.x enhancement New feature or request integrations Used to denote items related to the Integrations project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Integrations: Index Reader

4 participants