[EXPORTER] Add custom HTTP client - #4071
Conversation
|
Could you please add a test to use a simple custom http client? |
|
Thanks @ltowarek. Couple of review comments as of now -
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4071 +/- ##
==========================================
+ Coverage 82.08% 82.56% +0.49%
==========================================
Files 386 389 +3
Lines 16211 16617 +406
==========================================
+ Hits 13305 13718 +413
+ Misses 2906 2899 -7
🚀 New features to boost your workflow:
|
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR. Please see the question below.
|
Thanks for the feedback! I think I've covered everything in the new patch set:
|
marcalff
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
Please consider to use:
- an abstract class
HttpClientFactory, with virtual methods forCreateandCreateSync - A child class
HttpCurlClientFactory, which implement this for CURL - a HttpCurlClientFactory::singleton
Everywhere the static method HttpClientFactory::Create() is used today, pass a HttpClientFactory instance instead, taken as input parameter.
The user of an HTTP client can then decide which implementation to use:
- pass HttpCurlClientFactory::singleton to use CURL
- pass an instance of MyOwnHttpClientFactory for alternate implementations.
It should be possible to mix CURL and non CURL HTTP clients in the same binary, for example:
- send traces using HTTP + CURL to endpoint A
- send metrics using HTTP + alternate to endpoint B
5147826 to
516c51b
Compare
@marcalff, it looks like a good idea. My main goal with the current PR is build-time configuration - to enable I think runtime changes should be a part of a separate PR which I'm open to working on. |
|
Thanks for adding the runtime factory path. I think this is moving in the right direction, but can we make the injection story consistent across all HTTP exporters? Right now Also, after making |
…/opentelemetry-cpp into fix/issue-24-custom-http-client
marcalff
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
This is good work.
I suspect GetDefaultHttpClientFactory() may need minor adjustments later, but this will be for a different PR.
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the feature! Please see feedback below to build/test the new example in CI. Once that is in and CI is passing this should be good to merge.
Replaces the libcurl cross-compile workaround (stub FindCURL.cmake, ~30 CURL_*/SIZEOF_*/HAVE_* cache variables, and the opentelemetry_http_client_curl target hijack) with WITH_HTTP_CLIENT_CURL=OFF and the new OtlpHttpExporter(options, HttpClient) constructor, passing MakeEspHttpClient() directly. Bumps third_party/opentelemetry-cpp to 9e93606d, which includes the merged open-telemetry/opentelemetry-cpp#4071.
* Remove CURL workarounds * Use WITH_HTTP_CLIENT_CURL=OFF; bump opentelemetry-cpp Replace the non-existent WITH_CUSTOM_HTTP_CLIENT flag with the correct WITH_HTTP_CLIENT_CURL=OFF, which opentelemetry-cpp uses to signal that the caller supplies its own HttpClientFactory transport. Bump third_party/opentelemetry-cpp to e568cc0 (tip of the local fix/issue-24-custom-http-client fork branch), which adds the opentelemetry_http_client interface library and the custom_http_client example that accompanies opentelemetry-cpp PR #4071. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Refactor HttpClientFactory to use EspHttpClientFactory for improved clarity and maintainability * Remove stale libcurl references from comments Update comments in the transport source, header, and README to reflect that libcurl is now excluded entirely via WITH_HTTP_CLIENT_CURL=OFF rather than replaced at the link level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Use opentelemetry-cpp#4071 custom HTTP client instead of curl Replaces the libcurl cross-compile workaround (stub FindCURL.cmake, ~30 CURL_*/SIZEOF_*/HAVE_* cache variables, and the opentelemetry_http_client_curl target hijack) with WITH_HTTP_CLIENT_CURL=OFF and the new OtlpHttpExporter(options, HttpClient) constructor, passing MakeEspHttpClient() directly. Bumps third_party/opentelemetry-cpp to 9e93606d, which includes the merged open-telemetry/opentelemetry-cpp#4071. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ccepts Main grew its own FakeResponse, FakeRequest, FakeSession and FakeHttpClient in open-telemetry#4071 and open-telemetry#4501, in an unnamed namespace, and this branch already had four of the same names in another unnamed namespace in the same file. Reopening an unnamed namespace names the same namespace, so the rebase merged both without a conflict into four redefinitions that do not compile. Merged rather than renamed. Main's set is used once and answers with one fixed response; this branch's runs a script the case supplies, which is the general case. So the session and the client take a script and default to answering the way main's did, and the existing call site is untouched. The default body had to change too, which is not cosmetic. It read `{"errors": false, "failed" : 0}`, and this pull request stops reading `failed` and starts requiring one `items` result per submitted record, so that body is now rejected: with it, ExportingARecordWithInvalidUtf8DoesNotAbort returns kFailure and fails. The default is now a body a server could send for a one record batch, and the comment that described the old substring search is rewritten. Verified in both configurations, since the guards cut different code: 26 tests, 22 passed and 4 skipped without async export, 20 passed and 6 skipped with it, no failures in either. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Main's open-telemetry#4071 and open-telemetry#4501 put FakeResponse, FakeRequest, FakeSession and FakeHttpClient in an unnamed namespace at the top of this file, and this branch already had four of those names in a second unnamed namespace lower down. Reopening an unnamed namespace names the same namespace, so the rebase merged both with no conflict at all and left four redefinitions. One set now. The session and the client take a script and default to answering the way main's did, so main's own call site needs no edit, and DeferredSession and DeferredHttpClient are untouched. The default body stays as main wrote it. This branch does not change how the exporter decides success, so what main's cases send still passes here. Verified in both configurations with maintainer mode on: 19 cases, all passing without async export, 3 passing and 16 skipping with it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Main's open-telemetry#4071 and open-telemetry#4501 put FakeResponse, FakeRequest, FakeSession and FakeHttpClient in an unnamed namespace at the top of this file, and this branch already had four of those names in a second unnamed namespace lower down. Reopening an unnamed namespace names the same namespace, so the rebase merged both with no conflict at all and left four redefinitions. One set now. The session and the client take a script and default to answering the way main's did, so main's own call site needs no edit. The script carries the handler as a shared_ptr rather than a reference, because the cases here have to keep it and send a second event to it, and the client keeps its on_create_session and on_cancel_all hooks, both empty by default. The default body stays as main wrote it. This branch does not change how the exporter decides success, so what main's cases send still passes here. Verified in both configurations with maintainer mode on: 12 cases, 12 passing with async export and 3 passing with 9 skipping without it. Removing CompleteOnce's compare and exchange turns six of them red, so the rewritten fixtures still discriminate. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Fixes #2084
Changes
Adds a way to provide a custom HTTP client instead of the default
libcurl-based client.The main motivation are embedded and cross-compiled targets where libcurl is unavailable. A working example for ESP32S3 using ESP-IDF's
esp_http_clientas the transport backend is at ltowarek/esp-opentelemetry-cpp#30.