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

Could we add worker thread pool for BatchLogProcessor and BatchSpanProcessor? #1175

Closed
owent opened this issue Jan 17, 2022 · 7 comments
Closed
Assignees

Comments

@owent
Copy link
Member

owent commented Jan 17, 2022

Is your feature request related to a problem?

In some system , we have a lot of logs to commit in one batch and it takes a long time to receive the response.(via BatchLogProcessor and OtlpHttpLogExporter, it takes about 100-200ms to get the http response).The worker thread cost only about 10% of CPU time but it will drop logs because the circular buffer is full when waiting for response of previous http request.

Describe the solution you'd like

I thinkg maybe it can be solved by adding worker pool to BatchLogProcessor and BatchSpanProcessor or use asynchronous IO APIs of gRPC or HTTP client.I think it's easier to implement a worker pool.

Describe alternatives you've considered

Now, HttpOperation::SendAsync spawn a new thread by std::async and use the synchronous APIs of curl.Maybe we can also use curl_multi_* and do not wait for reponse in OtlpHttpClient::Export to optimize the performance?

This can assign to me if the solution is discussed and acceptable.

@lalitb
Copy link
Member

lalitb commented Jan 17, 2022

The worker thread cost only about 10% of CPU time but it will drop logs because the circular buffer is full when waiting for response of previous http request.

How is thread pool going to help here, as it still needs to wait for a response from ongoing Export() before invoking another? We can't have concurrent execution of Export() method from multiple threads(as per the specs). Also, the Export() should return with the status of export over the wire (as per the specs). I think it's important to specify the proper values for BatchSpanProcessor configuration parameters to minimize dropping logs/spans, along with optimal memory usage.

Sorry if I am missing the approach suggested here, we can discuss more in that case :)

@owent
Copy link
Member Author

owent commented Jan 17, 2022

The worker thread cost only about 10% of CPU time but it will drop logs because the circular buffer is full when waiting for response of previous http request.

How is thread pool going to help here, as it still needs to wait for a response from ongoing Export() before invoking another? We can't have concurrent execution of Export() method from multiple threads(as per the specs). Also, the Export() should return with the status of export over the wire (as per the specs). I think it's important to specify the proper values for BatchSpanProcessor configuration parameters to minimize dropping logs/spans, along with optimal memory usage.

Sorry if I am missing the approach suggested here, we can discuss more in that case :)

With worker pool, we can start more http requests parallel. I mean let BatchSpanProcessor contains more than one SpanExporter and also let BatchLogProcessor contains more than one LogExporter, and create a worker thread for each Exporter. All SpanExporter::Export() and LogExporter::Export() are still run in their own thread and will not execute concurrently.
We can dispatch logs and spans by hash of trace_id or log_name .

@lalitb
Copy link
Member

lalitb commented Jan 17, 2022

Ok thanks for explaining. Do you think its possible to do a proof of concept for logs, with some benchmark and discuss over it? If it works, we can implement it for traces too. Will assign this issue to you.

@owent
Copy link
Member Author

owent commented Jan 17, 2022

Ok and thanks.

@owent
Copy link
Member Author

owent commented Jan 17, 2022

I create another issue for make OtlpHttpClient asynchronously. #1176

@owent
Copy link
Member Author

owent commented Jan 24, 2022

I'm a little busy recently and I may finish this several days later.

@owent
Copy link
Member Author

owent commented Mar 4, 2022

This issue is splited into #1175 and #1243.

@owent owent closed this as completed Mar 4, 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

2 participants