feat[elastic output]: add elastic pipeline flags by zpriddy · Pull Request #10505 · influxdata/telegraf

zpriddy · 2022-01-24T21:38:12Z

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

resolves #10502

Add new inputs and a new function to the elastic output plugin. The functionality used in these changes are modeled after existing functionality for getting the elastic index name in the plugin. Documentation in the generated sample config file was also updated to include these changes.

Tests were also added to test the functionality of the newly added function GetPipelineName.

telegraf-tiger · 2022-01-24T21:38:16Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

telegraf-tiger · 2022-01-24T21:46:57Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

zpriddy · 2022-01-24T21:50:04Z

!signed-cla

Error: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)

plugins/outputs/elasticsearch/elasticsearch.go

srebhan

Hey @zpriddy. Nice PR! I have some minor comments in the code, nothing big. Can you please take a look?

plugins/outputs/elasticsearch/README.md

plugins/outputs/elasticsearch/elasticsearch.go

plugins/outputs/elasticsearch/elasticsearch_test.go

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

zpriddy · 2022-01-25T22:14:01Z

Hi @srebhan - I applied all fixes except for one that i left the discussion open on in effort to not change the expected output by the plugin

srebhan · 2022-01-28T14:58:10Z

@zpriddy please check my replies and let me know what you think...

srebhan

Almost @zpriddy. If you can reorder the struct-fields (see my comment) we are ready to go I think.

srebhan · 2022-02-01T08:35:23Z

plugins/outputs/elasticsearch/elasticsearch.go

-	FloatReplacement    float64         `toml:"float_replacement_value"`
+	IndexName           string   `toml:"index_name"`
+	DefaultTagValue     string   `toml:"default_tag_value"`
+	tagKeys             []string


Can you maybe group the "user-option" fields and the internal fields here? I.e. move tagKeys and majorReleaseNumber at the end of the struct? That's what most other plugins look like...

I couldn't decide before if they should be done that way, or alphabetically, or... So I ended up leaving it how it was.. Now they are done alphabetically grouped by User Config, locals, then core structs...

telegraf-tiger · 2022-02-01T19:13:45Z

⚠️ This pull request increases the Telegraf binary size by 1.75 % for linux amd64 (new size: 140.2 MB, nightly size 137.8 MB)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

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_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
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
		static_linux_amd64.tar.gz

srebhan

Looks good to me. Thanks for your effort @zpriddy!

powersj

Thanks for the work on this!

reimda · 2022-02-08T22:38:26Z

plugins/outputs/elasticsearch/README.md

+  ## To use a ingest pipeline, set this to the name of the pipeline you want to use.
+  # use_pipeline = "my_pipeline"
+  ## Additionally, you can specify a tag name using the notation {{tag_name}}
+  ## which will be used as part of the pipeline name. 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.
+  # use_pipeline = "{{es_pipeline}}"


We usually only mention a variable once in the sample config. Could you merge the two sections describing "use_pipeline" into one and remove one of the commented settings? It's just a project readme style that makes all the various plugins look more uniform.

Also maybe move the description of default_pipeline to be right before that setting?
Thanks

I was trying to follow the same format as the index name section. But this can be merged, i just wanted to make it clear between using a static value and a dynamic value

@reimda are you good with this as-is?

I'm ok with bending the "one mention" rule to make it more clear, especially since index_name does it already in this plugin.

go.mod

telegraf-tiger · 2022-02-09T00:56:01Z

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 doesn't significantly change the Telegraf binary size (less than 1%)

📦 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_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
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
		static_linux_amd64.tar.gz

powersj · 2022-02-15T17:44:32Z

@zpriddy I think there are a couple of outstanding questions/changes and then this is ready?

zpriddy · 2022-02-15T18:24:04Z

@zpriddy I think there are a couple of outstanding questions/changes and then this is ready?

@powersj I think I have responded to all comments that were left behind.. I have been waiting for responses back to know what the next step should be.

powersj · 2022-02-15T18:26:29Z

@powersj I think I have responded to all comments that were left behind.. I have been waiting for responses back to know what the next step should be.

ok - I've responded to the one open comment

* origin/master: (211 commits) feat: update configs (influxdata#10676) feat[elastic output]: add elastic pipeline flags (influxdata#10505) Update changelog fix: ensure folders do not get loaded more than once (influxdata#10551) docs: update VMWare doc links (influxdata#10663) fix: prometheusremotewrite wrong timestamp unit (influxdata#10547) feat: update configs (influxdata#10662) fix: add graylog toml tags (influxdata#10660) feat: add socks5 proxy support for kafka output plugin (influxdata#8192) docs: override reported OpenSearch version (influxdata#10586) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10659) fix: bump all go.opentelemetry.io dependencies (influxdata#10647) feat: collection offset implementation (influxdata#10545) chore: update go to 1.17.7 (influxdata#10658) fix: check for nil client before closing in amqp (influxdata#10635) fix: timestamp change during execution of json_v2 parser. (influxdata#10657) fix: bump github.com/signalfx/golib/v3 from 3.3.38 to 3.3.43 (influxdata#10652) fix: bump github.com/aliyun/alibaba-cloud-sdk-go (influxdata#10653) fix: incorrect handling of json_v2 timestamp_path (influxdata#10618) feat: gather additional stats from memcached (influxdata#10641) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10649) fix: Revert deprecation of http_listener_v2 (influxdata#10648) fix: bump github.com/denisenkom/go-mssqldb from 0.10.0 to 0.12.0 (influxdata#10503) fix: bump github.com/gopcua/opcua from 0.2.3 to 0.3.1 (influxdata#10626) fix: use current time as ecs timestamp (influxdata#10636) fix: bump github.com/nats-io/nats-server/v2 from 2.6.5 to 2.7.2 (influxdata#10638) chore: add -race flag to go tests (influxdata#10629) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10631) fix: license doc outdated causing CI failure (influxdata#10630) fix: bump k8s.io/client-go from 0.22.2 to 0.23.3 (influxdata#10589) feat: Implemented support for reading raw values, added tests and doc (influxdata#6501) fix: Improve parser tests by using go-cmp/cmp (influxdata#10497) feat(mongodb): add FsTotalSize and FsUsedSize informations (influxdata#10625) docs: update quay docs for auth (influxdata#10612) chore: allow downgrade of go version in windows script (influxdata#10614) chore: update CI go to 1.17.6 (influxdata#10611) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10600) fix(inputs.opcua): add more data to error log (influxdata#10465) fix: bump github.com/aws/aws-sdk-go-v2/service/kinesis from 1.6.0 to 1.13.0 (influxdata#10601) refactor: use early return pattern (influxdata#10591) ...

Elastic output pipelines (influxdata#10502)

50f6b8d

chore: update elastic readme

0a6b285

zpriddy added 2 commits January 24, 2022 13:53

fix: fix linting error

681cabd

Error: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)

chore: update license list file

48621cd

archoversight reviewed Jan 24, 2022

View reviewed changes

plugins/outputs/elasticsearch/elasticsearch.go Outdated Show resolved Hide resolved

zpriddy added 2 commits January 24, 2022 14:29

fix: cleanup some nested if statement5s per linting

0fd6d52

fix: fix list of dependencies

88b1214

zpriddy force-pushed the elasic-output-pipelines branch from d20687a to 88b1214 Compare January 24, 2022 23:20

zpriddy marked this pull request as ready for review January 24, 2022 23:22

srebhan reviewed Jan 25, 2022

View reviewed changes

srebhan self-assigned this Jan 25, 2022

srebhan added area/elasticsearch 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 Jan 25, 2022

zpriddy and others added 4 commits January 25, 2022 13:19

Update plugins/outputs/elasticsearch/README.md

48cf442

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

lint: order imports

18576d2

fix: added all toml tags

6b9ed8e

fix: responding to feedback

3ed78c3

Merge branch 'master' into elasic-output-pipelines

19871a9

zpriddy requested a review from srebhan January 27, 2022 23:01

zpriddy force-pushed the elasic-output-pipelines branch from 8f5dcd6 to cde270a Compare January 28, 2022 21:03

fix: apply requested changes

4ad6eb9

zpriddy force-pushed the elasic-output-pipelines branch from cde270a to 4ad6eb9 Compare January 28, 2022 21:05

srebhan reviewed Feb 1, 2022

View reviewed changes

fix: re-order imports

a7a90c2

zpriddy requested a review from srebhan February 1, 2022 19:05

srebhan approved these changes Feb 2, 2022

View reviewed changes

srebhan 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 Feb 2, 2022

powersj approved these changes Feb 3, 2022

View reviewed changes

reimda suggested changes Feb 8, 2022

View reviewed changes

powersj reviewed Feb 8, 2022

View reviewed changes

go.mod Show resolved Hide resolved

Merge branch 'influxdata:master' into elasic-output-pipelines

f3824e3

reimda approved these changes Feb 17, 2022

View reviewed changes

reimda merged commit a60027a into influxdata:master Feb 17, 2022

MyaLongmire pushed a commit that referenced this pull request Jul 6, 2022

feat[elastic output]: add elastic pipeline flags (#10505)

7631641

Conversation

zpriddy commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

telegraf-tiger bot commented Jan 24, 2022

Uh oh!

telegraf-tiger bot commented Jan 24, 2022

Uh oh!

zpriddy commented Jan 24, 2022

Uh oh!

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zpriddy commented Jan 25, 2022

Uh oh!

srebhan commented Jan 28, 2022

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

srebhan Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

zpriddy Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

telegraf-tiger bot commented Feb 1, 2022

Artifact URLs

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

powersj left a comment

Choose a reason for hiding this comment

Uh oh!

reimda Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

zpriddy Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

powersj Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

reimda Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

telegraf-tiger bot commented Feb 9, 2022

Artifact URLs

Uh oh!

powersj commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zpriddy commented Feb 15, 2022

Uh oh!

powersj commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zpriddy commented Jan 24, 2022 •

edited

Loading

powersj commented Feb 15, 2022 •

edited

Loading