Http Client API #309

lalitb · 2020-08-31T11:47:16Z

This PR brings the HTTP client API to be used across various exporters ( eg zipkin, azure monitor ) to communicate with remove telemetry servers.

Need to discuss/review on the location of these headers. Not sure if they are right place.

TBD:

Documentation on usage
Sample implementation.

linux-foundation-easycla · 2020-08-31T11:47:21Z

The committers are authorized under a signed CLA.

✅ Lalit Kumar Bhasin (e67bba8, 9bf7c41, f6bd12a, c58d316, d7fa6c9, 041191e, 71eaa99, 4958615, 0f4592b, a993179, c648f9f, aa8ee7c)

codecov · 2020-08-31T11:52:23Z

Codecov Report

Merging #309 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
+ Coverage   94.55%   94.56%   +0.01%     
==========================================
  Files         146      146              
  Lines        6610     6629      +19     
==========================================
+ Hits         6250     6269      +19     
  Misses        360      360

Impacted Files	Coverage Δ
sdk/include/opentelemetry/sdk/metrics/controller.h	`90.32% <0.00%> (-6.46%)`	⬇️
...pi/include/opentelemetry/context/runtime_context.h	`95.83% <0.00%> (-4.17%)`	⬇️
sdk/include/opentelemetry/sdk/metrics/instrument.h	`93.84% <0.00%> (-0.36%)`	⬇️
api/include/opentelemetry/trace/span_context.h	`100.00% <0.00%> (ø)`
sdk/test/trace/tracer_test.cc	`99.09% <0.00%> (+0.01%)`	⬆️
sdk/src/trace/span.cc	`84.33% <0.00%> (+3.56%)`	⬆️
sdk/src/trace/tracer.cc	`73.52% <0.00%> (+4.56%)`	⬆️
sdk/src/trace/span.h	`100.00% <0.00%> (+100.00%)`	⬆️

lalitb · 2020-08-31T12:00:19Z

❌The commit (e67bba8 ,9bf7c419dfcc4ee81bf36a7d3b2e8bb4c7a56497 ,f6bd12a7a57f930db7ffab6097d62be04eb640fe ,c58d316c8afed28370df5a2f5ef6daa3746003cd ,d7fa6c965217bffd4f81453874e82e783a87e81a ,041191eca1a2b11f4442f06138b0e4a4fe419ca2 ,71eaa9922613a9ed3e2b4e859a227dd0f24b5c37) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve. For further assistance with EasyCLA, please submit a support request ticket.

Have raised request for this.

api/include/opentelemetry/http/http_client.h

sdk/include/opentelemetry/sdk/http/http_client.h

pyohannes · 2020-09-01T15:59:19Z

api/include/opentelemetry/http/http_client.h

+  virtual void set(nostd::string_view const &name, nostd::string_view const &value) = 0;
+
+  // Inserts a name/value pair
+  virtual void add(nostd::string_view const &name, nostd::string_view const &value) = 0;


What's the exact difference between add and set? Will add allow having multiple values for a single header? Will set fail if an element with the given name doesn't yet exist?

I think we should refactor this. Original idea was that:

add - appends to multimap.

set - either adds or replaces if header already exists.

Maybe we should converge on alternate names for that.

I'm not sure if we really want to employ nostd::string_view here though. It's in SDK, which won't have ABI-stability requirement..... Unless we want to runtime dynamically inject an instance of HTTP stack.

pyohannes · 2020-09-01T16:03:22Z

api/include/opentelemetry/http/http_client.h

+  // Returns:
+  //      If there are multiple headers with same name, it returns any one of the value
+  //      (implementation defined) Empty string if there is no header with speicfied name..
+  virtual nostd::string_view const &get(nostd::string_view const &name) const = 0;


We usually don't pass string_views as reference in other parts of the API.

pyohannes · 2020-09-01T16:04:23Z

api/include/opentelemetry/http/http_client.h

+  // must be set by the caller. The request object can then be sent
+  // using SendRequestAsync(). If you are not going to use the request object
+  // then you can delete it safely using its virtual destructor.
+  virtual HttpRequest *CreateRequest() = 0;


Can we return a std::unique_ptr here?

pyohannes · 2020-09-01T16:05:54Z

api/include/opentelemetry/http/http_client.h

+  // latter case, the OnHttpResponse() callback is called before this
+  // method returns. You must keep the callback object alive until its
+  // OnHttpResponse() callback is called.
+  virtual void SendRequest(HttpRequest *request, HttpResponseCallback *callback) = 0;


Moving the request as an unique pointer would enforce the conditions you documented above (taking ownership and no modification after the call): std::unique_ptr<HttpRequest>&& request.

All the raw pointers would be converted to shared_ptr/unique_ptr in new PR. In this case, I don't see need to move semantics as <unique_ptr> would anyway transfer the ownership.

pyohannes · 2020-09-01T16:06:51Z

api/include/opentelemetry/http/http_client.h

+{
+public:
+  // Inserts a name/value pair, and removes elements with the same name.
+  virtual void set(nostd::string_view const &name, nostd::string_view const &value) = 0;


Why are the method names lower case here?

Will fix this.

pyohannes · 2020-09-01T16:07:44Z

api/include/opentelemetry/http/http_client.h

+enum class HttpResult : uint8_t
+{
+  // Response has been received successfully from target server.
+  HttpResult_OK = 0,


Please conform the the Google C++ Style Guide. Here's the part for enums.

@lalitb FYI https://github.com/open-telemetry/opentelemetry-cpp/blob/master/CONTRIBUTING.md#development

Will fix this.

sdk/include/opentelemetry/sdk/http/http_client.h

api/include/opentelemetry/http/http_client.h

maxgolov

I'd like to block merging this until we sort out some (minor) legal Q 😄 ... We can continue iterations / discussions on this.

sdk/include/opentelemetry/sdk/http/http_client.h

ThomsonTan · 2020-09-02T01:25:12Z

sdk/include/opentelemetry/sdk/http/http_client.h

+    [](const std::string &s1, const std::string &s2) -> bool {
+  std::string str1(s1.length(), ' ');
+  std::string str2(s2.length(), ' ');
+  auto lowerCase = [](char c) -> char { return tolower(c); };


Missing std:: before tolower(c);?

We may want to use this implementation into some form of header-only common utility class, since the same pattern is being used more than once elsewhere.

sdk/include/opentelemetry/sdk/http/http_client.h

ThomsonTan · 2020-09-02T01:40:53Z

sdk/include/opentelemetry/sdk/http/http_client.h

+
+// HttpHeaders implementation
+class HttpHeaders : http_api::HttpHeaders,
+                    std::multimap<std::string, std::string, decltype(CaseInsensitiveComparator)>


Should this std::multimap based on nostd::string_view according to insert() called in add() below?

I'd actually advocate again that we can just use std::string here in SDK for the sake of simplicity and compat with older compilers, just using plain standard C++11 (or maybe C++14) classes, so that external components do not have to take a dependency on nostd. There is no ABI stability guarantee for exporters. We discussed it in prior SIG meetings that we are not providing ABI stable interface in SDK, for exporters, and thus, using nostd here is a bit of an overkill.

In short: there is no requirement to use nostd inside the SDK, because there is no ABI-stability requirement/guarantee within the SDK code.

We definitely shouldn't store nostd::string_view in the multimap.

There are some slight advantages of using nostd::string_view over std::string as type for parameters in function calls, as this allows users to pass both std::string and const char* without creating an intermediate std::string. But I don't have a strong preference for it in this API, which will not be on a performance critical path.

This would be changed in new PR. string_view class won't be used anymore. Although agree on string_view advantage as stated by @pyohannes

maxgolov · 2020-09-02T01:47:45Z

api/include/opentelemetry/http/http_client.h

+};
+
+// The HttpHeaders class contains a set of HTTP headers.
+class HttpHeaders


Is there a place where do use HttpHeaders in API? Does it mean we need to have a transform somewhere from std::multimap<std::string, std::string> used within the SDK - to ABI-safe HttpHeaders implementation used in API surface? Can you provide some concrete example where we require this in API?

I don't see a use case for this in the API.

sdk/include/opentelemetry/sdk/http/http_client.h

lalitb · 2020-09-07T14:17:19Z

@maxgolov @reyang @pyohannes @ThomsonTan Thank you all for the review comments. I would abandon this PR due to the current non-clarity of the copyright issue as some part is coming from Microsoft's MIT licensed code base. I am in process of re-writing the API and would be raising the new PR in couple of days. I would be respond to relevant comments ( which holds good in new PR too) in this PR before abandon.

g-easy · 2020-09-08T02:33:45Z

Need to discuss/review on the location of these headers

Should these headers be in the SDK rather than the API?

IMO the API is just for instrumentation calls and the no-op default implementation thereof.

lalitb · 2020-09-08T04:36:57Z

Need to discuss/review on the location of these headers

Should these headers be in the SDK rather than the API?

IMO the API is just for instrumentation calls and the no-op default implementation thereof.

@g-easy Yes, that's the plan for new PR. Would be moving both interface / implementation to SDK.

lalitb · 2020-10-20T14:16:12Z

Closing for copyright issues, new PR raised : #370

lalitb added 7 commits August 27, 2020 11:48

draft

e67bba8

remove vector

9bf7c41

implement

f6bd12a

fix implementation

c58d316

fix indent

d7fa6c9

fix indent

041191e

spacing

71eaa99

lalitb requested a review from a team August 31, 2020 11:47

lalitb mentioned this pull request Aug 31, 2020

REQUEST: New membership for lalitb open-telemetry/community#464

Closed

maxgolov linked an issue Aug 31, 2020 that may be closed by this pull request

HTTP client API for exporter developers #6

Closed

MS license

4958615

reyang reviewed Aug 31, 2020

View reviewed changes

api/include/opentelemetry/http/http_client.h Outdated Show resolved Hide resolved

reyang reviewed Aug 31, 2020

View reviewed changes

api/include/opentelemetry/http/http_client.h Outdated Show resolved Hide resolved

reyang reviewed Aug 31, 2020

View reviewed changes

api/include/opentelemetry/http/http_client.h Outdated Show resolved Hide resolved

lalitb added 2 commits September 1, 2020 06:28

review comments

0f4592b

fix format

a993179

maxgolov reviewed Sep 1, 2020

View reviewed changes

api/include/opentelemetry/http/http_client.h Outdated Show resolved Hide resolved

maxgolov reviewed Sep 1, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/http/http_client.h Outdated Show resolved Hide resolved

maxgolov reviewed Sep 1, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/http/http_client.h Show resolved Hide resolved

maxgolov reviewed Sep 1, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/http/http_client.h Show resolved Hide resolved

maxgolov reviewed Sep 1, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/http/http_client.h Outdated Show resolved Hide resolved

lalitb added 2 commits September 1, 2020 11:34

review comments

c648f9f

fix review comments

aa8ee7c

pyohannes reviewed Sep 1, 2020

View reviewed changes

ThomsonTan reviewed Sep 1, 2020

View reviewed changes

api/include/opentelemetry/http/http_client.h Show resolved Hide resolved

ThomsonTan reviewed Sep 1, 2020

View reviewed changes

api/include/opentelemetry/http/http_client.h Show resolved Hide resolved

ThomsonTan reviewed Sep 1, 2020

View reviewed changes

api/include/opentelemetry/http/http_client.h Show resolved Hide resolved

maxgolov suggested changes Sep 2, 2020

View reviewed changes

ThomsonTan reviewed Sep 2, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/http/http_client.h Show resolved Hide resolved

ThomsonTan reviewed Sep 2, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/http/http_client.h Show resolved Hide resolved

ThomsonTan reviewed Sep 2, 2020

View reviewed changes

maxgolov reviewed Sep 2, 2020

View reviewed changes

ThomsonTan reviewed Sep 2, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/http/http_client.h Show resolved Hide resolved

ThomsonTan mentioned this pull request Sep 4, 2020

REQUEST: New membership for ThomsonTan open-telemetry/community#471

Closed

6 tasks

jajanet mentioned this pull request Sep 14, 2020

Restructure Ext folder #292

Closed

reyang added the pr:do-not-merge This PR is not ready to be merged. label Sep 25, 2020

reyang mentioned this pull request Oct 7, 2020

Add lalitb and ThomsonTan as approvers #355

Merged

lalitb mentioned this pull request Oct 20, 2020

Providing an abstract HTTP interface. #370

Merged

lalitb closed this Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http Client API #309

Http Client API #309

lalitb commented Aug 31, 2020

linux-foundation-easycla bot commented Aug 31, 2020 •

edited

Loading

codecov bot commented Aug 31, 2020 •

edited

Loading

lalitb commented Aug 31, 2020

pyohannes Sep 1, 2020

maxgolov Sep 2, 2020

pyohannes Sep 1, 2020

pyohannes Sep 1, 2020

pyohannes Sep 1, 2020

lalitb Sep 7, 2020

pyohannes Sep 1, 2020

lalitb Sep 7, 2020

pyohannes Sep 1, 2020

reyang Sep 1, 2020

lalitb Sep 7, 2020

maxgolov left a comment •

edited

Loading

ThomsonTan Sep 2, 2020

maxgolov Sep 2, 2020

ThomsonTan Sep 2, 2020

maxgolov Sep 2, 2020 •

edited

Loading

pyohannes Sep 4, 2020

lalitb Sep 7, 2020

maxgolov Sep 2, 2020

pyohannes Sep 4, 2020

lalitb commented Sep 7, 2020 •

edited

Loading

g-easy commented Sep 8, 2020

lalitb commented Sep 8, 2020

lalitb commented Oct 20, 2020

Http Client API #309

Http Client API #309

Conversation

lalitb commented Aug 31, 2020

linux-foundation-easycla bot commented Aug 31, 2020 • edited Loading

codecov bot commented Aug 31, 2020 • edited Loading

Codecov Report

lalitb commented Aug 31, 2020

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

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

maxgolov left a comment • 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 Sep 2, 2020 • 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

lalitb commented Sep 7, 2020 • edited Loading

g-easy commented Sep 8, 2020

lalitb commented Sep 8, 2020

lalitb commented Oct 20, 2020

linux-foundation-easycla bot commented Aug 31, 2020 •

edited

Loading

codecov bot commented Aug 31, 2020 •

edited

Loading

maxgolov left a comment •

edited

Loading

maxgolov Sep 2, 2020 •

edited

Loading

lalitb commented Sep 7, 2020 •

edited

Loading