Skip to content

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

Closed
jmarantz wants to merge 1 commit intoenvoyproxy:mainfrom
jmarantz:admin-chunk
Closed

WiP admin: get basic streaming working for /stats#19649
jmarantz wants to merge 1 commit intoenvoyproxy:mainfrom
jmarantz:admin-chunk

Conversation

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Jan 21, 2022

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

Commit Message: This is nowhere near ready for review but it's a basic proof-of-concept for streaming out the admin /stats endpoint using chunked encoding.

This concept can be part of the solution to resolve #16425 , #16981 , and ultimately maybe #16139 .

I think what needs to happen in this PR is to drop the stats-specific changes, as that's really better covered by starting from #19546 , and to change the interface for an Admin handler generally to have the callback create an instance of a HandlerInterface that provides an API to stream the next chunk. This will involve small changes to each existing admin handler.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@repokitteh-read-only
Copy link

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

see: more, trace.

@jmarantz jmarantz changed the title admin: get basic streaming working for /stats WiP admin: get basic streaming working for /stats Jan 21, 2022

if (response.length() > 0) {
decoder_callbacks_->encodeData(response, end_stream_on_complete_);
for (bool cont = true, first = true; cont; first = false) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern here is deeply flawed on a couple of levels. This loops over all the data, streaming it out to the client. Consider the data could be 1G. That will block the main thread until all the data gets to the client.

It also will stuff this into the network without any notion of client flow-control.

Instead we should use the exiting Envoy filter state-machine stuff to stream out a chunk at a time and return control.

See

trailers = HeaderMapOptRef(std::ref(decoder_callbacks_->addDecodedTrailers()));
(gzip decompress filter) for something that I think is doing this right.

OTOH I tested this manually and it seems to "work" in the sense that all the data comes out and is received by 'wget' via chunked encoding.

Also what we really want here is create a Handler from the callback and save it in the filter, and then call the handler for each chunk whenever encodeData is called (I think). I am not too fluent in Envoy filter streaming anymore.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 21, 2022
@jmarantz jmarantz closed this Feb 22, 2022
@jmarantz jmarantz deleted the admin-chunk branch February 22, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

slow stats endpoint and blocking /ready endpoint when hitting /stats endpoint

1 participant