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

Using libcurl-multi to implement opentelemetry::ext::http::client::curl::HttpClient #1243

Closed
owent opened this issue Mar 4, 2022 · 11 comments
Assignees

Comments

@owent
Copy link
Member

owent commented Mar 4, 2022

Is your feature request related to a problem?

To improve IO performance as mentioned in #1175 and #1176

Describe the solution you'd like

Using libcurl-multi instead of invoking threads for HttpClient.

Additional context

@owent
Copy link
Member Author

owent commented Mar 4, 2022

This issue can be assigned to me, and I will finish it several days later.There will not be any conflict with #1209 .

@lalitb
Copy link
Member

lalitb commented Mar 4, 2022

Thanks. Was thinking to create an issue for this.

@owent owent changed the title Using libcurl-multi to implement opentelemetry::ext::http::client::curl Using libcurl-multi to implement opentelemetry::ext::http::client::curl::HttpClient Mar 4, 2022
@ThomsonTan
Copy link
Contributor

Do we need provide an option for libcurl vs. libcurl-multi? I think libcurl-multi could take more resource usage, so may be not expected for some scenario?

@owent
Copy link
Member Author

owent commented Mar 4, 2022

Do we need provide an option for libcurl vs. libcurl-multi? I think libcurl-multi could take more resource usage, so may be not expected for some scenario?

I think we can use libcurl-multi in SendAsync and still use libcuel-easy in Send.
To my understanding, libcurl-multi do not cost more because we use select() to wait for response , when we use libcurl-multi, we just use curl_multi_poll() to replace select().

@owent
Copy link
Member Author

owent commented Apr 3, 2022

Can I do some break changes to curl::HttpOperator to finish this feature? multi_handle will be visited by more than one thread, I have to change the way to share data to keep thread-safety and I think curl::HttpOperator is not for SDK's user.

@owent owent mentioned this issue Apr 5, 2022
3 tasks
@meastp
Copy link
Contributor

meastp commented Apr 28, 2022

Hi - I'm using the otlp http exporter, and I'm hitting the issue described in this thread and updating the registry key resolves the issue. However, as mentioned in the thread, the way curl_easy_init() and curl_easy_cleanup() is used isn't good. What I'm wondering is if this issue is also resolved when moving to libcurl-multi?

(I think the issue is that a new session (with calling both curl_easy_init() and curl_easy_cleanup() every time) is used every time Export is called.)

@lalitb
Copy link
Member

lalitb commented Apr 28, 2022

I think the issue is that a new session (with calling both curl_easy_init() and curl_easy_cleanup() every time) is used every time Export is called

Is the issue due to local port reuse/overlap? In current setup, the sessions are not concurrent, so the problem should only happen if cleanup is not happening properly in some scenario.

@meastp
Copy link
Contributor

meastp commented Apr 28, 2022

@lalitb Yes, I believe that's the cause, but I'm not sure about the specifics. It might be Windows specific, but please reuse the CURL* resource (allocated with curl_easy_init() if possible, to avoid this problem.

(I implemented a move assignment operator to reuse CURL in HttpOperation, but it did not help, and needs to be done on a higher level)

The problem disappeared in my tests (no output to stdout/stderr from otel-cpp/curl) after creating the registry key as specified in the linked thread above. I modified to curl_easy_setopt(curl_, CURLOPT_VERBOSE, 1L); to debug/read output from Curl, because the output from otel-cpp was rather sparse.

@owent
Copy link
Member Author

owent commented Apr 28, 2022

Hi - I'm using the otlp http exporter, and I'm hitting the issue described in this thread and updating the registry key resolves the issue. However, as mentioned in the thread, the way curl_easy_init() and curl_easy_cleanup() is used isn't good. What I'm wondering is if this issue is also resolved when moving to libcurl-multi?

(I think the issue is that a new session (with calling both curl_easy_init() and curl_easy_cleanup() every time) is used every time Export is called.)

We still need curl_easy_init() and curl_easy_cleanup() when using libcurl-multi, curl_multi_handle will maintain all curl_easy_handle and manage connections, it also allow us to reuse connections between a lot of curl_easy_handle.

This jobs is already finished in #1317 . But it will be merged into async-changes first.

@meastp
Copy link
Contributor

meastp commented Apr 29, 2022

(...) it also allow us to reuse connections for a lot of curl_easy_handle.

@owent This is the crucial bit that will solve the problem, I think. Thanks :)

@github-actions
Copy link

github-actions bot commented Jul 1, 2022

This issue was marked as stale due to lack of activity.

@github-actions github-actions bot added the Stale label Jul 1, 2022
@lalitb lalitb removed the Stale label Jul 1, 2022
@owent owent closed this as completed Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants