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(outputs.opensearch): opensearch output plugin #11958

Merged
merged 35 commits into from
Sep 29, 2023

Conversation

mannukalra
Copy link
Contributor

@mannukalra mannukalra commented Oct 7, 2022

Required for all PRs

resolves #11345

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Oct 7, 2022
@mannukalra
Copy link
Contributor Author

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

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

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.

plugins/outputs/opensearch/README.md Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
@mannukalra
Copy link
Contributor Author

mannukalra commented Oct 14, 2022

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.
opensearch-project/opensearch-go#169

Maybe we can update the client in future.

plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/README.md Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
@powersj
Copy link
Contributor

powersj commented Feb 16, 2023

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?

@powersj powersj added the waiting for response waiting for response from contributor label Feb 16, 2023
@mannukalra
Copy link
Contributor Author

mannukalra commented Feb 18, 2023

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.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Feb 18, 2023
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

I'm still in favour of using golang templates instead of something mixed and a lookalike of it.

plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
plugins/outputs/opensearch/opensearch.go Outdated Show resolved Hide resolved
@powersj
Copy link
Contributor

powersj commented Feb 21, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

powersj commented Feb 21, 2023

@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
Copy link
Contributor

That makes sense to me.

@mannukalra
Copy link
Contributor Author

@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
Copy link
Contributor

Hipska commented Feb 27, 2023

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

@Hipska Hipska requested review from Hipska and removed request for Hipska February 27, 2023 08:21
@powersj powersj marked this pull request as draft February 27, 2023 13:35
@powersj
Copy link
Contributor

powersj commented Apr 4, 2023

@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 powersj added the waiting for response waiting for response from contributor label Apr 4, 2023
@mannukalra
Copy link
Contributor Author

@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 closed this Apr 10, 2023
@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Apr 10, 2023
@mannukalra mannukalra reopened this Apr 10, 2023
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Great! Finally using the full power of GoLang templates!

Some minor remarks..

// Determine if we should process NaN and inf values
valOptions := []string{"", "none", "drop", "replace"}
if err := choice.Check(o.FloatHandling, valOptions); err != nil {
return fmt.Errorf("config float_handling type %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("config float_handling type %w", err)
return fmt.Errorf("config float_handling type: %w", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


indexTmpl, err := template.New("index").Parse(o.IndexName)
if err != nil {
return fmt.Errorf("error parsing indextemplate %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("error parsing indextemplate %w", err)
return fmt.Errorf("error parsing index_name template: %w", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


pipelineTmpl, err := template.New("index").Parse(o.UsePipeline)
if err != nil {
return fmt.Errorf("error parsing pipelineTemplate for UsePipeline %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("error parsing pipelineTemplate for UsePipeline %w", err)
return fmt.Errorf("error parsing use_pipeline template: %w", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


For example: "telegraf-{{.Time.Format "2006-01-02"}}" would set it to telegraf-2023-07-27
You can also specify
metric name (`{{Name}}`), tag value (`{{Tag "tag_name"}}`), field value (`{{Field "feild_name"}}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
metric name (`{{Name}}`), tag value (`{{Tag "tag_name"}}`), field value (`{{Field "feild_name"}}`)
metric name (`{{ .Name }}`), tag value (`{{ .Tag "tag_name" }}`), field value (`{{ .Field "feild_name" }}`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment on lines 13 to 19
## %Y - year (2016)
## %y - last two digits of year (00..99)
## %m - month (01..12)
## %d - day of month (e.g., 01)
## %H - hour (00..23)
## %V - week of the year (ISO week) (01..53)
## For example: telegraf-%Y.%m.%d would set it to telegraf-2006-01-02
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still the old way, please update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment on lines 23 to 25
## Additionally, you can specify a tag name using the notation {{tag_name}}
## which will be used as part of the index name: "telegraf-{{host}}-%Y.%m.%d"
## If the tag does not exist, the default tag value will be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would mention the possibility to use tags in the relevant section of "Index Name" and here just what happens if that tag doesn't exist..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be replaced with empty sting, have explained.
Just so you know- don't see a scope of handling the default_tag_value anymore with this implementation(have tried in test-cases, it only replaced with "" and not the value specified for DefaultTagValue), so removed DefaultTagValue field from struct. Let me know if I'm mistaken here and it can still work.

Comment on lines 98 to 102
## Additionally, you can specify a tag name using the notation {{tag_name}}
## which will be used as part of the pipeline name (e.g. "{{es_pipeline}}").
## If the tag does not exist, the default pipeline will be used as the
## pipeline. If no default pipeline is set, no pipeline is used for the
## metric.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, explain the usage of tags directly in "Pipeline Config".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Thanks for all the work!

@Hipska Hipska added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 18, 2023
@Hipska Hipska removed their assignment Sep 18, 2023
@srebhan
Copy link
Member

srebhan commented Sep 19, 2023

@mannukalra would you please be so kind to fix the CI tests before I'm doing the review!? It should be a make fmt.

I'm usually running make tidy && make fmt && make && make test && make vet && make check-deps && make lint && markdownlint ***/*.md before committing as this catches most of the CI issues...

@powersj
Copy link
Contributor

powersj commented Sep 27, 2023

I've cleaned up the PR so tests at least pass locally, will check CI shortly. I do want to do a review before we land as well.

@powersj powersj self-requested a review September 27, 2023 20:29
@Hipska
Copy link
Contributor

Hipska commented Sep 28, 2023

@powersj Still needs a make docs I assume.

@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

🥳 This pull request decreases the Telegraf binary size by -2.32 % for linux amd64 (new size: 197.3 MB, nightly size 202.0 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for all your effort @mannukalra and thanks for fixing the remaining issues @powersj!

@srebhan srebhan merged commit 4e35ac8 into influxdata:master Sep 29, 2023
23 checks passed
@github-actions github-actions bot added this to the v1.29.0 milestone Sep 29, 2023
@mannukalra
Copy link
Contributor Author

Thanks for all your effort @mannukalra and thanks for fixing the remaining issues @powersj!

Thank you all for your support 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create OpenSearch output as fork diverges from ElasticSearch
5 participants