Sending sync request through http_client #448

lalitb · 2020-12-14T15:29:06Z

Currently, http_client api supports sending async http requests, with error and response received through callback. For exporters, we need to have the status of http request returned from export function ( refer - #444 (comment) ) . Adding a method to send sync request in client interface, along with it's curl implementation.

Summary of changes:
2. Split SessionManager to HttpClient ( default async http client ) and HttpClientSync ( sync http client )
2. Sync Request Example:

GET:

  HttpClientSync httpClient;
  auto result = httpClient.Get(url);
  if (result){
    auto response = result.GetResponse();
  } else {
    std::cout << result.GetSessionState();
  }

POST:

  HttpClientSync httpClient;
  auto result = httpClient.Post(url, data, headers); // GET request
  if (result){
    auto response = result.GetResponse();
  } else {
    std::cout << result.GetSessionState();
  }

Pending Change:

Add test case for HTTP POST sync request.

codecov · 2020-12-14T15:36:11Z

Codecov Report

Merging #448 (d43d5ae) into main (8fd86ed) will decrease coverage by 0.29%.
The diff coverage is 78.88%.

@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
- Coverage   94.46%   94.16%   -0.30%     
==========================================
  Files         194      195       +1     
  Lines        8509     8571      +62     
==========================================
+ Hits         8038     8071      +33     
- Misses        471      500      +29

Impacted Files	Coverage Δ
...t/src/http/client/curl/http_client_factory_curl.cc	`60.00% <33.33%> (-40.00%)`	⬇️
ext/src/http/client/curl/http_client_curl.cc	`55.55% <55.55%> (ø)`
...ntelemetry/ext/http/client/curl/http_client_curl.h	`82.07% <65.78%> (-13.05%)`	⬇️
...lemetry/ext/http/client/curl/http_operation_curl.h	`78.52% <100.00%> (-3.91%)`	⬇️
...nclude/opentelemetry/ext/http/client/http_client.h	`95.45% <100.00%> (+4.54%)`	⬆️
ext/test/http/curl_http_test.cc	`94.50% <100.00%> (+0.49%)`	⬆️
sdk/test/metrics/counter_aggregator_test.cc	`98.21% <0.00%> (-1.79%)`	⬇️
sdk/src/logs/batch_log_processor.cc	`93.82% <0.00%> (-1.24%)`	⬇️
... and 1 more

ext/test/http/curl_http_test.cc

lalitb · 2020-12-18T06:07:19Z

@maxgolov Need you review for this : ) Thanks.

maxgolov · 2020-12-18T18:48:42Z

ext/test/http/curl_http_test.cc

+  received_requests_.clear();
+  curl::SessionManager session_manager;
+
+  auto session =


If I need to send 2 synchronous requests from two different threads, do I need to create two separate sessions?

Why do I have to specify session IP, port, and Uri separately - is there a reason to not having something like:

oReq.open("GET", "http://www.example.org/example.txt")

If I need to send 2 synchronous requests from two different threads, do I need to create two separate sessions?

Session is a logical entity here for maintaining session id, ssl cert ( if needed ), callback object ( for async request) etc. HTTP is stateless protocol, so it doesn't makes sense to have ongoing session, and sending HTTP Requests over that.

Why do I have to specify session IP, port, and Uri separately - is there a reason to not having something like:
oReq.open("GET", "http://www.example.org/example.txt")

Yeah, its doable as separate PR.

Ok, I have done the modified sync requests as below:

HttpClient httpClient; auto result = httpClient.Get(url); if (result){ auto response = result.GetResponse(); } else { std::cout << result.GetSessionState(); }

maxgolov · 2020-12-18T18:50:50Z

ext/include/opentelemetry/ext/http/client/http_client.h

+  auto session = sessionManager.createSession("localhost", 8000);
+  auto request = session->CreateRequest();
+  request->AddHeader(..);
+  SessionState session_state;


So this is not a session state - this is given request state?

maxgolov · 2020-12-18T18:57:40Z

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h

@@ -153,6 +153,33 @@ class Session : public http_client::Session
    });
  }

+  virtual std::unique_ptr<http_client::Response> SendRequestSync(


I believe it would be much cleaner if you operate only on Response object rather than on SessionState. Let me elaborate - right now you have two possible states:

success - in this case you return Response and status code; or

failure - in this case you force the user to test if Response ptr is nullptr, and then inspect the SessionState parameter passed by reference.. which is not really Session state (because Session may have multiple requests), but rather this exact Request state.

So since in reality it is Request state, can you have this aggregated on Response object - making sure that you always return Response: failed or succeeded, always return it. It may be successful response or empty response, failed response, with an indication on Response object that it's a failed response. Then you avoid the nullptr entirely, you avoid checking on two separate objects, and you can just propagate the object elsewhere to the place that needs to make a decision what to do next with that failed request-response pair.

Could you please clarify - if the original intent of SessionManager was to support more than one concurrent request at a time? If so, then SessionState is a misleading name for a synchronous request resultt. Session may have more than one request, one failed, one successful, and SessionState name does not represent the actual SessionManager Session state.

Response object represents http-response containing - body, header and code. Any error during http handshake are returned in SessionState. This is to keep it consistent with how we achieve it in async requests:

EventHandler::OnResponse(Response &); EventHandler::OnError(SessionState); Session::SendRequest(EventHandler &); // async unique_ptr<Request> Session::SendRequestSync(SessionState &); // sync

If it's fine, we can always return empty response in case of error, and the Response::status_code would be 0 in that case.

Session is more of logical entity, and doesn't represent multiple ongoing http requests.
SessionManager -> Multiple Sessions.
Single Session -> Single Request

maxgolov · 2020-12-18T19:01:33Z

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h

+    {
+      // we have a http response
+      auto response      = std::unique_ptr<Response>(new Response());
+      response->headers_ = curl_operation_->GetResponseHeaders();


How do you propagate Response code? If you are using int for that, then in Java it'd typically allocate -1 for failed response status code, which can then be used to determine if request was actually processed, as in connected and got some response code from server.

Currently we use uint16_t for status_code. If it's fine, we can always return empty response in case of error, and the Response::status_code would be 0 in that case ?

ThomsonTan · 2021-01-13T07:13:31Z

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h

+  {
+    is_session_active_ = true;
+    std::string url    = host_ + std::string(http_request_->uri_);
+    curl_operation_.reset(new HttpOperation(http_request_->method_, url, nullptr, RequestMode::Sync,


Could we set RequestMode::Sync in SendSync() or just deduce it from SendSync() and SendAsync(). Is the inconsistency between ctor and SendSync() handled?

We do need to know the actual sync mode in ctor. There are some initializations done in ctor, and the sync mode information is used to propagate the error/results to clients.

Is the inconsistency between ctor and SendSync() handled?

What kind of inconsistency we are talking about.

ThomsonTan · 2021-01-19T16:50:46Z

exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h

@@ -117,7 +117,7 @@ class ElasticsearchLogExporter final : public sdklogs::LogExporter
  ElasticsearchExporterOptions options_;

  // Object that stores the HTTP sessions that have been created
-  std::unique_ptr<ext::http::client::SessionManager> session_manager_;
+  std::unique_ptr<ext::http::client::HttpClient> session_manager_;


Rename the variable name to http_client_ too?

yeah, this needs to be done in both cc and h. I was thinking to change it once @maxgolov is fine with the current changes ( to avoid reverting back). But thanks for noticing it. I will add this as pending change in PR description.

ThomsonTan · 2021-01-19T16:53:49Z

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h

 private:
  std::shared_ptr<Request> http_request_;
  std::string host_;
  std::unique_ptr<HttpOperation> curl_operation_;
  uint64_t session_id_;
-  SessionManager &session_manager_;
+  HttpClient &session_manager_;


Rename the variable to http_client_ as well?

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h

ThomsonTan

LGTM with some minor comments.

…pp into sync-http-request

lalitb added 6 commits December 14, 2020 17:02

add sync request

8195e58

curl test

2e3031f

fix unique_ptr

9a5ac8a

fix memory leak

088bae8

add sample example

86b0922

fix timeout, and add timeout test

f27b07b

lalitb requested a review from a team December 14, 2020 15:29

ThomsonTan reviewed Dec 14, 2020

View reviewed changes

ext/test/http/curl_http_test.cc Show resolved Hide resolved

This was referenced Dec 18, 2020

Zipkin exporter #471

Merged

Add Elasticsearch Log Exporter with tests #444

Merged

maxgolov reviewed Dec 18, 2020

View reviewed changes

MarkSeufert mentioned this pull request Dec 23, 2020

Refactor Elasticsearch Log Exporter to use Synchronous HTTP methods #484

Closed

ThomsonTan reviewed Jan 13, 2021

View reviewed changes

lalitb added 5 commits January 13, 2021 17:21

missing status code

08a07f3

fix operation

7f6d292

Merge branch 'master' into sync-http-request

0c8b6d5

review comments

3d8c763

update comment

a0bdc20

ThomsonTan reviewed Jan 19, 2021

View reviewed changes

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h Show resolved Hide resolved

ThomsonTan approved these changes Jan 19, 2021

View reviewed changes

lalitb added 3 commits January 19, 2021 23:59

fix http post

2d93638

sync and http sync clients

0344793

Merge branch 'master' into sync-http-request

da399cd

lalitb added 7 commits January 22, 2021 19:12

add missing file

c7f4bca

fix w3c trace context compile

daf3046

bazel build

a4c7fc5

fix bazel

8800e9d

disabled timeout test

5509530

temporarily disable elastic exporter tests

85c1efd

fix asan error

6652aa3

maxgolov approved these changes Jan 22, 2021

View reviewed changes

Base automatically changed from master to main January 26, 2021 00:46

lalitb added 5 commits January 26, 2021 19:28

fix

0292a14

Merge branch 'main' into sync-http-request

a288fe8

fix

3d860ae

Merge branch 'sync-http-request' of github.com:lalitb/opentelemetry-c…

47a14ce

…pp into sync-http-request

fix

d43d5ae

lalitb merged commit 7c6220a into open-telemetry:main Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sending sync request through http_client #448

Sending sync request through http_client #448

lalitb commented Dec 14, 2020 •

edited

Loading

codecov bot commented Dec 14, 2020 •

edited

Loading

lalitb commented Dec 18, 2020

maxgolov Dec 18, 2020 •

edited

Loading

lalitb Jan 13, 2021 •

edited

Loading

lalitb Jan 19, 2021

maxgolov Dec 18, 2020

lalitb Jan 13, 2021

maxgolov Dec 18, 2020 •

edited

Loading

lalitb Jan 13, 2021 •

edited

Loading

maxgolov Dec 18, 2020 •

edited

Loading

lalitb Jan 13, 2021 •

edited

Loading

ThomsonTan Jan 13, 2021

lalitb Jan 13, 2021

ThomsonTan Jan 19, 2021

lalitb Jan 19, 2021

ThomsonTan Jan 19, 2021

ThomsonTan left a comment

Sending sync request through http_client #448

Sending sync request through http_client #448

Conversation

lalitb commented Dec 14, 2020 • edited Loading

codecov bot commented Dec 14, 2020 • edited Loading

Codecov Report

lalitb commented Dec 18, 2020

maxgolov Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

lalitb Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgolov Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

lalitb Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

maxgolov Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

lalitb Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomsonTan left a comment

Choose a reason for hiding this comment

lalitb commented Dec 14, 2020 •

edited

Loading

codecov bot commented Dec 14, 2020 •

edited

Loading

maxgolov Dec 18, 2020 •

edited

Loading

lalitb Jan 13, 2021 •

edited

Loading

maxgolov Dec 18, 2020 •

edited

Loading

lalitb Jan 13, 2021 •

edited

Loading

maxgolov Dec 18, 2020 •

edited

Loading

lalitb Jan 13, 2021 •

edited

Loading