admin: Richer HTML home page with forms for params by jmarantz · Pull Request #19546 · envoyproxy/envoy

jmarantz · 2022-01-14T03:51:52Z

Commit Message:
High level: creates an HTML UI to tweak query params for admin endpoints.

This PR covers a few things and could be broken up.

adds ParamDescriptor abstraction and gives every endpoint a vector of those so the query-params can be specified before the admin page is visited.
Re-organizes CSS and HTML fragments so they are in their own files. This makes it easy to iterate on them as editors can use the right syntax-highlighting each file type, and you can hack the generator script during development to reference the file from a local server so you can iterate on the web markup without recompiling/relinking C++ or even restarting the server.
Enable adjusting query-params for admin points from the admin home page:

Adds an "html" format option for /stats and allow incremental adjustment of settings from that view also.

Adds selection of types (text-readouts, gauges, counters, histograms, or All) within /stats.

Some of these changes could be broken out as their own PRs if this is too large to review.

Additional Description:
This is was broken out from #18670 which will enable a hierarchical view of the stat names, made much more efficient by employing Stats::Scope to bound the traversal that must be done by the server.
Risk Level: medium -- this adds a novel new mechanism to build HTML and CSS files into the C++ binary as string constants, to simplify deployment. This appears to pass CI on non-Unix platforms so I think this is correct.
Tests: //test/server/..., manually clicking around admin page, and validating HTML with https://validator.w3.org/nu/#textarea
Docs Changes: will be needed in this PR but would like to get traction on code before adding that.
Platform Specific Features: n/a

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

repokitteh-read-only · 2022-01-14T03:51:55Z

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: #19546 was opened by jmarantz.

see: more, trace.

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

…ion. This requires a carefully defined set that does not proactively allocate and deallocate the set element. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

… StatName, unfortunately. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

…rtsWith (#19563) Commit Message: Improvements in the admin panel (#19546 #18670) put greater pressure on sorting of stats via StatName, as opposed to saving elaborated strings for every stat and sorting by that. This new implementation provides an iterator-like interface for decoding the tokens in StatName, enabling us to early-exit when comparing stat names with multiple tokens. For example, if you are comparing "a.b.c.d" against "a.x.y.z" we can abort out after the "b" vs "x" comparison, and there is no need to compare "c" to "y". Of course it's not as fast as comparing strings, but it saves having to hold the elaborated strings in memory. It also adds a new sortByStatNames free function which is more efficient than calling std::sort directly because it can take the symbol-table lock once for the whole sort, rather than re-taking the lock on each comparison. Taking uncontended locks is fast, but as the benchmark shows, it's not as fast as not taking locks. Additional Description: A benchmark for this is added in this PR: OLD: ``` ------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------ bmCompareElements 174 ns 174 ns 4028127 bmStdSort 318591243 ns 318515288 ns 2 ``` NEW: ``` ------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------ bmCompareElements 55.9 ns 55.9 ns 12515696 bmSortByStatNames 61342554 ns 61329512 ns 11 bmStdSort 80578659 ns 80562977 ns 9 ``` The raw CompareElements numbers show the raw speed improvement offered by the early-exit as well as refraining from creating temp vectors, but count on 20M compares to sort 1M stats. The sort numbers provide more context. in the old code, sorting 100k stats (from 1k clusters) takes 318ms (manually testing 1M stats is around 2.8 seconds but I didn't want to have CI run such a long test each time). Doing that using std::sort with the new comparison algorithm is 81ms and with the optimized sortByStatNames is 61ms. The burst of main-thread activity due to an admin /stats request may impact long-tail data-plane latency on a heavily loaded system. which is why in #18670 we segment into scopes. In the meantime, this reduces the speed impact of sorting. Risk Level: medium -- hopefully the existing unit tests covered all interesting corner cases Testing: //test/common/stats/... plus the new benchmark Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

…rtsWith (envoyproxy#19563) Commit Message: Improvements in the admin panel (envoyproxy#19546 envoyproxy#18670) put greater pressure on sorting of stats via StatName, as opposed to saving elaborated strings for every stat and sorting by that. This new implementation provides an iterator-like interface for decoding the tokens in StatName, enabling us to early-exit when comparing stat names with multiple tokens. For example, if you are comparing "a.b.c.d" against "a.x.y.z" we can abort out after the "b" vs "x" comparison, and there is no need to compare "c" to "y". Of course it's not as fast as comparing strings, but it saves having to hold the elaborated strings in memory. It also adds a new sortByStatNames free function which is more efficient than calling std::sort directly because it can take the symbol-table lock once for the whole sort, rather than re-taking the lock on each comparison. Taking uncontended locks is fast, but as the benchmark shows, it's not as fast as not taking locks. Additional Description: A benchmark for this is added in this PR: OLD: ``` ------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------ bmCompareElements 174 ns 174 ns 4028127 bmStdSort 318591243 ns 318515288 ns 2 ``` NEW: ``` ------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------ bmCompareElements 55.9 ns 55.9 ns 12515696 bmSortByStatNames 61342554 ns 61329512 ns 11 bmStdSort 80578659 ns 80562977 ns 9 ``` The raw CompareElements numbers show the raw speed improvement offered by the early-exit as well as refraining from creating temp vectors, but count on 20M compares to sort 1M stats. The sort numbers provide more context. in the old code, sorting 100k stats (from 1k clusters) takes 318ms (manually testing 1M stats is around 2.8 seconds but I didn't want to have CI run such a long test each time). Doing that using std::sort with the new comparison algorithm is 81ms and with the optimized sortByStatNames is 61ms. The burst of main-thread activity due to an admin /stats request may impact long-tail data-plane latency on a heavily loaded system. which is why in envoyproxy#18670 we segment into scopes. In the meantime, this reduces the speed impact of sorting. Risk Level: medium -- hopefully the existing unit tests covered all interesting corner cases Testing: //test/common/stats/... plus the new benchmark Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Josh Perry <josh.perry@mx.com>

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

jmarantz · 2022-07-19T17:01:19Z

thanks @Mythra !

pradeepcrao

Thank you so much for doing this!

I only have a few nits after a first pass. Doing a more detailed review now.

source/server/admin/stats_html_render.cc

source/server/admin/stats_params.h

source/server/admin/stats_params.cc

source/server/admin/stats_params.h

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

pradeepcrao · 2022-07-25T13:29:15Z

test/server/admin/admin_test.cc

+      filter: Regular expression (ecmascript) for filtering stats
+      format: Format to use; One of html text json
+      type: Stat types to include.; One of All Counters Histograms Gauges TextReadouts
+      histogram_buckets: Histogram bucket display mode; One of cumulative disjoint none


It's not immediately apparent to me that cumulative disjoint and none are enum choices. Is there a way to make this clearer when printing the help text?

How about "One of (cumulative, disjoint, none)"

looks good (I am guessing you meant to put only 1 comma after cumulative)

pradeepcrao · 2022-07-25T14:07:55Z

source/server/admin/admin.cc

                      MAKE_ADMIN_HANDLER(clusters_handler_.handlerClusters), false, false),
-          makeHandler("/config_dump", "dump current Envoy configs (experimental)",
-                      MAKE_ADMIN_HANDLER(config_dump_handler_.handlerConfigDump), false, false),
+          makeHandler(


Does it make sense to have an assert in the constructor body that there are no duplicate prefixes in handlers_ ?

pradeepcrao · 2022-07-25T14:26:51Z

source/server/admin/admin.cc

+          makeHandler(
+              "/config_dump", "dump current Envoy configs (experimental)",
+              MAKE_ADMIN_HANDLER(config_dump_handler_.handlerConfigDump), false, false,
+              {{Admin::ParamDescriptor::Type::String, "resource", "The resource to dump"},


This is missing certain parameters listed on the doc page:
https://www.envoyproxy.io/docs/envoy/latest/operations/admin#get--config_dump?include_eds

I realized that and was thinking of filling in all the new parameters that were added since I first wrote this code as a follow up. However I'm glad you asked about this because this brings a new complexity. /logging is a mutating operation, and thus must be issued as a POST. POST params are transmitted from the browser to the server as POST params in the request body rather than query-params, and there's not currently a path to get get them into the admin handlers. I'm going to have to iterate on this a bit.

I'll get that sorted before asking for the next round of comments.

/wait

I decided to back out my addition of POST params in this PR and will follow up. I got this working but it was too hacky. I need to make some infrastructural changes to get POST params to work well, and also to handling the "/logging" semantics for an optional enum param.

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

jmarantz

OK I think this is ready for another round of review.

A follow-up will be needed to finish the POST params.

jmarantz · 2022-07-26T13:12:40Z

source/server/admin/admin.cc

+          makeHandler(
+              "/config_dump", "dump current Envoy configs (experimental)",
+              MAKE_ADMIN_HANDLER(config_dump_handler_.handlerConfigDump), false, false,
+              {{Admin::ParamDescriptor::Type::String, "resource", "The resource to dump"},


I decided to back out my addition of POST params in this PR and will follow up. I got this working but it was too hacky. I need to make some infrastructural changes to get POST params to work well, and also to handling the "/logging" semantics for an optional enum param.

pradeepcrao

LGTM

pradeepcrao · 2022-07-27T17:45:42Z

source/server/admin/utils.cc

@@ -63,16 +63,15 @@ absl::Status histogramBucketsParam(const Http::Utility::QueryParams& params,
                                   HistogramBucketsMode& histogram_buckets_mode) {


nit: Not part of this PR, but moving the comments for the methods in this file to the header might make them easier to use.

pradeepcrao · 2022-07-27T18:52:22Z

test/server/admin/stats_handler_test.cc

+      EXPECT_THAT(code_response.second, Not(HasSubstr(not_expect))) << "params=" << params;
+    }
+  };
+  test("",


Commit Message: Make the HTML view for stats a compile-time option, defaulting to on. This PR doesn't change any code-paths, but it moves the HTML support into a separate file, compiled into //source/server/admin:admin_lib. In the future, and admin_html_lib will be added, built only when admin-html is not disabled. That will allow envoyproxy/envoy#19546 and ultimately envoyproxy/envoy#18670 to proceed. To disable, compile with `--define=admin_html=disabled`, in which case the admin `"/"` endpoint will just print that the thml mode was disabled. Additional Description: This PR is a step toward solving envoyproxy/envoy#18675 Risk Level: low -- this just moves code around and doesn't change it at all. Testing: //test/..., both with and without `--define=admin_html=disabled` Docs Changes: included Release Notes: included Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

initial patch

c4101e1

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

jmarantz added 6 commits January 14, 2022 09:28

get tests working

29cf776

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

Add parameters for other admin endpoints. Still need runtime_modify.

94c97c0

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

add coverage for the HTML generator

aa00670

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

coverage

34c7607

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

coverage and doc

4e821b4

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

cover html rendering

8cbcf36

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

jmarantz mentioned this pull request Jan 16, 2022

stats: faster versions of SymbolTableImpl::lessThan and StatName::startsWith #19563

Merged

revert pointless name-change and rewrite comment.

ff2aaf9

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

jmarantz marked this pull request as ready for review January 16, 2022 18:25

jmarantz added 4 commits January 16, 2022 13:36

remove unused styles

1e4d606

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

test cleanup

256b6c9

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

Merge branch 'main' into admin-params

49212f6

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

Remove the intermediate vectors for stats by using forEach for insert…

ba5a1dc

…ion. This requires a carefully defined set that does not proactively allocate and deallocate the set element. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz marked this pull request as draft January 18, 2022 04:05

jmarantz added 2 commits January 18, 2022 20:21

Merge branch 'main' into admin-params

c78b6a4

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

Go back to sorting by strings as it's somewhat faster than sorting by…

33b2f22

… StatName, unfortunately. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Jan 19, 2022

stats: add ability to view histogram buckets to the /stats endpoint #19586

Merged

add in ifdef'd version where we do the sort in StaName space.

efe142e

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

jmarantz mentioned this pull request Jan 21, 2022

WiP admin: get basic streaming working for /stats #19649

Closed

jmarantz changed the title ~~admin: enrich the admin UI by prompting for params in the home page~~ WiP admin: enrich the admin UI by prompting for params in the home page Feb 11, 2022

jmarantz changed the title ~~WiP admin: enrich the admin UI by prompting for params in the home page~~ WiP admin: Richer HTML home page with forms for params Feb 22, 2022

jmarantz added 4 commits February 26, 2022 09:28

Merge branch 'main' into admin-params

78091d1

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

get build working

119270c

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

fix respones code expectation

68a89d2

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

cleanup

70ce924

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

jmarantz assigned pradeepcrao and unassigned ggreenway Jul 20, 2022

pradeepcrao reviewed Jul 21, 2022

View reviewed changes

jmarantz added 2 commits July 21, 2022 21:25

review comments

0859243

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

Merge branch 'main' into admin-params

603f0f8

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

jmarantz dismissed Mythra’s stale review via 603f0f8 July 22, 2022 01:26

phlax assigned mattklein123 Jul 22, 2022

pradeepcrao reviewed Jul 25, 2022

View reviewed changes

jmarantz added 3 commits July 25, 2022 16:18

Merge branch 'main' into admin-params

1cc67ca

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

add missing params (round 1)

ee21f74

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

add remaining missing admin query params.

379b316

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

repokitteh-read-only bot added the waiting label Jul 26, 2022

jmarantz added 2 commits July 26, 2022 08:53

back out post support for now

a7b3565

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

back out more post params

c456954

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

repokitteh-read-only bot removed the waiting label Jul 26, 2022

format

8c3af8b

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

jmarantz commented Jul 26, 2022

View reviewed changes

pradeepcrao reviewed Jul 27, 2022

View reviewed changes

jmarantz assigned ggreenway and yanavlasov and unassigned ggreenway and mattklein123 Jul 27, 2022

yanavlasov approved these changes Jul 29, 2022

View reviewed changes

yanavlasov merged commit 8e64f2b into envoyproxy:main Jul 29, 2022

jmarantz deleted the admin-params branch July 29, 2022 20:27

jmarantz mentioned this pull request Aug 1, 2022

admin: support HTML UI for admin post params #22424

Merged

		@@ -63,16 +63,15 @@ absl::Status histogramBucketsParam(const Http::Utility::QueryParams& params,
		HistogramBucketsMode& histogram_buckets_mode) {

Conversation

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

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 14, 2022

Uh oh!

jmarantz commented Jul 19, 2022

Uh oh!

pradeepcrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Jul 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Jul 26, 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!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradeepcrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jmarantz commented Jan 14, 2022 •

edited

Loading

jmarantz Jul 25, 2022 •

edited

Loading

jmarantz Jul 26, 2022 •

edited

Loading