Mock for http exporters

[esigo]

Fixes #1049 (issue)

Changes

removes http server from exporter tests, and uses mock for the clients.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #1185 (a643d1a) into main (9502339) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1185   +/-   ##
=======================================
  Coverage   92.96%   92.96%           
=======================================
  Files         196      196           
  Lines        7040     7040           
=======================================
  Hits         6544     6544           
  Misses        496      496           

Impacted Files	Coverage Δ
...ntelemetry/ext/http/client/curl/http_client_curl.h	91.67% <ø> (ø)
...t/src/http/client/curl/http_client_factory_curl.cc	50.00% <ø> (ø)
ext/test/http/curl_http_test.cc	95.17% <ø> (ø)

[lalitb]

Nit - --copt=-DENABLE_TEST is also part of BAZEL_OPTIONS, so can be removed here.

[lalitb]

nit - googletest suggests to mock the non-virtual function using template - https://chromium.googlesource.com/external/github.com/google/googletest/+/refs/tags/release-1.8.0/googlemock/docs/CookBook.md#mocking-nonvirtual-methods. Sharing if this is feasible, though I don't see much issue with the existing approach.

[lalitb]

I am thinking if mocking ext::http::client::HttpClient would be a better approach (the way we do it for Zipkin) as this will allow us to test most of the existing code in OtlpHttpClient::Export() method. We may have to add ext::http::client::HttpClient object as dependency injection to OtlpHttpClient to make this feasible.

[esigo]

I didn't use template because I thought it will be too much of noise in code.
Thanks for the idea, I will use dependency injection.

[lalitb]

Do we need to mock the SendRequest(), or can we create a empty (200 OK) response, and call the callback from same thread. Something like this

void SendRequest(EventHandler &callback) noexcept override
{
    auto response   = std::unique_ptr<Response>(new Response());
    .. populate response object..
    callback.OnResponse(response);
}

This will allow us to remove dependency of gtest/gmock from this target.

[esigo]

I have used Mock for testing the request that is going to be sent. The other option would be to make the client test itself via:

std::function<void(http_client::nosend::Session *)> CreateJsonTester(
    const std::string report_trace_id)
{
  return [report_trace_id](http_client::nosend::Session *mock_session) {
    auto check_json    = nlohmann::json::parse(mock_session->GetRequest()->body_, nullptr, false);
    // extract received_trace_id from request
    EXPECT_EQ(received_trace_id, report_trace_id);
  };
}

Then store this function into session, so whenever there is a request going to be sent the session tests itself:

  void SendRequest(opentelemetry::ext::http::client::EventHandler &callback) noexcept override
  {
    if (test_input_)
    {
      test_input_(this);
    }
    // let the otlp_http_client to continue
    Response response;
    callback.OnResponse(response);
  }

Shall I keep gMock or let the session test itself?

[lalitb]

Thank. Let's keep the gMock for now, we can fix it later.

I am just thinking another solution to change signature for EventHandler::OnEvent to something like:

virtual void OnEvent(SessionState, void* data = nullptr, size_t size = 0) noexcept = 0;

The method in its current form is not useful. Sending back client implementation-specific data would be useful in real-world scenarios like server certificate validation during SSL Handshake. And nosend client can then use this to return the Request object for validation, something like

void SendRequest(EventHandler &callback) noexcept override
{
    callback.OnEvent(SessionState::Connected, (void*)http_request_.get(), size);    
    auto response   = std::unique_ptr<Response>(new Response());
      .. populate response object..
    callback.OnResponse(response);
}

But don't want to delay this PR more, let's have this fix separately. We can have a github ticket to add this support / or remove mock from nosend client.

[esigo]

good idea, thanks.
yes I agree let's merge this PR and add the support in another issue.
Please assign the new issue to me.
client factory should be fixed.

[lalitb]

Thanks for the PR. Changes look good, apart from minor issues to address

• Remove mock dependency for nosend HTTP interface.
• Small change in HTTP factory implementation (discussing offline)

[ThomsonTan]

Duplicated with line 95?

[esigo]

thanks, cleaned.

[ThomsonTan]

Duplicated with line 94.

[esigo]

thanks, cleaned