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.

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

config timeout with CURLOPT_TIMEOUT_MS instead of CURLOPT_TIMEOUT for curl implementation #1258

Closed
lalitb opened this issue Mar 10, 2022 Discussed in #1254 · 3 comments · Fixed by #1261
Closed

config timeout with CURLOPT_TIMEOUT_MS instead of CURLOPT_TIMEOUT for curl implementation #1258

lalitb opened this issue Mar 10, 2022 Discussed in #1254 · 3 comments · Fixed by #1261
Labels
area:exporter bug Something isn't working do-not-stale priority:p2 Issues that are not blocking

Comments

@lalitb
Copy link
Member

lalitb commented Mar 10, 2022

Discussed in #1254

Originally posted by wxl374 March 10, 2022
When configure OtlpHttpExporter's timeout less than 1s, it will set to 0 for the use of CURLOPT_TIMEOUT(

curl_easy_setopt(curl_, CURLOPT_TIMEOUT, http_conn_timeout_.count() / 1000);
), then never times out (sometimes limited by CURLOPT_LOW_SPEED_TIME). Why not use CURLOPT_TIMEOUT_MS instead?

@lalitb
Copy link
Member Author

lalitb commented Mar 10, 2022

This is a good point. Would be good to see the libcurl version having CURLOPT_TIMEOUT_MS support before supporting this. Moving this to issue to investigate further.

@lalitb lalitb added area:exporter bug Something isn't working priority:p2 Issues that are not blocking do-not-stale labels Mar 10, 2022
@wxl374
Copy link
Contributor

wxl374 commented Mar 11, 2022

CURLOPT_TIMEOUT_MS was provided from 7.16.2
https:/curl/curl/blob/e87c53d7eadd52d2f18cf00241d6afef958fa881/docs/libcurl/symbols-in-versions#L666
how about selected CURLOPT_TIMEOUT_MS by checking libcurl version?
https:/curl/curl/blob/master/include/curl/curlver.h

@lalitb
Copy link
Member Author

lalitb commented Mar 11, 2022

CURLOPT_TIMEOUT_MS was provided from 7.16.2

Thanks, 7.16.2 seems long back (~2007), I think we can safely use CURLOPT_TIMEOUT_MS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:exporter bug Something isn't working do-not-stale priority:p2 Issues that are not blocking
Projects
None yet
2 participants