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.

Sign up for GitHub

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 add url support #833

Merged
merged 17 commits into from
Jun 11, 2021
Merged

Http client add url support #833

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
379204e
Added overloading url function, changed CreateSession value in one un…
ohamuy Jun 5, 2021
3ccc3bf
Update Return Value of failed parsed URL
ohamuy Jun 7, 2021
d4a13cb
Fix formatting and remove TODO comment
ohamuy Jun 7, 2021
b68a259
Merge branch 'main' into HTTPClient_Add_Url_Support
ohamuy Jun 7, 2021
481d4b4
Update CHANGELOG.md
ohamuy Jun 7, 2021
281e257
Merge branch 'HTTPClient_Add_Url_Support' of https://github.com/open-…
ohamuy Jun 7, 2021
2a8b9a8
Fix linting Change Log error
ohamuy Jun 8, 2021
e137175
Merge branch 'main' into HTTPClient_Add_Url_Support
ohamuy Jun 8, 2021
9d28920
Deprecated original HTTP method, changed tests to match new singular …
ohamuy Jun 9, 2021
f11c94d
Merge branch 'main' into HTTPClient_Add_Url_Support
ohamuy Jun 9, 2021
fe2c73e
Delete settings.json
ohamuy Jun 9, 2021
0ea406e
Fix old uses of CreateSession
ohamuy Jun 11, 2021
357f879
Merge branch 'main' into HTTPClient_Add_Url_Support
ohamuy Jun 11, 2021
6ecbebf
Fixing main.cc Create Session
ohamuy Jun 11, 2021
9257924
Merge branch 'HTTPClient_Add_Url_Support' of https://github.com/open-…
ohamuy Jun 11, 2021
2be9bd5
Final fix to CreateSession bugs
ohamuy Jun 11, 2021
365e18d
Fix CreateSession Bugs
ohamuy Jun 11, 2021
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Increment the:

## [Unreleased]

* [INSTRUMENTATION] HTTPClient: Change support for full URL argument ([#833](https://github.com/open-telemetry/opentelemetry-cpp/pull/833))
* [EXPORTER] Add OTLP/HTTP+JSON Protocol exporter ([#810](https://github.com/open-telemetry/opentelemetry-cpp/pull/810))
* [EXPORTER] Rename `OtlpExporter` to `OtlpGrpcExporter`, rename `otlp_exporter.h` to `otlp_grpc_exporter.h` ([#810](https://github.com/open-telemetry/opentelemetry-cpp/pull/810))

Expand Down
27 changes: 14 additions & 13 deletions ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "http_operation_curl.h"
#include "opentelemetry/ext/http/client/http_client.h"
#include "opentelemetry/ext/http/common/url_parser.h"
#include "opentelemetry/version.h"

#include <map>
Expand Down Expand Up @@ -115,18 +116,13 @@ class HttpClient;
class Session : public http_client::Session
{
public:
Session(HttpClient &http_client, const std::string &host, uint16_t port = 80)
Session(HttpClient &http_client,
std::string scheme = "http",
const std::string &host = "",
uint16_t port = 80)
: http_client_(http_client), is_session_active_(false)
{
if (host.rfind("http://", 0) != 0 && host.rfind("https://", 0) != 0)
{
host_ = "http://" + host; // TODO - https support
}
else
{
host_ = host;
}
host_ += ":" + std::to_string(port) + "/";
host_ = scheme + "://" + host + ":" + std::to_string(port) + "/";
}

std::shared_ptr<http_client::Request> CreateRequest() noexcept override
Expand Down Expand Up @@ -249,10 +245,15 @@ class HttpClient : public http_client::HttpClient
// The call (curl_global_init) is not thread safe. Ensure this is called only once.
HttpClient() : next_session_id_{0} { curl_global_init(CURL_GLOBAL_ALL); }

std::shared_ptr<http_client::Session> CreateSession(nostd::string_view host,
uint16_t port = 80) noexcept override
std::shared_ptr<http_client::Session> CreateSession(nostd::string_view url) noexcept override
{
auto session = std::make_shared<Session>(*this, std::string(host), port);
auto parsedUrl = common::UrlParser(std::string(url));
if (!parsedUrl.success_)
{
return std::make_shared<Session>(*this);
}
auto session =
std::make_shared<Session>(*this, parsedUrl.scheme_, parsedUrl.host_, parsedUrl.port_);
auto session_id = ++next_session_id_;
session->SetId(session_id);
sessions_.insert({session_id, session});
Expand Down
6 changes: 3 additions & 3 deletions ext/include/opentelemetry/ext/http/client/http_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,9 @@ class Session
class HttpClient
{
public:
virtual std::shared_ptr<Session> CreateSession(nostd::string_view host,
uint16_t port = 80) noexcept = 0;
virtual bool CancelAllSessions() noexcept = 0;
virtual std::shared_ptr<Session> CreateSession(nostd::string_view url) noexcept = 0;
Copy link
Contributor

@maxgolov maxgolov Jun 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, I see what you are doing. But I am not sure - maybe we should just deprecate the old method? I think this new one is much better. If we look at Javascript API for the open method, it only requires the URL parameter. In Java - this is also completely outsourced to java.net.URL . It's even more logical to assume that the session itself accepts URL object or string, rather than trying to implement multi-parameter parsing from host, port. Because it's rather [ scheme, host, port, location ] than simply [ host, port ] pair. It'd be best we just collapse all of it into single string parameter, then parse it using URL class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly, I just overloaded the function because that is what the @lalitb (the original issue creator) said might have been good. If its alright I can just simply remove the old parameter and just leave the new one since its also consistent with the GET and POST commands. But that is definitely a decision for the maintainers so when they give their input we can see whats the best course of action. @lalitb @reyang

Copy link
Member

@lalitb lalitb Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it makes sense to remove the original method as @maxgolov suggested.


virtual bool CancelAllSessions() noexcept = 0;

virtual bool FinishAllSessions() noexcept = 0;

Expand Down
13 changes: 6 additions & 7 deletions ext/test/http/curl_http_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ TEST_F(BasicCurlHttpTests, SendGetRequest)
auto session_manager = http_client::HttpClientFactory::Create();
EXPECT_TRUE(session_manager != nullptr);

auto session = session_manager->CreateSession("127.0.0.1", HTTP_PORT);
auto session = session_manager->CreateSession("http://127.0.0.1:19000");
auto request = session->CreateRequest();
request->SetUri("get/");
GetEventHandler *handler = new GetEventHandler();
Expand All @@ -208,7 +208,7 @@ TEST_F(BasicCurlHttpTests, SendPostRequest)
auto session_manager = http_client::HttpClientFactory::Create();
EXPECT_TRUE(session_manager != nullptr);

auto session = session_manager->CreateSession("127.0.0.1", HTTP_PORT);
auto session = session_manager->CreateSession("http://127.0.0.1:19000");
auto request = session->CreateRequest();
request->SetUri("post/");
request->SetMethod(http_client::Method::Post);
Expand All @@ -235,8 +235,7 @@ TEST_F(BasicCurlHttpTests, RequestTimeout)
auto session_manager = http_client::HttpClientFactory::Create();
EXPECT_TRUE(session_manager != nullptr);

auto session =
session_manager->CreateSession("222.222.222.200", HTTP_PORT); // Non Existing address
auto session = session_manager->CreateSession("222.222.222.200:19000"); // Non Existing address
auto request = session->CreateRequest();
request->SetUri("get/");
GetEventHandler *handler = new GetEventHandler();
Expand Down Expand Up @@ -309,14 +308,14 @@ TEST_F(BasicCurlHttpTests, GetBaseUri)
{
curl::HttpClient session_manager;

auto session = session_manager.CreateSession("127.0.0.1", 80);
auto session = session_manager.CreateSession("127.0.0.1:80");
ASSERT_EQ(std::static_pointer_cast<curl::Session>(session)->GetBaseUri(), "http://127.0.0.1:80/");

session = session_manager.CreateSession("https://127.0.0.1", 443);
session = session_manager.CreateSession("https://127.0.0.1:443");
ASSERT_EQ(std::static_pointer_cast<curl::Session>(session)->GetBaseUri(),
"https://127.0.0.1:443/");

session = session_manager.CreateSession("http://127.0.0.1", 31339);
session = session_manager.CreateSession("http://127.0.0.1:31339");
ASSERT_EQ(std::static_pointer_cast<curl::Session>(session)->GetBaseUri(),
"http://127.0.0.1:31339/");
}