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

Jump to bottom

Fix missing notify of conditional variable in OTLP HTTP exporter test #858

Merged
merged 6 commits into from
Jun 15, 2021
Merged

Fix missing notify of conditional variable in OTLP HTTP exporter test #858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2054db9
Fix missing notify of conditional variable in OTLP HTTP exporter test
ThomsonTan Jun 14, 2021
3d7fba4
Format code
ThomsonTan Jun 15, 2021
92ca920
Restore the wait timeout
ThomsonTan Jun 15, 2021
d82b72a
Fix missing cv.notify_one for BasicCurlHttpTests
ThomsonTan Jun 15, 2021
97a5cc8
Remove stale TODO
ThomsonTan Jun 15, 2021
066484a
Merge branch 'main' into FixMissingNotify
lalitb Jun 15, 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: 0 additions & 1 deletion exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};

Expand Down
22 changes: 14 additions & 8 deletions exporters/otlp/test/otlp_http_exporter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -102,8 +104,8 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS::
}
else
{
response.body = "{\"code\": 400, \"message\": \"Parse binary failed\"}";
return 400;
response.body = "{\"code\": 400, \"message\": \"Parse binary failed\"}";
response_status = 400;
}
}
else if (nullptr != request_content_type && *request_content_type == kHttpJsonContentType)
Expand All @@ -112,8 +114,8 @@ 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\"}";
return 400;
response.body = "{\"code\": 400, \"message\": \"Parse json failed\"}";
response_status = 400;
}
else
{
Expand All @@ -123,19 +125,23 @@ class OtlpHttpExporterTestPeer : public ::testing::Test, public HTTP_SERVER_NS::
}
else
{
response.body = "{\"code\": 400, \"message\": \"Unsupported content type\"}";
return 400;
response.body = "{\"code\": 400, \"message\": \"Unsupported content type\"}";
response_status = 400;
}

return 200;
response_status = 200;
}
else
{
std::unique_lock<std::mutex> lk(mtx_requests);
response.headers["Content-Type"] = "text/plain";
response.body = "404 Not Found";
return 200;
response_status = 200;
}

cv_got_events.notify_one();
Copy link
Member

@lalitb lalitb Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

why is this needed ? cv.wait_for is supposed to be unblocked with a timeout. I think this will fail now if the test is modified so that cv.wait_for has multiple requests to wait for, as now it will be unblocked after a single request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timeout in cv.wait_for is the last resort. cv should be waked up anytime its checking condition could be changed, so its condition could be re-checked instead of waiting for timing out. notify_one just tell cv to recheck so it should not trigger failure behavior?

cc @owent

Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that without notification, the cv will keep blocked for 2sec (as configured) even though the condition is satisfied early?

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, this is the case the PR trying to fix.

Copy link
Member

@lalitb lalitb Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Asking because we are using this construct in other places too:

opentelemetry-cpp/ext/test/http/curl_http_test.cc

Line 124 in 7fca2c3

if (cv_got_events.wait_for(lk, std::chrono::milliseconds(1000 * timeOutSec),
, probably they need a change too ( in separate PR ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it is needed in curl_http_test.cc as well. Added the change to the PR and avoid another 2 timeout to happen there. Thanks for the catch.

Copy link
Member

Choose a reason for hiding this comment

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

Timeout in cv.wait_for is the last resort. cv should be waked up anytime its checking condition could be changed, so its condition could be re-checked instead of waiting for timing out. notify_one just tell cv to recheck so it should not trigger failure behavior?

cc @owent

You are right. LGTM.


return response_status;
}

bool waitForRequests(unsigned timeOutSec, size_t expected_count = 1)
Expand Down
10 changes: 7 additions & 3 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 @@ -99,23 +99,27 @@ 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<std::mutex> lk(mtx_requests);
received_requests_.push_back(request);
response.headers["Content-Type"] = "text/plain";
return 200;
response_status = 200;
}
if (request.uri == "/post/")
{
std::unique_lock<std::mutex> lk(mtx_requests);
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)
Expand Down