feat: add new Influx Line Protocol Parser by akrantz01 · Pull Request #9685 · influxdata/telegraf

akrantz01 · 2021-08-26T18:36:34Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

resolves #9474

~~Currently blocked by influxdata/line-protocol#43~~

Replaces the Influx Line Protocol parser with the new, faster, and zero-allocation parser found here. The only downside to the new parser is that we cannot get the current buffer when using the stream parser variant, so the messages will be slightly less descriptive when an error is encountered.

wz2b · 2021-09-01T15:51:55Z

I have an open feature request #9487 that requires the line protocol parser in telegraf to process and handle comments. After some discussion with @reimda I have been talked out of this idea but I am still looking at this new line parser and I don't see where it gracefully handles comments (even by just throwing them away). Did I miss it?

akrantz01 · 2021-09-01T16:40:39Z

@wz2b I'm not sure where exactly it handles comments, but it does handle them by just discarding them.

srebhan

Hey @akrantz01 thanks for looking into this. I have some comments, the most prominent being

My suggestion to use errors.Is() when checking for a certain error type to handle wrapped errors.
A question regarding the removal of the bytes_received stat.

Would be nice if you can take a look.

plugins/common/shim/processor.go

plugins/inputs/execd/execd.go

plugins/inputs/influxdb_listener/influxdb_listener.go

plugins/inputs/influxdb_listener/influxdb_listener_benchmark_test.go

plugins/inputs/influxdb_v2_listener/influxdb_v2_listener.go

plugins/outputs/execd/execd_test.go

plugins/parsers/influx/parser.go

telegraf-tiger · 2021-09-17T01:53:59Z

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

sspaink

lgtm, do you know if anyone has tested this? @akrantz01 were you able to test all of these changes?

akrantz01 · 2021-09-22T00:35:50Z

I don't know if anyone else has tested it. I also haven't done any extensive testing.

oplehto · 2021-09-22T12:53:47Z

I ran this against a set of diverse real-world metrics data also with a variety of badly formatted lines. It worked quite well but ended up panicing. Unfortunately I do not have the exact line which caused this but maybe the trace will already be helpful. Working on identifying the culprit line as well.

Trace:

panic: runtime error: slice bounds out of range [:-1007604]

goroutine 10 [running]:
github.com/influxdata/telegraf/plugins/parsers/influx.(*ParseError).Error(0xc001d20018)
        /go/src/github.com/influxdata/telegraf/plugins/parsers/influx/parser.go:77 +0x3b0
github.com/influxdata/telegraf/plugins/inputs/nats_consumer.(*natsConsumer).receiver(0xc000001d40, {0x53bd4a8, 0xc000c88640})
        /go/src/github.com/influxdata/telegraf/plugins/inputs/nats_consumer/nats_consumer.go:237 +0x442
created by github.com/influxdata/telegraf/plugins/inputs/nats_consumer.(*natsConsumer).Start.func2
        /go/src/github.com/influxdata/telegraf/plugins/inputs/nats_consumer/nats_consumer.go:202 +0xd5

oplehto · 2021-09-29T11:30:20Z

I haven't been able to pinpoint what could have caused the panic in the metrics stream. One simple and fast workaround would be to have better bounds checking on this array to ensure that the slice is valid and provide the necessary context in the error message to make identifying the problematic line easier.

oplehto · 2021-09-29T16:47:03Z

Found another panic, this one is within the line protocol parser itself so reported it here: influxdata/line-protocol#50

powersj · 2021-09-30T17:14:08Z

@oplehto, would you be willing to share any of the data you are using to cause these issues?

oplehto · 2021-09-30T18:52:17Z

@oplehto, would you be willing to share any of the data you are using to cause these issues?

It's a pretty high-volume real-time data stream with a variety of sources. Thus pinpointing the needle in the haystack is a bit challenging, which I why I was hoping for some more robust exception handling to help narrow it down. In general it would be nice to have bounds checking when referencing slices like this to better catch these type of "unknown unknowns" in live data.

plugins/parsers/influx/parser.go

powersj · 2021-09-30T18:59:32Z

plugins/parsers/influx/parser.go

 	if len(buffer) > maxErrorBufferSize {
 		startEllipsis := true
-		offset := e.Offset - e.LineOffset
+		offset := e.Column - 1 - lineStart


as pointed out in the comments, we need to verify that offset is not > len(buffer) and if it is return len(buffer)

powersj · 2021-10-06T18:58:49Z

Hi!

I am going to take this work over from Alex. I have opened a new PR #9871 where I have pushed his existing work plus a fix for the buffer overflow and have Telegraf set to use the debug version of the Influx Line Protocol parser for help with testing.

I am going to close this PR in favor of my own and hope @oplehto can use that to help debug the current panic.

Thanks!

telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins labels Aug 26, 2021

akrantz01 marked this pull request as ready for review September 1, 2021 16:41

akrantz01 force-pushed the 9474-new-influx-parser branch from 7d99a8e to bf40aff Compare September 1, 2021 16:50

srebhan reviewed Sep 7, 2021

View reviewed changes

srebhan self-assigned this Sep 7, 2021

akrantz01 added 15 commits September 16, 2021 18:31

Switch plain and series parser to new library

695a5be

Switch stream parser to new library

81c92dd

Fix most external tests

8c59991

Remove old parser

a22d9cc

Fix most of influxdb_listener

eeb92c6

Update influxdata/lineprotocol and fix unit tests

973418e

Update dependency licenses

4768757

Fix rebase issues

aa9e39c

Fix linter issues

d8315a2

Fix linter issues (again)

eafd245

Run gofmt

6a51204

Include influx parser in check and fmt

ef1f5f2

Use errors.Is

633f1cd

Inherit from DecodeError

742d6d5

Add back bytes_recieved stat

070d298

akrantz01 force-pushed the 9474-new-influx-parser branch from 642db8c to 070d298 Compare September 17, 2021 01:37

sspaink approved these changes Sep 21, 2021

View reviewed changes

reimda mentioned this pull request Sep 24, 2021

fix: memory leak in influx parser #9787

Merged

3 tasks

oplehto mentioned this pull request Sep 29, 2021

Panic in syntax error handling influxdata/line-protocol#50

Closed

powersj reviewed Sep 30, 2021

View reviewed changes

powersj mentioned this pull request Oct 6, 2021

feat: use new Influx Line Protocol Parser #9871

Closed

3 tasks

powersj closed this Oct 6, 2021

powersj mentioned this pull request Mar 3, 2022

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

Merged

Conversation

akrantz01 commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

wz2b commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akrantz01 commented Sep 1, 2021

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!

telegraf-tiger bot commented Sep 17, 2021

Artifact URLs

Uh oh!

sspaink left a comment

Choose a reason for hiding this comment

Uh oh!

akrantz01 commented Sep 22, 2021

Uh oh!

oplehto commented Sep 22, 2021

Uh oh!

oplehto commented Sep 29, 2021

Uh oh!

oplehto commented Sep 29, 2021

Uh oh!

powersj commented Sep 30, 2021

Uh oh!

oplehto commented Sep 30, 2021

Uh oh!

Uh oh!

powersj Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

powersj commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

akrantz01 commented Aug 26, 2021 •

edited

Loading

wz2b commented Sep 1, 2021 •

edited

Loading