From 2054db9168ad2a4c9a102dd9fb93471af6b1fa81 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 14 Jun 2021 14:56:24 -0700 Subject: [PATCH 1/5] Fix missing notify of conditional variable in OTLP HTTP exporter test --- exporters/otlp/test/otlp_http_exporter_test.cc | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index ef4ab2094e..b5c9ab2f6f 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -87,6 +87,8 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: } } + int response_status = 0; + if (request.uri == kDefaultTracePath) { response.headers["Content-Type"] = "application/json"; @@ -103,7 +105,7 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: else { response.body = "{\"code\": 400, \"message\": \"Parse binary failed\"}"; - return 400; + response_status = 400; } } else if (nullptr != request_content_type && *request_content_type == kHttpJsonContentType) @@ -113,7 +115,7 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: if (json.is_discarded()) { response.body = "{\"code\": 400, \"message\": \"Parse json failed\"}"; - return 400; + response_status = 400; } else { @@ -124,18 +126,22 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: else { response.body = "{\"code\": 400, \"message\": \"Unsupported content type\"}"; - return 400; + response_status = 400; } - return 200; + response_status = 200; } else { std::unique_lock lk(mtx_requests); response.headers["Content-Type"] = "text/plain"; response.body = "404 Not Found"; - return 200; + response_status = 200; } + + cv_got_events.notify_one(); + + return response_status; } bool waitForRequests(unsigned timeOutSec, size_t expected_count = 1) @@ -220,7 +226,7 @@ TEST_F(OtlpHttpExporterTestPeer, ExportJsonIntegrationTest) report_trace_id.assign(trace_id_hex, sizeof(trace_id_hex)); } - ASSERT_TRUE(waitForRequests(2, old_count + 1)); + ASSERT_TRUE(waitForRequests(20, old_count + 1)); auto check_json = received_requests_json_.back(); auto resource_span = *check_json["resource_spans"].begin(); auto instrumentation_library_span = *resource_span["instrumentation_library_spans"].begin(); From 3d7fba4e69cbd49e03203adb4ceac92998e6ef2e Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 14 Jun 2021 22:36:25 -0700 Subject: [PATCH 2/5] Format code --- exporters/otlp/test/otlp_http_exporter_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index b5c9ab2f6f..f63853c46b 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -104,7 +104,7 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: } else { - response.body = "{\"code\": 400, \"message\": \"Parse binary failed\"}"; + response.body = "{\"code\": 400, \"message\": \"Parse binary failed\"}"; response_status = 400; } } @@ -114,7 +114,7 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: response.headers["Content-Type"] = "application/json"; if (json.is_discarded()) { - response.body = "{\"code\": 400, \"message\": \"Parse json failed\"}"; + response.body = "{\"code\": 400, \"message\": \"Parse json failed\"}"; response_status = 400; } else @@ -125,7 +125,7 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: } else { - response.body = "{\"code\": 400, \"message\": \"Unsupported content type\"}"; + response.body = "{\"code\": 400, \"message\": \"Unsupported content type\"}"; response_status = 400; } @@ -136,7 +136,7 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS:: std::unique_lock lk(mtx_requests); response.headers["Content-Type"] = "text/plain"; response.body = "404 Not Found"; - response_status = 200; + response_status = 200; } cv_got_events.notify_one(); From 92ca9202534de64894a59ae7b20ac2e3181ceb48 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 14 Jun 2021 22:41:38 -0700 Subject: [PATCH 3/5] Restore the wait timeout --- exporters/otlp/test/otlp_http_exporter_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index f63853c46b..2dfd32ce05 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -226,7 +226,7 @@ TEST_F(OtlpHttpExporterTestPeer, ExportJsonIntegrationTest) report_trace_id.assign(trace_id_hex, sizeof(trace_id_hex)); } - ASSERT_TRUE(waitForRequests(20, old_count + 1)); + ASSERT_TRUE(waitForRequests(2, old_count + 1)); auto check_json = received_requests_json_.back(); auto resource_span = *check_json["resource_spans"].begin(); auto instrumentation_library_span = *resource_span["instrumentation_library_spans"].begin(); From d82b72abd3f62e0d85d0db937fbb5a67a61c4414 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 14 Jun 2021 23:34:36 -0700 Subject: [PATCH 4/5] Fix missing cv.notify_one for BasicCurlHttpTests --- ext/test/http/curl_http_test.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 31c0d06570..121dbd143a 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -99,13 +99,14 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe virtual int onHttpRequest(HTTP_SERVER_NS::HttpRequest const &request, HTTP_SERVER_NS::HttpResponse &response) override { + int response_status = 404; if (request.uri == "/get/") { std::unique_lock lk(mtx_requests); received_requests_.push_back(request); response.headers["Content-Type"] = "text/plain"; - return 200; + response_status = 200; } if (request.uri == "/post/") { @@ -113,9 +114,12 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe received_requests_.push_back(request); response.headers["Content-Type"] = "application/json"; response.body = "{'k1':'v1', 'k2':'v2', 'k3':'v3'}"; - return 200; + response_status = 200; } - return 404; + + cv_got_events.notify_one(); + + return response_status; } bool waitForRequests(unsigned timeOutSec, unsigned expected_count = 1) From 97a5cc8c26714a42a9d361c005d14452dd6d7e2b Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Tue, 15 Jun 2021 00:42:50 -0700 Subject: [PATCH 5/5] Remove stale TODO --- .../include/opentelemetry/exporters/otlp/otlp_http_exporter.h | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h index 777221d23e..0d6f04e653 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h @@ -67,7 +67,6 @@ struct OtlpHttpExporterOptions bool console_debug = false; // TODO: Enable/disable to verify SSL certificate - // TODO: Reuqest timeout std::chrono::milliseconds timeout = std::chrono::milliseconds(30000); };