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

feat: add support for OpenSearch backend #348

Merged
merged 7 commits into from
Oct 4, 2023

Conversation

maximepln
Copy link
Contributor

@maximepln maximepln commented Sep 1, 2023

Everything is in the title but I will add a bit of details

The goal of that PR is to implement a new backend provider Opensearch

For instance, I opened an issue about this there: Issue 346,
in which I explained why the Elasticsearch wasn't usable with Opensearch

As you will see, some code could have been mutualised between the two backend. But as we don't know the direction that will take the 2 backends in the future, and due to the fact that they could become quite different, I chose to avoid adding too much complexity.

Furthermore, most of the people will work on either Opensearch or Elasticsearch so having a common codebase could become complicated when wanting to add new features.

Everything is documented in the right directories and unit tests & mocks were implemented.

@maximepln
Copy link
Contributor Author

Hi @lvaylet, @bkamin29

What are your thought on this ?

I had to disable py-lint code duplication to have the CI to pass.

@lvaylet
Copy link
Collaborator

lvaylet commented Sep 6, 2023

Hi @maximepln, thanks a lot for this submission. I will take a look at it shortly. Regarding your choice of going with two different backends (with some duplicate code), I ended up doing the exact same thing to support MQL in Cloud Monitoring on top of the existing MQF backend.

@lvaylet lvaylet self-requested a review September 6, 2023 12:34
@lvaylet lvaylet assigned lvaylet and maximepln and unassigned lvaylet Sep 6, 2023
@lvaylet lvaylet added feature New feature or request backend labels Sep 6, 2023
@lvaylet lvaylet changed the title feat(Backend): Add Opensearch as a backend provider feat: add support for OpenSearch backend Sep 6, 2023
@lvaylet lvaylet added the p/opensearch OpenSearch provider issue label Sep 6, 2023
Copy link
Collaborator

@lvaylet lvaylet left a comment

Choose a reason for hiding this comment

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

A few minor changes left. Thanks for the good work!

docs/providers/opensearch.md Outdated Show resolved Hide resolved
docs/providers/opensearch.md Outdated Show resolved Hide resolved
slo_generator/backends/opensearch.py Outdated Show resolved Hide resolved
@maximepln
Copy link
Contributor Author

Thanks for the feedback @lvaylet,

I updated to code to use OpenSearch instead of Opensearch

Don't hesitate if you have any other comments

@maximepln maximepln removed their assignment Sep 6, 2023
Copy link
Collaborator

@lvaylet lvaylet left a comment

Choose a reason for hiding this comment

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

Thanks a lot for going over every instance of OpenSearch in the commits. I suggested a couple more changes to remain consistent with other backends (Cloud Monitoring specifically).

docs/providers/opensearch.md Outdated Show resolved Hide resolved
docs/providers/opensearch.md Outdated Show resolved Hide resolved
docs/providers/opensearch.md Outdated Show resolved Hide resolved
@maximepln
Copy link
Contributor Author

Hi @lvaylet,

Sorry for the delay to respond, I was away on holiday.

Hope everything is fine now

@bkamin29
Copy link

bkamin29 commented Oct 3, 2023

hello @lvaylet, any updates ?

@lvaylet
Copy link
Collaborator

lvaylet commented Oct 3, 2023

I will take a look tomorrow afternoon.

@lvaylet lvaylet merged commit f7bb0d9 into google:master Oct 4, 2023
10 checks passed
@maximepln maximepln deleted the feature/opensearch-backend branch October 4, 2023 09:39
lvaylet pushed a commit that referenced this pull request Oct 20, 2023
* feat(Backend): Add Opensearch as a backend provider

* fix: naming of package

* fix: disable pylint code duplication

* fix: rename opensearch to OpenSearch

* fix: Opensearch to OpenSearch and opensearch to open_search

* fix: few typo comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend feature New feature or request p/opensearch OpenSearch provider issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💡 [REQUEST] - Extend Elasticsearch backend to be enable to use Opensearch
3 participants