HTTPClient : Support for full URL argument instead of host & port

[lalitb]

As discussed here:

• HttpClient::CreateSession method should support passing full URL as an argument instead of host and port (or perhaps support both). This would be then consistent with HttpClientSync::Get(url) and HttpClientSync::Post(url) methods.
• Validate if HTTPS URL is parsed successfully, and supported for curl implementation. Remove TODO from here once https works/fixed:

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

Line 123 in ac0e617

host_ = "http://" + host; // TODO - https support

[ohamuy]

Hey! Could I pick up this issue?

@alolita

[ohamuy]

@lalitb Would you rather have me allow for HttpClient::CreateSession to have the template have multiple parameters, one for host/port and one for a full url? Or would you rather have me change the current implementation to simply fix https and work for full url

[ohamuy]

I’d like to work on this issue. Can you please assign this to me? @alolita

[lalitb]

Thanks @ohamuy - assigned to you now. We can use function overloading to support both unless you see benefit with templates?

[ohamuy]

Yea @lalitb I could definitely do function overloading. Also how do we know whether the user wants http or https appended if they only provide the host and port? Also for testing would I use Bazel or is there some other way to test my code?

[lalitb]

Yea @lalitb I could definitely do function overloading. Also how do we know whether the user wants http or https appended if they only provide the host and port? Also for testing would I use Bazel or is there some other way to test my code?

default should be always http unless user explicitly specified https. This should be irrespective of whether port number is 80 or 443.

[ohamuy]

Hey @lalitb @reyang I wrote this block of code overloading the Session to allow for url input. This utilizes the urlparser in the common folder. What would you like me to return if it doesn't successfully parse? Nullptr? Also this code compiles successfully using Bazel

Are there any additional features besides this url addition that are needed (like in the constructor or implementation)? Also how should I validate that this overload function works properly? Should I create and run an example in the examples folder or is the http Bazel example sufficient?

 std::shared_ptr<http_client::Session> CreateSession(nostd::string_view url) noexcept override
  {
	  auto parsedUrl = common::UrlParser(std::string(url));
	  if (!parsedUrl.success_) {
		  //return std::make_shared<Session>();
	  }
	  auto session    = std::make_shared<Session>(*this, parsedUrl.host_, parsedUrl.port_);
	  auto session_id = ++next_session_id_;
	  session->SetId(session_id);
	  sessions_.insert({session_id, session});
	  return session;
  }

[lalitb]

@ohamuy - Your approach looks good. We can return the Session object with an empty/non-existent host, and default port(80). And let the actual connection failure get caught at ResponseHandler level:

 std::shared_ptr<http_client::Session> CreateSession(nostd::string_view url) noexcept override
  {
	  auto parsedUrl = common::UrlParser(std::string(url));
	  if (!parsedUrl.success_) {
                  //TBD - Log error once error handler is supported
		  return std::make_shared<Session>(*this, "empty-host", 80);
	  }
	  auto session    = std::make_shared<Session>(*this, parsedUrl.host_, parsedUrl.port_);
	  auto session_id = ++next_session_id_;
	  session->SetId(session_id);
	  sessions_.insert({session_id, session});
	  return session;
  }

While using ,

  auto session = httpClient.createSession("localhost", 8000);
  auto request = session->CreateRequest();
  request->AddHeader(..);
  SimpleResponseHandler res_handler;
  session->SendRequest(res_handler);

res_handler will catch the connection error.

[ohamuy]

@lalitb Ok great, I'll add these changes and get a PR ready (will have to get it reviewed first within Amazon!)

[ohamuy]

We can close this issue now, correct? @lalitb