[DESIGN] HTTP transport requirements and options

[thc1006]

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 decide where retry belongs, which is one of the open decisions at the end. Whether HttpOperation survives is written down as @owent's preference with the evidence for it, not as a conclusion.

What is in it

The three shapes named in the discussion, compared against the criteria named with them: throughput and connection reuse, cancellation, shutdown, who owns admission and retry, connection and stream scheduling, and what happens to the installed interfaces.

Before that, a table of what every in-tree consumer does today, read from main rather than from memory. That table is the load-bearing part of the argument, so here is where each row came from:

• Zipkin takes a HttpClientSync, in zipkin_exporter.h and its factory.
• Elasticsearch calls handler->waitForResponse() in Export, in es_log_record_exporter.cc.
• OtlpHttpClient::Export takes a max_running_requests budget, and its own comment says that only a sync export, meaning zero, needs to capture the result of the callback. The default path passes zero and waits.
• OTELCPP_WITH_ASYNC_EXPORT_PREVIEW is OFF by default in the top level CMakeLists.txt.

So in the default configuration 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 first of the two facts the document turns on. The second is @owent's production evidence in #4448, which is why the document does not conclude that the asynchronous machinery should go.

What is already settled

Three questions are recorded as settled rather than left open, on the strength of @owent's replies on #4448:

• The existing max_concurrent_requests and max_requests_per_connection are legacy curl compatibility controls. A replacement is not bound by their semantics, only by the goals behind them.
• A bounded governor needs both a count and a retained-byte dimension. 64 in flight at the 4 MB of the reported workload is 256 MB of request payload before responses or retries.
• Session::SendRequest was never meant to be called more than once. That is the contract violated by the use after free I measured on main and reported in Proposal: Redesign the curl HttpClient API to address thread-safety and lifetime issues #4448.

One shared CURLM is now written as the preferred default for the curl backend rather than one option among three, with its sharing scope left to the benchmark. What that sharing costs has its own section: a reference counted runtime beneath separate logical clients rather than a global HttpClient, a hierarchical budget, a fairness requirement, and a shared failure domain.

Two sections come from reading the reports together rather than from the discussion. An export operation and an HTTP attempt are given separate state sequences, because a retried operation makes a second attempt with its own deadline, handle and result, and four of the open reports are about points where that second sequence is not respected today. The other section names what a transport rewrite does not fix: processor batching in #4449 and #4498, which decides the request geometry the transport is handed and so gates the benchmark; the untimed join in #4532, so a deadline honoured by every attempt still does not bound Shutdown; and #4534, a throw crossing a noexcept callback, which the completion contract has to answer for even where there are no exceptions to catch.

What is deliberately not in it

No requirement numbering scheme and no migration plan. You asked for something short, and the open decisions at the end are what has to be settled before either is worth writing.

The bug to invariant mapping is not repeated here. It is the matrix already in the discussion thread, which the document points back to, and which 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.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.47%. Comparing base (312acb6) to head (163d7ca).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4458   +/-   ##
=======================================
  Coverage   83.47%   83.47%           
=======================================
  Files         522      522           
  Lines       20440    20440           
=======================================
  Hits        17061    17061           
  Misses       3379     3379           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.