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

/active_series: improve json encoding performance #6667

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

flxbk
Copy link
Contributor

@flxbk flxbk commented Nov 15, 2023

What this PR does

This adds a benchmark for the querier's ActiveSeriesCardinalityHandler and uses json-iterator to replace the standard library encoding/json to improve marshaling performance.

Here are benchmark results comparing the first two commits in this PR (std vs json-iterator encoding).

goos: darwin
goarch: arm64
pkg: github.com/grafana/mimir/pkg/querier
                                 │   stdjson    │              jsoniter              │
                                 │    sec/op    │   sec/op     vs base               │
ActiveSeriesHandler_ServeHTTP-10   987.62µ ± 0%   98.82µ ± 0%  -89.99% (p=0.002 n=6)

                                 │   stdjson    │              jsoniter               │
                                 │     B/op     │     B/op      vs base               │
ActiveSeriesHandler_ServeHTTP-10   965.7Ki ± 0%   148.8Ki ± 0%  -84.59% (p=0.002 n=6)

                                 │   stdjson    │             jsoniter              │
                                 │  allocs/op   │ allocs/op   vs base               │
ActiveSeriesHandler_ServeHTTP-10   13139.0 ± 0%   132.0 ± 0%  -99.00% (p=0.002 n=6)

Which issue(s) this PR fixes or relates to

n/a

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@flxbk flxbk changed the title Felix/active series encoding benchmark /active_series: improve json encoding performance Nov 15, 2023
@flxbk flxbk marked this pull request as ready for review November 15, 2023 15:07
@flxbk flxbk requested a review from a team as a code owner November 15, 2023 15:07
Copy link
Contributor

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -106,7 +107,16 @@ func ActiveSeriesCardinalityHandler(distributor Distributor, limits *validation.
return
}

util.WriteJSONResponse(w, activeSeriesResponse{res})
var json = jsoniter.ConfigCompatibleWithStandardLibrary
bytes, err := json.Marshal(v1.Response{Data: res})
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a Status: "success" field set as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prometheus does set that field, however it's not quite clear to me why. Isn't the HTTP status code a good enough indication that the payload is going to be ok/parseable as expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I don't think we need the field either, but I think right now it is returning Status: "", which is not useful. So I'd say we either keep it and conform to the prometheus response (which includes setting the error fields), or just return Data and nothing else.

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, makes sense, I'll stick with the custom struct then.

@flxbk flxbk merged commit 557992b into main Nov 16, 2023
28 checks passed
@flxbk flxbk deleted the felix/active-series-encoding-benchmark branch November 16, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants