Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Allow (controlled) reuse of parsers again #14025

Closed
athornton opened this issue Sep 29, 2023 · 4 comments · Fixed by #14149
Closed

Allow (controlled) reuse of parsers again #14025

athornton opened this issue Sep 29, 2023 · 4 comments · Fixed by #14149
Labels
feature request Requests for new plugin and for new features to existing plugins

Comments

@athornton
Copy link
Contributor

Use Case

After #13886 was merged, and we deployed a rebased telegraf instance, our performance took a nosedive. We realized it was because we were making a network call to our Schema Registry with every single measurement, because now we're creating a new Avro parser object each measurement, and therefore the schema cache, which is inside the parser, is always empty.

Expected behavior

We expected performance to stay about the same: one call to the registry per schema ID (there are many, many more measurements than distinct schemas).

Actual behavior

We're hitting the schema registry on each measurement, which is very slow, creates a huge load on the schema registry, and gobbles up a lot of memory for all of the simultaneously-open TCP connections.

Additional info

A little discussion on the InfluxDB Slack resulted in this suggested from @srebhan :

This is really another instance of "stateful parser"... :disappointed: In my view we need to introduce a Clone() telegraf.Parser interface to allow the parser to decide what shall be copied to a new instance and what shouldn't. This way we will get rid of special handling for e.g. CSV etc. In your case Clone() would simply return the present parser instance if if parsing is thread-safe (which was the reason for #13886). In other cases we might need to copy the whole parser or parts of it...

So I intend to add the Clone() interface to the Avro parser, and then take a look at what needs doing to plumb that into the generic create-a-parser logic.

@athornton athornton added the feature request Requests for new plugin and for new features to existing plugins label Sep 29, 2023
@athornton
Copy link
Contributor Author

I wanted to get some feedback from @powersj and @srebhan about how to implement this.

So, should Clone() be a zero-argument method you call on an initialized Parser object, returning a pointer to a Parser? The superclass implementation gets you a new Parser, calls Init() on it, and returns a pointer to that object, while the one for the Avro parser just returns a pointer to the extant object?

@athornton
Copy link
Contributor Author

So...my next question is...is there any easy way to tell which parsers are thread-safe? Is there any easy way to tell which fields for a given parser can be shallow-copied and where I need a deep copy?

The idea of implementing Clone() for every parser class fills me with a certain amount of dread, especially if it means that I'm going to have to learn how each one works internally.

@AlbertasB
Copy link
Contributor

Just wanted to add that I also encountered HUGE performance hit after #13886 has been merged

@powersj
Copy link
Contributor

powersj commented Oct 18, 2023

@athornton, @srebhan, and myself met to discuss this today.

The next steps involve:

The conclusion we reached was that the json_v2 has known issues around parsing data in a thread safe manner, resulting in #13886. However, this has impacted performance and other parsers that have state too much. Hence the revert.

For json_v2, we are going to add some blocking into the parse function in order to mitigate issues that were seen, without impact other parsers for now. Then additional work can occur later to better understand what is wrong specifically with json_v2.

Finally, in order to have real data around the parser performance, we want to add some benchmark functions around each parse function. This way we can get some idea of the performance for any given release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requests for new plugin and for new features to existing plugins
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants