fix(parsers.json_v2): Reset parser before parsing (#13805)

[agiilit-admin]

Required for all PRs

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

resolves #13805

Running json_v2 parser leaves p.subPathResults dirty, affecting subsequent runs. This is a problem for input plugins reusing json_v2 parser instance instead of creating a new one.

Following the pattern of csv parser, added a Reset() method, which clears subPathResults. It is called in Init() and when starting Parse().

[telegraf-tiger]

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

[agiilit-admin]

!signed-cla

[srebhan]

@agiilit-admin how do you plan to use it? IMO the subpaths should be cleaned out on every call to Parse as we will always get a new document in that call!?!?

[agiilit-admin]

@agiilit-admin how do you plan to use it? IMO the subpaths should be cleaned out on every call to Parse as we will always get a new document in that call!?!?

In CSV parser there is a separate ResetMode, and a conditional inside Parse():

// Reset the parser according to the specified mode
        if p.ResetMode == "always" {
                p.Reset()
        }

but since here I could not think of a use case where Reset() should not be called in Parse() (because, as you said, Parse() is always called with a new document), I left out the if and always call p.Reset() in Parse().

[srebhan]

I think my comment was a bit misleading, sorry for that! What I mean is, why implement an exported Reset method if you can put the p.subPathResults = nil statement directly as a first line in Parse()? Maybe add a small comment that this is required to reset the state of the parser or something...

[srebhan]

Oh and @agiilit-admin you need to sign the CLA as otherwise we cannot take your PR...

[agiilit-admin]

Oh and @agiilit-admin you need to sign the CLA as otherwise we cannot take your PR...

I thought I did and posted the !signed-cla comment here as requested. Is there still something I should do?

[agiilit-admin]

I think my comment was a bit misleading, sorry for that! What I mean is, why implement an exported Reset method if you can put the p.subPathResults = nil statement directly as a first line in Parse()? Maybe add a small comment that this is required to reset the state of the parser or something...

Probably it is just a learned predisposition: I don't think Parse() internal variables such as p.subPathResults should be visible via instance variables at all if Parse() is meant to be reused, but if they are, then there should be an instance method to revert the parser back to the state it was just after coming out of the constructor.

Anyway, comments added.

[srebhan]

While I do agree with your thinking, in the current state this is useless to export Reset() as no plugin can use it without type-asserting the parser. Furthermore, it is fundamentally wrong to require the user of the parser to know about the internal workings! You always need to reset this state after parsing, there is no use-case to not reset subPathResults (contrary to CSV), so just do it and do not require others to care.

Therefore, please do not export the Reset() function! And in turn this then is a one-function-call wrapping function which should go away.

[telegraf-tiger]

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_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

[srebhan]

Thanks for finding and fixing this issue @agiilit-admin! Very much appreciated!