[DESIGN] HTTP transport requirements and options - #4458
Draft
thc1006 wants to merge 1 commit into
Draft
Conversation
Written for the question in open-telemetry#4448: does the HTTP transport need to manage asynchronous concurrency itself, or is a request and result model enough with concurrency owned somewhere else. Sets out what every in-tree consumer actually does, where the current model is inconsistent with itself, and compares the three shapes named in the discussion: the current Session and EventHandler model, an implementation neutral request and result, and a single owner CURLM event loop for the case where transport level concurrency is wanted. It deliberately does not decide whether HttpOperation should exist, whether a Session carries one request, or which layer owns retry. Those are listed as decisions instead, together with the CMake and Bazel surfaces that disagree about the concrete curl headers today. The evidence is the invariant matrix already posted in open-telemetry#4448, linked rather than repeated, so this stays short enough to read in one sitting. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the requirements and options document @lalitb asked for in #4448, as a pull request so it can be reviewed before anyone chooses an implementation direction.
It answers the question that was actually asked, which is whether the HTTP transport needs to manage asynchronous concurrency itself or whether a request and result model is enough with concurrency owned somewhere else. It does not propose class names, and it does not assume that
HttpOperationhas to go, that aSessioncarries one request, or where retry belongs. Those are listed as decisions.What is in it
The three shapes named in the discussion, compared against the criteria named with them: throughput and connection reuse, cancellation and shutdown, who owns retry and concurrency, and compatibility with the installed interfaces and custom clients.
Before that, a table of what every in-tree consumer does today, read from
mainrather than from memory. That table is the load-bearing part of the argument, so here is where each row came from:HttpClientSync, inzipkin_exporter.hand its factory.handler->waitForResponse()inExport, ates_log_record_exporter.cc:475.OtlpHttpClient::Exporttakes amax_running_requestsbudget, and its own comment says that zero means a synchronous export; the default path passes zero and waits for the result.WITH_ASYNC_EXPORT_PREVIEWisOFFatCMakeLists.txt:241.So in the configuration almost everyone builds, every exporter blocks until its request is done, and two of the three do it by handing the request to an asynchronous client and then waiting. That is the fact the rest of the document turns on.
What is deliberately not in it
No requirement numbering scheme, no benchmark matrix, no migration plan. You asked for something short, and the six decisions at the end are the part that has to be settled before any of that is worth writing.
The bug to invariant mapping is not repeated here either. It is the matrix already in the discussion thread, linked from the document, which also marks which reports are measured and which are only reasoned.
On the recommendation section
There is one, it is labelled as an opinion, and it is meant to be argued with rather than accepted. Short version: define the one attempt semantics as the contract, keep the current interface working through an adapter, and treat the single owner event loop as the curl backend's implementation for the case where concurrency is actually wanted. I would also want the concurrency question settled by a measurement rather than by assumption.
If the shape of the document is useful but the recommendation is not, the recommendation is one section and can go.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes