Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http Client API #309

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 161 additions & 0 deletions api/include/opentelemetry/http/http_client.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#pragma once

#include "opentelemetry/nostd/string_view.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace http
{

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix this.


// Request has been aborted by the caller. The server might or
// might not have already received or processed the request.
HttpResult_Aborted = 1,

// Local conditions have prevented the request from being sent
// (invalid request parameters, out of memory, internal error etc.)
HttpResult_LocalFailure = 2,

// Network conditions somewhere between the local machine and
// the target server have caused the request to fail
// (connection failed, connection dropped abruptly etc.).
HttpResult_NetworkFailure = 3

};

// The HttpHeaders class contains a set of HTTP headers.
class HttpHeaders
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the method names lower case here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix this.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Gets a string value given a name
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Tests whether the headers contain the specified name.
virtual bool contains(nostd::string_view const &name) const = 0;
};

// The HttpRequest class represents a Request object.
// Individual HTTP client implementations can implement the request object in
// the most efficient way. Either fill the request first, and then issue
// the underlying request, or create the real request immediately, and then forward
// the methods and set the individual parameters directly one by one.
class HttpRequest
{
public:
// Gets the request ID.
virtual const nostd::string_view &GetId() const = 0;
lalitb marked this conversation as resolved.
Show resolved Hide resolved

// Sets the request Method
lalitb marked this conversation as resolved.
Show resolved Hide resolved
virtual void SetMethod(nostd::string_view const &method) = 0;

// Gets the request URI
virtual void SetUrl(nostd::string_view const &url) = 0;

// Gets the request Headers
virtual HttpHeaders &GetHeaders() const = 0;

// Sets the request body
virtual void SetBody(const uint8_t *const body, const size_t len) = 0;

// Gets the request body
lalitb marked this conversation as resolved.
Show resolved Hide resolved
virtual void GetBody(uint8_t *body, size_t &len) = 0;
};

// The HttpResponse class represents a Response object.
// Individual HTTP client implementations can implement the response object
// in the most efficient way. Either copy all of the underlying data to a new
// structure, and provide it to the callback; or keep the real
// response data around, forward the methods, and retrieve the individual
// values directly one by one.
class HttpResponse
{
public:
// Gets the response ID.
virtual const nostd::string_view &GetId() = 0;

// Get the response result code.
virtual HttpResult GetResult() = 0;

// Gets the response status code.
virtual unsigned GetStatusCode() = 0;

// Gets the response headers.
virtual const HttpHeaders &GetHeaders() = 0;

// Gets the response body.
virtual void GetBody(uint8_t *body, size_t &len) = 0;
};

// The HttpResponseCallback class receives HTTP client responses
class HttpResponseCallback
{
public:
// Called when an HTTP request completes.
// The passed response object contains details about the exact way the
// request finished (HTTP status code, headers, content, error codes
// etc.). The ownership of the response object is transferred to the
// callback object. It can store it for later if necessary. Finally, it
// must be deleted using its virtual destructor.
virtual void OnHttpResponse(HttpResponse *response) = 0;
};

// The HttpClient class is the interface for HTTP client implementations.
class HttpClient
{
public:
// Creates an empty HTTP request object.
// The created request object has only its ID prepopulated. Other fields
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we return a std::unique_ptr here?


// Begins an HTTP request.
// The method takes ownership of the passed request, and can destroy it before
// returning to the caller. Do not access the request object in any
// way after this invocation, and do not delete it.
// The callback object is always called, even if the request is
// cancelled, or if an error occurs immediately during sending. In the
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Cancels an HTTP request.
// The caller must provide a string ID returned earlier by request->GetId().
// The request is cancelled asynchronously. The caller must still
// wait for the relevant OnHttpResponse() callback (it can just come
// earlier with some "aborted" error status).
virtual void CancelRequest(nostd::string_view const &id) = 0;

virtual void CancelAllRequestsSync() {}
};

} // namespace http
OPENTELEMETRY_END_NAMESPACE
153 changes: 153 additions & 0 deletions sdk/include/opentelemetry/sdk/http/http_client.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#pragma once
#include "opentelemetry/http/http_client.h"
#include "opentelemetry/version.h"

#include <algorithm>
#include <cstring>
#include <functional>
#include <map>
#include <memory>
#include <vector>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace http
{
namespace http_api = opentelemetry::http;

static std::function<bool(const std::string &, const std::string &)> CaseInsensitiveComparator =
[](const std::string &s1, const std::string &s2) -> bool {
std::string str1(s1.length(), ' ');
lalitb marked this conversation as resolved.
Show resolved Hide resolved
std::string str2(s2.length(), ' ');
auto lowerCase = [](char c) -> char { return tolower(c); };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing std:: before tolower(c);?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

std::transform(s1.begin(), s1.end(), str1.begin(), lowerCase);
std::transform(s2.begin(), s2.end(), str2.begin(), lowerCase);
return str1 < str2;
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@maxgolov maxgolov Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public:
virtual void set(nostd::string_view const &name, nostd::string_view const &value) override
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{
auto range = equal_range(std::string(name.data()));
auto hint = erase(range.first, range.second);
insert(hint, std::make_pair(name, value));
}

virtual void add(nostd::string_view const &name, nostd::string_view const &value) override
{
insert(std::make_pair(name, value));
}

virtual nostd::string_view const &get(nostd::string_view const &name) const override
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{
auto it = find(std::string(name.data()));
return (it != end()) ? it->second : m_empty;
}

virtual bool contains(nostd::string_view const &name) const override
{
auto it = find(std::string(name.data()));
return (it != end());
}

private:
nostd::string_view m_empty{};
};

class SimpleHttpRequest : public http_api::HttpRequest
{

public:
SimpleHttpRequest(nostd::string_view const &id) : id_(id), method_("GET") {}

// Gets the HTTP request ID.
virtual const nostd::string_view &GetId() const override { return id_; }

// Sets the request method
virtual void SetMethod(nostd::string_view const &method) override { method_ = method; }

// Sets the HTTP request URI.
virtual void SetUrl(nostd::string_view const &url) override { url_ = url; }

// Gets the HTTP request headers.
virtual http_api::HttpHeaders &GetHeaders() const override { return *headers_; }

// Sets the request body.
virtual void SetBody(const uint8_t *const body, const size_t len) override
{
for (int i = 0; i < len; i++)
{
body_.push_back(body[i]);
}
}

virtual void GetBody(uint8_t *body, size_t &len) override
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{
len = 0;
for (auto &e : body_)
{
body[len++] = e;
}
len = (len > 0) ? len - 1 : len;
}

private:
std::unique_ptr<http_api::HttpHeaders> headers_;
std::vector<uint8_t> body_;
nostd::string_view method_;
nostd::string_view id_;
nostd::string_view url_;
};

class SimpleHttpResponse : public http_api::HttpResponse
{

public:
SimpleHttpResponse(nostd::string_view const &id)
: id_(id), result_(http_api::HttpResult::HttpResult_LocalFailure), statusCode_(0)
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{}

virtual http_api::HttpResult GetResult() override { return result_; }

virtual unsigned GetStatusCode() override { return statusCode_; }

virtual const http_api::HttpHeaders &GetHeaders() override { return *headers_; }

virtual void GetBody(uint8_t *body, size_t &len) override
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{
int i = 0;
for (auto &e : body_)
{
body_[i++] = e;
}
}

private:
nostd::string_view id_;
unsigned statusCode_;
std::unique_ptr<http_api::HttpHeaders> headers_;
http_api::HttpResult result_;
std::vector<uint8_t> body_;
};
} // namespace http
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE