Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: reallow specifying the influx parser type #11806

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

powersj
Copy link
Contributor

@powersj powersj commented Sep 14, 2022

There was logic in the parser registry that was lost in recent parser consolidation that allowed a user to specify which parser they wanted. In the case of exec, execd, and other plugins that use the older parsers.ParserInput, this logic was no longer applied.

Additionally, there were errors about the influx_parser_type config option not getting used.

fixes: #11801

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Sep 14, 2022
@powersj powersj requested a review from srebhan September 14, 2022 15:24
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 the fix @powersj. I think we should add influx_parser_type back to missingTomlField() instead of adding this intermediate option to the struct. For 2.0 we should consider deprecating this additional flag, move the upstream parser below plugins/parsers and require users to set data_format = "influx_upstream".

config/config.go Outdated
Comment on lines 701 to 706
var influxParserType string
c.getFieldString(table, "influx_parser_type", &influxParserType)
if influxParserType == "upstream" {
dataformat = "influx_upstream"
}
Copy link
Member

Choose a reason for hiding this comment

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

How about dealing with this here completely like

Suggested change
var influxParserType string
c.getFieldString(table, "influx_parser_type", &influxParserType)
if influxParserType == "upstream" {
dataformat = "influx_upstream"
}
var influxParserType string
c.getFieldString(table, "influx_parser_type", &influxParserType)
if dataformat == "influx" && influxParserType == "upstream" {
dataformat = "influx_upstream"
}

This should automatically solve the issue without the need to pass the parser type down the line...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok! changes pushed!

@@ -104,7 +104,8 @@ func convertToParseError(input []byte, rawErr error) error {
type Parser struct {
DefaultTags map[string]string `toml:"-"`
// If set to "series" a series machine will be initialized, defaults to regular machine
Type string `toml:"-"`
Type string `toml:"-"`
ParserType string `toml:"influx_parser_type"`
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not needed, instead add back "influx_parser_type" back to missingTomlField() here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

@@ -62,7 +62,8 @@ func (e *ParseError) Error() string {
type Parser struct {
DefaultTags map[string]string `toml:"-"`
// If set to "series" a series machine will be initialized, defaults to regular machine
Type string `toml:"-"`
Type string `toml:"-"`
ParserType string `toml:"influx_parser_type"`
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

There was logic in the parser registry that was lost in recent parser
consolidation that allowed a user to specify which parser they wanted.
In the case of exec, execd, and other plugins that use the older
parsers.ParserInput, this logic was no longer applied.

fixes: influxdata#11801
@powersj
Copy link
Contributor Author

powersj commented Sep 20, 2022

For 2.0 we should consider deprecating this additional flag, move the upstream parser below plugins/parsers and require users to set data_format = "influx_upstream".

For 2.0, we want to set the default parser to be the upstream, and deprecate the additional option. Then for a 3.0, remove the option and only use "upstream"

@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 increases the Telegraf binary size by 1.01 % for linux amd64 (new size: 154.1 MB, nightly size 152.6 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_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 powersj merged commit d091a59 into influxdata:master Sep 21, 2022
popey pushed a commit that referenced this pull request Oct 3, 2022
dba-leshop pushed a commit to dba-leshop/telegraf that referenced this pull request Oct 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

influx_parser_type field unused
3 participants