switch to using json sanitizer for admin counters, gauges, and text-readouts by jmarantz · Pull Request #20745 · envoyproxy/envoy

jmarantz · 2022-04-08T21:21:29Z

Commit Message: Uses the new Json::sanitizer from #20637 to generate counters, gauges, and text-readouts in response to admin endpoint /stats?format=json, resulting in a 5.6x speedup for AllCountersJson, and making JSON output only around 10% slower than text output.

Additional Description:
Before:

------------------------------------------------------------------
Benchmark                        Time             CPU   Iterations
------------------------------------------------------------------
BM_AllCountersText             454 ms          453 ms            2
BM_UsedCountersText           35.8 ms         35.8 ms           20
BM_FilteredCountersText       1806 ms         1805 ms            1
BM_AllCountersJson            2824 ms         2823 ms            1
BM_UsedCountersJson           61.8 ms         61.8 ms           11
BM_FilteredCountersJson       1798 ms         1798 ms            1

after

------------------------------------------------------------------
Benchmark                        Time             CPU   Iterations
------------------------------------------------------------------
BM_AllCountersText             458 ms          458 ms            2
BM_UsedCountersText           36.1 ms         36.1 ms           19
BM_FilteredCountersText       1790 ms         1790 ms            1
BM_AllCountersJson             496 ms          496 ms            2
BM_UsedCountersJson           36.4 ms         36.4 ms           19
BM_FilteredCountersJson       1789 ms         1789 ms            1

Risk Level: low -- Json::sanitizer is new, but based on nlohmann, and is differentially fuzzed vs protobuf serialization
Testing: //test/server/admin/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

…eadouts Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only · 2022-04-08T21:21:33Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #20745 was opened by jmarantz.

see: more, trace.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2022-04-10T00:19:45Z

/retest

repokitteh-read-only · 2022-04-10T00:19:49Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20745 (comment) was created by @jmarantz.

see: more, trace.

wbpcode

LGTM overall. Only one nit comment.

wbpcode · 2022-04-11T03:39:40Z

source/server/admin/stats_render.h

  std::unique_ptr<ProtobufWkt::ListValue> histogram_array_;
  bool found_used_histogram_{false};
-  bool first_{true};
+  absl::string_view delim_{""};


nit: absl::string_view delim_;

nope; need to init to a valid pointer as this is used in Buffer::addFragments which passes .data() to memcpy, and having nullptr for the src arg fails ASAN even if size==0 :)

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2022-04-11T14:30:19Z

/retest

repokitteh-read-only · 2022-04-11T14:30:23Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20745 (comment) was created by @jmarantz.

see: more, trace.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

wbpcode

LGTM.

wbpcode · 2022-04-12T01:50:18Z

cc @envoyproxy/senior-maintainers for merge or second pass for this PR that contains some core code updates.

zuercher · 2022-04-12T18:36:49Z

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2022-04-12T18:36:52Z

@envoyproxy/senior-maintainers assignee is @zuercher

🐱

Caused by: a #20745 (comment) was created by @zuercher.

see: more, trace.

zuercher · 2022-04-12T18:38:31Z

@jmarantz I'll leave the merge to you

…eadouts (envoyproxy#20745) Commit Message: Uses the new Json::sanitizer from envoyproxy#20637 to generate counters, gauges, and text-readouts in response to admin endpoint `/stats?format=json`, resulting in a 5.6x speedup for AllCountersJson, and making JSON output only around 10% slower than text output. Additional Description: Before: ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 454 ms 453 ms 2 BM_UsedCountersText 35.8 ms 35.8 ms 20 BM_FilteredCountersText 1806 ms 1805 ms 1 BM_AllCountersJson 2824 ms 2823 ms 1 BM_UsedCountersJson 61.8 ms 61.8 ms 11 BM_FilteredCountersJson 1798 ms 1798 ms 1 ``` after ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 458 ms 458 ms 2 BM_UsedCountersText 36.1 ms 36.1 ms 19 BM_FilteredCountersText 1790 ms 1790 ms 1 BM_AllCountersJson 496 ms 496 ms 2 BM_UsedCountersJson 36.4 ms 36.4 ms 19 BM_FilteredCountersJson 1789 ms 1789 ms 1 ``` Risk Level: low -- Json::sanitizer is new, but based on nlohmann, and is differentially fuzzed vs protobuf serialization Testing: //test/server/admin/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> ogle.com> Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>

…eadouts (envoyproxy#20745) Commit Message: Uses the new Json::sanitizer from envoyproxy#20637 to generate counters, gauges, and text-readouts in response to admin endpoint `/stats?format=json`, resulting in a 5.6x speedup for AllCountersJson, and making JSON output only around 10% slower than text output. Additional Description: Before: ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 454 ms 453 ms 2 BM_UsedCountersText 35.8 ms 35.8 ms 20 BM_FilteredCountersText 1806 ms 1805 ms 1 BM_AllCountersJson 2824 ms 2823 ms 1 BM_UsedCountersJson 61.8 ms 61.8 ms 11 BM_FilteredCountersJson 1798 ms 1798 ms 1 ``` after ``` ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_AllCountersText 458 ms 458 ms 2 BM_UsedCountersText 36.1 ms 36.1 ms 19 BM_FilteredCountersText 1790 ms 1790 ms 1 BM_AllCountersJson 496 ms 496 ms 2 BM_UsedCountersJson 36.4 ms 36.4 ms 19 BM_FilteredCountersJson 1789 ms 1789 ms 1 ``` Risk Level: low -- Json::sanitizer is new, but based on nlohmann, and is differentially fuzzed vs protobuf serialization Testing: //test/server/admin/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> ogle.com>

switch to using json sanitizer for admin counters, gauges, and text-r…

0e66147

…eadouts Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 2 commits April 8, 2022 18:58

remove unused constant

2e260b4

Signed-off-by: Joshua Marantz <jmarantz@google.com>

clean up and simplfy

85942e3

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~switch to using json sanitizer for admin counters, gauges, and text-r…~~ switch to using json sanitizer for admin counters, gauges, and text-readouts Apr 9, 2022

jmarantz added 2 commits April 9, 2022 00:02

format

429b4fc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

reformat based on clang 12.

2068644

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz marked this pull request as ready for review April 10, 2022 02:55

wbpcode self-assigned this Apr 11, 2022

wbpcode reviewed Apr 11, 2022

View reviewed changes

review comments.

4683f6f

Signed-off-by: Joshua Marantz <jmarantz@google.com>

revert previous commit -- ASAN doesn't like memcpy(dst, nullptr, 0).

81f15d3

Signed-off-by: Joshua Marantz <jmarantz@google.com>

wbpcode approved these changes Apr 12, 2022

View reviewed changes

repokitteh-read-only bot assigned zuercher Apr 12, 2022

zuercher approved these changes Apr 12, 2022

View reviewed changes

jmarantz merged commit 18acdb0 into envoyproxy:main Apr 12, 2022

jmarantz deleted the json-sanitize-admin-stats branch April 12, 2022 20:03

Conversation

jmarantz commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 8, 2022

Uh oh!

jmarantz commented Apr 10, 2022

Uh oh!

repokitteh-read-only bot commented Apr 10, 2022

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

jmarantz Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Apr 11, 2022

Uh oh!

repokitteh-read-only bot commented Apr 11, 2022

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Apr 12, 2022

Uh oh!

zuercher commented Apr 12, 2022

Uh oh!

repokitteh-read-only bot commented Apr 12, 2022

Uh oh!

zuercher commented Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmarantz commented Apr 8, 2022 •

edited

Loading

jmarantz Apr 11, 2022 •

edited

Loading