api/prometheus/v1: merge envelope and payload JSON decode, fixes #977 - #2078
api/prometheus/v1: merge envelope and payload JSON decode, fixes #977#2078felpau05 wants to merge 1 commit into
Conversation
Removes the redundant second JSON parse across all 20 methods affected by prometheus#977. Decodes the envelope and typed payload in a single json.Unmarshal pass instead of an intermediate json.RawMessage that each method re-parsed separately. Changes apiResponse.Data from json.RawMessage to interface{}. Updates Do and DoGetFallback to take a new data interface{} destination. A typed pointer decodes directly via Go's existing behavior of following a non-nil pointer stored in an interface field. Passing nil falls back to the current json.RawMessage behavior unchanged (used by CleanTombstones and DeleteSeries). FormatQuery remains unconverted as it uses a raw string(body) cast rather than json.Unmarshal on the body. Fixes prometheus#977. Signed-off-by: felpau05 <felpau05@gmail.com>
| } | ||
|
|
||
| return resp, []byte(result.Data), result.Warnings, result.Infos, err | ||
| return resp, []byte(rawData), result.Warnings, result.Infos, err |
There was a problem hiding this comment.
rawData ends up being empty in success cases when a destination object is provided as an arg, which is a confusing behavior ... can we eliminate the []byte return value and require callers to provide a RawValue destination object if they want raw bytes?
| req.Header["Idempotency-Key"] = nil | ||
|
|
||
| resp, body, warnings, infos, err := h.Do(ctx, req) | ||
| resp, body, warnings, infos, err := h.Do(ctx, req, data) |
There was a problem hiding this comment.
does passing the same object into Do twice expose us to double-decode issues where data accumulates into the decoded object from decoding twice? are we sure we will only ever get back these statuses when Do() didn't attempt to unmarshal at all?
|
I just opened a tracking issue (#2105) around moving away from the unsafe/unmaintained json-iterator decoding library to stdlib json, since Go 1.27 makes json and json/v2 more efficient. If there are other PRs optimizing json decoding (like this one) it could be ideal to make that transition as part of the optimization so that our overall performance improves as well as becoming safer / more maintained. What do the benchmarks for this PR look like if the client decodes start using stdlib json to decode? Can we benchmark client receives of the |
Fixes #977.
Removes the redundant second JSON parse across all 20 methods affected by this issue, by decoding the response envelope and the typed payload in a single
json.Unmarshalpass instead of decoding into an intermediatejson.RawMessagethat each method then re-parsed separately.How
apiResponse.Datachanges fromjson.RawMessagetointerface{}.apiClient.Do/DoGetFallbacktake a newdata interface{}destination parameter. When a caller passes a typed pointer (e.g.&queryResult{}),json.Unmarshaldecodes directly into it via Go's existing behavior of following a non-nil pointer already stored in an interface field — no second parse. When a caller passesnil,Dofalls back to decoding into a localjson.RawMessageand returns it as[]byte, preserving the exact current behavior for any caller that only needs the raw bytes.CleanTombstonesandDeleteSeriesare unaffected — they never read the response body, so they already passnil.FormatQueryis intentionally not converted. It doesn't calljson.Unmarshalon the body at all — it does a rawstring(body)cast — so it was never affected by the double-parse this issue describes. While verifying this, I found what looks like a separate, pre-existing bug (the raw cast leaves JSON quoting/escaping in the returned string instead of decoding it) — filing that separately rather than mixing an unrelated behavior change into this PR.Why this is scoped differently from #1090
That attempt removed the buffering in
httpClient.Do(api/client.go) itself to stream fromresp.Body, which broke request cancellation handling and ended up with a benchmark that stopped measuring anything real. This PR doesn't touchapi/client.goor cancellation at all —Dostill receives a fully-buffered body exactly as before. The only change is not parsing that body twice.Benchmark
New, committed
BenchmarkQueryRangedrivingQueryRangethrough the public API over a real HTTP round trip, against a 100-series × 350-sample matrix response (~35k samples),-count=100,benchstat:RawMessagecopy and then reparses it; removing that copy is the whole effect.n=100) benchmark completely flattened the localhost HTTP round-trip noise, revealing a massive and highly consistent decode speedup.Tests
queryTeststable inTestAPIs, alongside the existing scalar case, assertingreflect.DeepEqualon the fully decodedmodel.Matrix— the path this change (and the benchmark) actually exercises. Confirmed the assertion catches regressions by manually corrupting the expected value and checking the test fails, then reverting.go build,go vet,gofmt -l ., andgo test -count=1 ./api/...are all clean.@kakkoyun — you triaged this issue originally; would appreciate a look when you have time.