admin: Streaming /stats implementation

[jmarantz]

Commit Message: Implements streaming /stats API for JSON and Text. Prometheus can be done in a follow-up.

Additional Description:

The speed up with this algorithm is dramatic for the "/stats" endpoint on 10k clusters, improving a 1.76 second CPU/memory burst into less than half a second, and speeding up JSON delivery from 5.2 seconds to 2.5 seconds. There is still a bottleneck is the JSON serialization library, which is needed to ensure we have proper escaping of stat-names with special characters, and a larger bottleneck whenever regex filers are used because std::regex has poor performance.

In the future, better mechanisms for filtering can be added, such as an HTML UI for navigating stats by name hierarchy (#18670) or prefix/suffix matching or using RE2.

This PR fully resolves #16981 but does not address #16139 at all. It might be possible to generalize the solution here to cover Prometheus by keeping our statss_map_ indexed by tagExtractedStatName, but that would be a whole separate effort, and we'd have to figure out if related-stats could span scopes.

This PR partially resolves #18675 but there's still a non-trivial burst of CPU especially when there's a regex filter. #18670 would (arguably) fully resolve #18675.

BEFORE:

------------------------------------------------------------------
Benchmark                        Time             CPU   Iterations
------------------------------------------------------------------
BM_AllCountersText            1760 ms         1760 ms            1
BM_UsedCountersText            118 ms          118 ms            6
BM_FilteredCountersText       4290 ms         4289 ms            1
BM_AllCountersJson            5221 ms         5218 ms            1
BM_UsedCountersJson            152 ms          152 ms            5
BM_FilteredCountersJson       4162 ms         4161 ms            1

AFTER

------------------------------------------------------------------
Benchmark                        Time             CPU   Iterations
------------------------------------------------------------------
BM_AllCountersText             454 ms          454 ms            2
BM_UsedCountersText           37.7 ms         37.7 ms           19
BM_FilteredCountersText       4056 ms         4055 ms            1
BM_AllCountersJson            2707 ms         2706 ms            1
BM_UsedCountersJson           63.4 ms         63.4 ms           11
BM_FilteredCountersJson       4008 ms         4008 ms            1

I also ran a manual test with 100k clusters sending /stats to the admin port with output to /dev/null. Before this change, this takes 35 seconds and after this change it takes 7 seconds, and consumes about 2G less memory. These were measured with time wget ... and observing the process with top. To run that I had to set up the clusters to reference static IP addresses and make the stats-sink interval 299 seconds, as others DNS resolution and stats sinks dominate the CPU and make other measurements difficult.

Risk Level: medium -- the stats generation code in admin is largely rewritten and is important in some flows.
Testing: //test/...
Docs Changes: n/a -- no functional changes in this PR -- just perf improvements
Release Notes: n/a
Platform Specific Features: n/a
Fixes: #16981

[repokitteh-read-only]

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

see: more, trace.

[ggreenway]

Drive-by comment to start: I think it's important that by default, all the stats are sampled as close together in time as possible. If the stats are sampled over a long period of time (if there's a slow client reading them), it's hard to reason about the relative values. From a quick glance through this code, I think it would still work fine if by default it uses a ChunkSize of infinity (or UINT64_MAX).

[jmarantz]

Thanks @ggreenway that's a good point. But sampling all the stats quickly requires storing them in a large map, which requires visiting all of them, which with a large # of clusters can take a pretty significant chunk of CPU time wall-clock time, and memory. E.g. with 10k clusters it takes 2 seconds to wget the /stats endpoint without this change, and about 0.5 seconds with it. So you might not be getting that good a correlation even now, and you are also potentially impacting other uses of the main thread.

I also tried 100k clusters and with this PR it's 15 seconds and without it it's about 3.5 minutes. It gets non-linear because currently we have to do a flat sort across the entire set of stats. Moreover there's a memory burst measured in gigabytes, whereas when we chunk we get to bound the memory we use. I'll try to capture that data also.

I'll see if I can get some timing though for how long it takes to sample all the stats today. It's not optimal even now as we store the stats in a std::map<string,int> which means we are paying for keeping it sorted while sampling. We could sample into a hash_map<CounterSharedPtr,int> which would collect faster. Then we could sort and stream the data out.

WDYT of having /stats have the behavior you suggest, where by default we traverse all the stats and capture their values, but having (in a later PR) the default behavior hitting the "stats" button in the admin home page be to visit only a chunks worth of stats? Or...have it fast by default but a query-param to use if you want to pay for minimizing skew?

Is skew between different stats you collect an issue for you right now?

Also would a more sophisticated filter mechanism help, beyond a simple regex? If you could express (say) 1000 stat of interest those would all come out in one chunk anyway.

[jmarantz]

@ggreenway I measured the time it takes to run block of code where we sample the values with 10k clusters at HEAD -- it takes 2 seconds which is 4x as long as the new system takes to do the entire request. So I think the proposed changes here improve skew, unless there is a slow client, relative to the current system.

However I think skew could be improved in the current system by capturing the values in an array first and then sorting it.

One possible direction is to have a query-param to disable chunked encoding so a user can minimize skew at the cost of memory, and we can make the default chunk size pretty high so we will get minimal skew within each chunk, and typical servers will only get one chunk. WDYT?

[ggreenway]

Capturing in an array and then sorting makes a lot of sense; latching all the values as quickly as possible results in better stats consistency.

I think it's undesirable for the consistency of the stats to change depending on admin-client's network conditions, even if it's faster when on a fast network. Possible example (although I'm not sure if this works as I think it might): curl localhost:<admin port>/stats | less. I wouldn't expect the stats to be latched as I page through the output (assuming curl applies backpressure when stdout is blocked).

[jmarantz]

Greg: WDYT of initially going forward with this approach, which improves skew relative to the current code because:

• this is algorithmically faster because it incrementally sort one cluster at a time, so a bunch of really small sorts rather than a giant one
• in the current state we are generating a chunked encoding but we are not actually streaming with flow-control yet, I think, because we are just calling write multiple times with each chunk, without returning control to the user. Later we can make this use the filter architecture better to do real streaming. We are getting the algorithmic advantage though, and also much lower peak memory.

As a follow-up, if we think the skew needs to be improved further at the expense of peak memory and overall speed, we can go back to collecting a huge array, maybe with a query-param.

[jmarantz]

With the current state of this PR, no flow-control happens, so wget -q -O - localhost:8080/stats | less on 10k clusters shows me the first page of 100Mbytes worth of stat output, but as far as Envoy is concerned the request is complete. I know this because the admin requests come on main thread and while I've got less waiting for me to go to the next page, I can open up a new terminal and make more admin requests which respond immediately.

I'm not sure where that 100M gets buffered, but there is nothing in the current code to block it from capturing all the data and streaming it. It all completes in the (ugly) loop I added to AdminFilter::onComplete()..

The thing that's ugly about it (and what IMO prevents this PR from being reviewable) is that it keeps calling the handler callback until it returns something other than HTTP 100. The 100 never makes it to the wire. This should instead be a handler class with a constructor that fills the response-headers, and nextChunk() API that returns true/false, rather than the 100-hack.

I think from a memory usage perspective this should ultimately allow flow-controlled chunks but that could be a separate PR with its own debate, and maybe a flag, as you'd trade off skew vs memory consumption.

[ggreenway]

This looks great, aside from the (probably accidental?) bazel change.

/wait

[ggreenway]

This shouldn't be part of this PR

removed the accidental modification to the bazel version file.

[jmarantz]

/retest

[repokitteh-read-only]

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

🐱

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

see: more, trace.