feat(parsers.influx): New influx line protocol via feature flag #10749

powersj · 2022-02-28T19:58:51Z

Add the ability to use the upstream Influx Line Protocol parser with the new, zero-allocation with the existing internal parser. Users can choose to use the new 'upstream' parser with the influx_parser_type config option or with the parser_type config option with the influxdb_v2_listener.

Moves time influx.TimeFunc to a common file for use by both parsers.

Blocked by influxdata/line-protocol#50
Previous PR #9685
Resolves #9474
Authored-by: Alex Krantz [email protected]

plugins/parsers/registry.go

This introduces a new parser option to allow users to choose between the upstream (newer, more memory efficient and faster) influx line protocol parser or the built-in, included influx line protocol.

powersj · 2022-03-03T18:37:05Z

@reimda would you be willing to give the last commit a quick review? The big changes are:

Moving TimeFunc to a common location to be used by both parsers
add parser_type config option to influxdb_v2_listener + tests
Update README for line protocol parsers

I am looking at changes to influxdb_v1_listener, but so far do not feel my changes are appropriate

reimda

review of 08d78ec

plugins/parsers/influx/parser.go

reimda · 2022-03-03T21:54:22Z

plugins/inputs/influxdb_v2_listener/influxdb_v2_listener.go


-		if err != influx.EOF && err != nil {
+		if err != influx.EOF && err != influx_upstream.ErrEOF && err != nil {


It feels a little strange and maybe unsafe to handle errors from either parser in the same place without any type assertion on the error. I think in the logs we're also going to want to know which parser generated the error. Maybe handle the specific errors caused by each parser right after each Parse finishes and only handle common errors like the badRequest after the if/else?

influx.EOF and influx.ErrEOF are the same thing, an errors.New("EOF"). We are only handling types of error in this if statement so I'm not sure I follow the concern about type assertions.

Let's store errors.New("EOF) as a variable in this package and compare err to that variable here.

plugins/inputs/influxdb_v2_listener/README.md

plugins/parsers/influx_upstream/README.md

plugins/parsers/registry.go

* Keep influx_upstream under influx * Add and update READMEs for influx parsers

reimda · 2022-03-04T22:52:34Z

plugins/inputs/influxdb_v2_listener/influxdb_v2_listener_test.go

-	require.NoError(t, err)
-	require.NoError(t, resp.Body.Close())
-	require.EqualValues(t, 204, resp.StatusCode)
+	for _, parser := range []string{"internal", "upstream"} {


I've been thinking about this pattern. It reuses the listener for both parsers and always runs in a specific order of parsers. It's best to test as close to real use as possible so this isn't ideal. I think it would be better to pull the initialization into the parser for loop to use a new listener for each parser.

Doing it this way has some testing usability drawbacks too. If there is a failure we won't know which parser was involved. Also it doesn't allow us to run all tests of just one of the parser types. Using golang subtests would fix both those problems. See https://go.dev/blog/subtests#table-driven-tests-using-subtests

Maybe these improvements aren't needed, but they were on my mind so I thought I'd share them with you.

done - the reason I did not do this initially was since the expected input and outputs are the same and the parser is a run-time check it did not seem to make sense. We both agree that it is a best practice, however slightly not sure it is a perfect fit here, but made the change anyway.

Looks good with the Run func.

Could we define the testCases slice in one place, maybe global, so it's not repeated in each function?

reimda · 2022-03-08T22:33:48Z

plugins/parsers/influx/README.md

+  ## Influx parser type to use. Users can choose between 'internal' and
+  ## 'upstream'. The internal parser is what Telegraf has historically used.
+  ## While the upstream parser involved a large re-write to make it more
+  ## memory efficient and performant.
+  ## influx_parser_version = "internal"


Let's use the same shorter text here too.

* update parser readme to be inline with listeners * global EOF error check * consolidate the test cases for both listener tests

telegraf-tiger · 2022-03-08T23:21:22Z

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

reimda

Looks good to me!

sjwang90 · 2022-03-22T20:12:34Z

@oplehto Do you by chance have before/after performance numbers of using the new parser compared to the old one?

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Feb 28, 2022

powersj commented Feb 28, 2022

View reviewed changes

plugins/parsers/registry.go Show resolved Hide resolved

powersj force-pushed the feat/influx-line-protocol-flag branch from 654ef28 to ff0d893 Compare March 1, 2022 15:06

feat: new influx line protocol via feature flag

8149f90

This introduces a new parser option to allow users to choose between the upstream (newer, more memory efficient and faster) influx line protocol parser or the built-in, included influx line protocol.

powersj force-pushed the feat/influx-line-protocol-flag branch from 5fb6ed2 to 08d78ec Compare March 3, 2022 18:28

reimda reviewed Mar 3, 2022

View reviewed changes

Add the new parser as an option to influxdb_v2_listener

ce01646

* Keep influx_upstream under influx * Add and update READMEs for influx parsers

powersj force-pushed the feat/influx-line-protocol-flag branch from 08d78ec to ce01646 Compare March 4, 2022 15:51

Add option to influxdb_listener to use new upstream parser

c8d3590

reimda reviewed Mar 4, 2022

View reviewed changes

test -> use testcases

1bf05c2

powersj marked this pull request as ready for review March 7, 2022 17:20

powersj 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 Mar 8, 2022

reimda reviewed Mar 8, 2022

View reviewed changes

sspaink approved these changes Mar 8, 2022

View reviewed changes

review changes:

bf3f3de

* update parser readme to be inline with listeners * global EOF error check * consolidate the test cases for both listener tests

reimda approved these changes Mar 10, 2022

View reviewed changes

reimda changed the title ~~feat: new influx line protocol via feature flag~~ feat(parsers.influx): New influx line protocol via feature flag Mar 10, 2022

reimda merged commit 40ed7fb into influxdata:master Mar 10, 2022

powersj mentioned this pull request Mar 10, 2022

feat: use new Influx Line Protocol Parser #9871

Closed

3 tasks

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

feat(parsers.influx): New influx line protocol via feature flag (#10749)

3b872ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(parsers.influx): New influx line protocol via feature flag #10749

feat(parsers.influx): New influx line protocol via feature flag #10749

powersj commented Feb 28, 2022 •

edited

Loading

powersj commented Mar 3, 2022

reimda left a comment

reimda Mar 3, 2022

powersj Mar 4, 2022

reimda Mar 8, 2022

reimda Mar 4, 2022

powersj Mar 7, 2022

reimda Mar 8, 2022

reimda Mar 8, 2022

telegraf-tiger bot commented Mar 8, 2022

Artifact URLs

reimda left a comment

sjwang90 commented Mar 22, 2022


		if err != influx.EOF && err != nil {
		if err != influx.EOF && err != influx_upstream.ErrEOF && err != nil {

feat(parsers.influx): New influx line protocol via feature flag #10749

feat(parsers.influx): New influx line protocol via feature flag #10749

Conversation

powersj commented Feb 28, 2022 • edited Loading

powersj commented Mar 3, 2022

reimda left a comment

Choose a reason for hiding this comment

reimda Mar 3, 2022

Choose a reason for hiding this comment

powersj Mar 4, 2022

Choose a reason for hiding this comment

reimda Mar 8, 2022

Choose a reason for hiding this comment

reimda Mar 4, 2022

Choose a reason for hiding this comment

powersj Mar 7, 2022

Choose a reason for hiding this comment

reimda Mar 8, 2022

Choose a reason for hiding this comment

reimda Mar 8, 2022

Choose a reason for hiding this comment

telegraf-tiger bot commented Mar 8, 2022

Artifact URLs

reimda left a comment

Choose a reason for hiding this comment

sjwang90 commented Mar 22, 2022

powersj commented Feb 28, 2022 •

edited

Loading