feat(outputs.opensearch): opensearch output plugin

[mannukalra]

Required for all PRs

• [x] Updated associated README.md.
• [x] Wrote appropriate unit tests.
• [x] Pull request title or commits are in conventional commit format

resolves #11345

[mannukalra]

#11345 All test cases passed with Opensearch 1.1.0 and 2.2.1.

[mannukalra]

Although I know in the referenced FR it was decided to create a separate plugin, I'm wondering what is different to ES in this case, as it looks like the exact same API module is being used.

Thought of using the opensearch-go client, but as of now there is no stable version which can support both 1.x and 2.x. https://github.com/opensearch-project/opensearch-go/issues/169

Maybe we can update the client in future.

[powersj]

Thought of using the opensearch-go client, but as of now there is no stable version which can support both 1.x and 2.x. opensearch-project/opensearch-go#169

Maybe we can update the client in future.

We recently added the opensearch_query input plugin that uses that library, tested with both versions. In that PR we had a discussion about not using the github.com/olivere/elastic for any further. Is that because of comparability mode?

[mannukalra]

Thought of using the opensearch-go client, but as of now there is no stable version which can support both 1.x and 2.x. opensearch-project/opensearch-go#169 Maybe we can update the client in future.

We recently added the opensearch_query input plugin that uses that library, tested with both versions. In that PR we had a discussion about not using the github.com/olivere/elastic for any further. Is that because of comparability mode?

I was in opinion that opensearch-go/v2 is not backward compatible, but if it works with both will try to use this if you can confirm we need this change.

[powersj]

I was in opinion that opensearch-go/v2 is not backward compatible, but if it works with both will try to use this if you can confirm we need this change.

We do not want any further plugins dependent on github.com/olivere/elastic

@NullOranje - when you looked at testing the new opensearch_query plugin you contributed, did you have to do anything special to test OpenSearch 1 and 2? Were there any compatibility issues?

[NullOranje]

@powerjs No, I didn't do anything differently other than test against a v1 container. I'm not sure what the breaking API changes might be in opensearch-go/v2, but the query API is the same, which is why it works

[powersj]

@mannukalra, what do you think about using opensearch-go/v2 and only targeting opensearch v2 and users can continue to use the elasticsearch plugin for v1?

[NullOranje]

That makes sense to me.

[mannukalra]

@mannukalra, what do you think about using opensearch-go/v2 and only targeting opensearch v2 and users can continue to use the elasticsearch plugin for v1?

Sure @powersj, that also sounds good, will try to make the change in the coming weeks.

[Hipska]

Can you mark the PR as draft in the meantime please?

[powersj]

@mannukalra,

Any progress on the updates to this PR? I know it is in draft, but trying to get an idea of the status of current PRs.

Thanks!

[mannukalra]

@mannukalra,

Any progress on the updates to this PR? I know it is in draft, but trying to get an idea of the status of current PRs.

Thanks!

@powersj, sorry it's been delayed a bit, will update shortly on the progress.

[mannukalra]

@mannukalra, Any progress on the updates to this PR? I know it is in draft, but trying to get an idea of the status of current PRs. Thanks!

@powersj, sorry it's been delayed a bit, will update shortly on the progress. @powersj I've made the changes, would like to bring in notice below 2 items:-

1. Pipeline part is in TODO because bulkIndexer support Pipeline at config level and not metric level, we need to decide on the implementation once.
2. EnableGzip need to be tested thoroughly as I've worked with header as there is not specific config available.

[powersj]

Pipeline part is in TODO because bulkIndexer support Pipeline at config level and not metric level, we need to decide on the implementation once.

Can you outline what this would mean to users a bit more? And what the possible ways forward are?

Thanks for the updates!

[mannukalra]

Pipeline part is in TODO because bulkIndexer support Pipeline at config level and not metric level, we need to decide on the implementation once.

Can you outline what this would mean to users a bit more? And what the possible ways forward are?

Thanks for the updates!

Bulk api provides option to pre-process documents before indexing, by defining a pipeline processors transforms the document in some way, details- https://www.elastic.co/guide/en/elasticsearch/reference/7.10/ingest.html Pipeline can be defined at the url level and inside index json as well, because bulkIndexer in opensearch api supports Pipeline at config level and not metric/json level, so in my opinion we'll have to implement by using multiple bulkIndexers based on the unique pipeline values in the data. https://stackoverflow.com/questions/41909936/elastic-search-bulk-api-pipeline-and-geo-ip

[powersj]

In my opinion we'll have to implement by using multiple bulkIndexers based on the unique pipeline values in the data.

In terms of the user experience, this sounds similar to what the current elasticsearch output does right? Either it uses a user defined pipeline or a user can specify a tag key to use the value of as the pipeline?

Is that something you plan on doing here as well?

[mannukalra]

In my opinion we'll have to implement by using multiple bulkIndexers based on the unique pipeline values in the data.

In terms of the user experience, this sounds similar to what the current elasticsearch output does right? Either it uses a user defined pipeline or a user can specify a tag key to use the value of as the pipeline?

Is that something you plan on doing here as well?

You are correct, no change from user experience stand point.

[mannukalra]

In my opinion we'll have to implement by using multiple bulkIndexers based on the unique pipeline values in the data.

In terms of the user experience, this sounds similar to what the current elasticsearch output does right? Either it uses a user defined pipeline or a user can specify a tag key to use the value of as the pipeline?

Is that something you plan on doing here as well?

@powersj, I've implemented the changes for multiple bulkIndexers based on the unique pipeline values as well, please take a look.

[mannukalra]

Thanks for the updates! Can you resolve the linting and formatting issues? Which versions of opensearch are you going to be able to test this with?

plugins/outputs/opensearch/opensearch.go:250:32 errcheck Error return value of (github.com/opensearch-project/opensearch-go/v2/opensearchutil.BulkIndexer).Add is not checked plugins/outputs/opensearch/opensearch.go:256:26 errcheck Error return value of (github.com/opensearch-project/opensearch-go/v2/opensearchutil.BulkIndexer).Add is not checked plugins/outputs/opensearch/opensearch.go:261:70 errorlint non-wrapping format verb for fmt.Errorf. Use %w to format errors plugins/outputs/opensearch/opensearch.go:268:10 revive indent-error-flow: if block ends with a return statement, so drop this else and outdent its block plugins/outputs/opensearch/opensearch.go:326:103 errorlint non-wrapping format verb for fmt.Errorf. Use %w to format errors plugins/outputs/opensearch/opensearch.go:363:92 errorlint non-wrapping format verb for fmt.Errorf. Use %w to format errors

I can do a full review once I see tests pass.

Thanks again!

Have tested with 1.1.0 and 2.6.0, apologies make fmt is failing for me so we still have formatting errors.

[mannukalra]

Although you started using text/template, it still looks you are doing a lot of unneeded manual pre-processing. The goal is to let the user specify a template and then only the metric needs to be passed to the Execute method.

@mannukalra do you have any feedback on that?

Sorry @Hipska I don't have any working example of this implementation you've asked for earlier as well, also the code suggestions you provided weren't of any help. As requested earlier if you could make this change on your own I'd really appreciate that.

[Hipska]

Hmm, okay, I think we should wait for @srebhan to come back on https://github.com/influxdata/telegraf/issues/13034#issuecomment-1507280841 first before going more into detail..

[srebhan]

Hmm, okay, I think we should wait for @srebhan to come back on #13034 (comment) first before going more into detail..

Why do we need to do this first? As of now, you can access all metric properties from a template, #13034 will only allow to be backward compatible if done.

[Hipska]

To see which approach would be taken, in order to avoid the need to rework stuff again afterwards.. But I agree @mannukalra could already start implementing..

[powersj]

Hi,

Just a gentle ping. v1.27 will release Monday, June 12 and I'd love to see this land before then.

[mannukalra]

Hi,

Just a gentle ping. v1.27 will release Monday, June 12 and I'd love to see this land before then.

Hi @powersj, apologies for the delay, I've made the proposed changes, exploring some efficient alternative to execute test-cases with 2 diff OpenSearch versions, also would request you to propose some approach if you have come across a similar scenario earlier.

[powersj]

exploring some efficient alternative to execute test-cases with 2 diff OpenSearch versions

Unfortunately I think you will need to run 2x the tests, once with 1.x and again with 2.x. I would probably put a table test inside each integration test, that loops through the image name and you can pass that image to launchTestContainer.

Let me know once you push changes!

Thanks!

[mannukalra]

exploring some efficient alternative to execute test-cases with 2 diff OpenSearch versions

Unfortunately I think you will need to run 2x the tests, once with 1.x and again with 2.x. I would probably put a table test inside each integration test, that loops through the image name and you can pass that image to launchTestContainer.

Let me know once you push changes!

Thanks!

As proposed, for now have added separate tests for V2, Thanks to you!

[powersj]

@mannukalra - I rebased on master and pushed a few formatting changes. If this passes tests, which I verified locally, except the integration tests, then I'll +1!

[powersj]

@mannukalra will you have time to go through srebhan's suggestions?

[mannukalra]

@mannukalra will you have time to go through srebhan's suggestions?

Hi @powersj, apologies for the delay, will try to make the changes in this week.