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

Reimplement JsonExpressionParser in terms of jsonparser #8734

Merged
merged 4 commits into from
Mar 8, 2023

Conversation

MasslessParticle
Copy link
Contributor

This PR reimplements the functionality of JSONExpressionParser using the jsonparser library rather than jsoniter

Benchmarks:

benchstat json_expression_old.txt json_expression_new.txt 
name                                    old time/op    new time/op    delta
JsonExpressionParser/json-expression-8    4.15µs ± 4%    0.82µs ± 3%  -80.17%  (p=0.000 n=10+10)

name                                    old alloc/op   new alloc/op   delta
JsonExpressionParser/json-expression-8    1.07kB ± 0%    0.02kB ± 0%  -97.76%  (p=0.000 n=10+10)

name                                    old allocs/op  new allocs/op  delta
JsonExpressionParser/json-expression-8      72.0 ± 0%       2.0 ± 0%  -97.22%  (p=0.000 n=10+10)

@MasslessParticle MasslessParticle requested a review from a team as a code owner March 7, 2023 17:43
for _, p := range paths {
switch v := p.(type) {
case int:
stingPaths = append(stingPaths, fmt.Sprintf("[%d]", v))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The index syntax for paths in jsonparser differs slightly from jsoniter. This function turns our representation into what is expected by the new implementation

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Great work @MasslessParticle! Really excited for this. I'd be keen to see how this stacks up against the regular json parser; if it's now faster/more resource-efficient, we can start recommending its use when specific keys are needed.

Added a comment about error handling, but overall LGTM

pkg/logql/log/parser.go Show resolved Hide resolved
return line, true
}

func isValidJSONStart(data []byte) bool {
switch data[0] {
case '"', '{', '}', '[', ']':
Copy link
Contributor

Choose a reason for hiding this comment

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

Are closing brackets really valid JSON start tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! This is copy paste from the parser internals

@MasslessParticle
Copy link
Contributor Author

MasslessParticle commented Mar 8, 2023

@dannykopping Here's @cyriltovena's original PR for JSON Parser: #7723.

the jsonparser library helped somewhat. this PR ought to buy us quite a bit -- at lease for metrics queries.

@MasslessParticle MasslessParticle merged commit 9c6e509 into main Mar 8, 2023
@MasslessParticle MasslessParticle deleted the tpatterson/jsonparser-for-json-expressions branch March 8, 2023 15:53
@MasslessParticle
Copy link
Contributor Author

@dannykopping It's interesting to note that when the JSON payload is small and doesn't contain nested keys, the JSON Parser performs about as well as logfmt. These benchmarks use a simpler JSON that @cyriltovena's

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants