admin: create new json streaming api and switch admin stats json format to use it#28988
admin: create new json streaming api and switch admin stats json format to use it#28988jmarantz merged 61 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…needed. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…h paging, disabling format for that block. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…r having emitted the key. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…bug-only field. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…ext. Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
I decided to simplify the streamer implementation by keeping the stack only for debug, and removing Json::Streamer::clear() -- to clear you need to destruct all objects in the proper order. This simplifies the logic and only adds a small complexity to the call-site in StatsJsonRender, to keep the json-related data in its own structure which can be cleared during StatsJsonRender::finalize(). This removed a few blocks of code and some special-cases. |
ravenblackx
left a comment
There was a problem hiding this comment.
Much nicer, thanks! Glad to see the back of the double-close support. :)
|
Thanks! Greg can you do a senior pass? |
|
/retest |
1 similar comment
|
/retest |
ggreenway
left a comment
There was a problem hiding this comment.
I like this approach!
/wait
source/server/admin/stats_render.cc
Outdated
| delim_ = ","; | ||
| ASSERT(!histograms_initialized_); | ||
| json_->stats_array_->addMap()->addEntries( | ||
| {{"name", name}, {"value", static_cast<double>(value)}}); |
There was a problem hiding this comment.
I think there could be loss of precision by converting to double. I think it would be better to add native int64/uint64 support to the json streamer.
There was a problem hiding this comment.
Great catch! If this is consumed by JavaScript this doesn't matter, I scanned JSON docs and it does not specify what the data type is in any language; just the syntax. So if we have another language reading json it could read high-precision integers and we should write them.
This also I think will be slightly faster.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…sion during serialization. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
@ravenblackx @ggreenway need another stamp -- I had to use hex to specify very big integers in the test to get gcc to compile. |
|
Thanks for the great feedback, Greg & Raven! |
A performance regression was found in a late commit as part of #28988 where we switched from a low-precision fast serialization of doubles (absl::StrCat) to a high-precision slow serialization of doubles (absl::StrFormat("%.15g", ...). This resolves that issue using std::to_chars which is faster than absl::StrCat and is also high precision. The downside is it doesn't work on all platforms, so an intermediate high-precision medium-speed solution was found for Apple and GCC. The serialize-double-to-buffer functionality is moved out of Json::Streamer and into a new Buffer::Util namespace. Latest Json histogram serialization performance: ``` ------------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------------ BM_HistogramsJson 17.3 ms 17.3 ms 41 Signed-off-by: Joshua Marantz <jmarantz@google.com>
Commit Message: When admin endpoint /stats?format=json is requested, all histograms are copied into a protobuf structure for subsequent serialization via the protobuf json serializer.
This approach scales poorly with large numbers of clusters or listeners, as every byte of histogram information must be copied into memory twice: Once when copying into the protobuf structure, and once when serializing into bytes. With the new 'detailed' histograms needed for graphically rendering them into /stats?format=html, this can represent a significant burst of memory and CPU time, and might risk an OOM.
This change removes the protobuf as an intermediate representation, and incorporates a streaming JSON serializer, which meshes well with the http streaming model used by stats. This methodology decreases the CPU time spent in a new histograms benchmark by almost 15x (249ms per hundred histograms to 17ms per hundred histograms).
The new serialization class is fairly simple -- its value-add is to simply keep track of punctuation across nested arrays and classes, and otherwise transfers API calls like adding a number or string into the response buffer.
Additional Description:
Risk Level: low
Testing: The existing stats_render_test, a new json serializer test, plus test/integration/admin_html/web_test.sh
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a