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

[Feature]: remove async runtime dependency in batch processors #2066

Open
ThomsonTan opened this issue Aug 29, 2024 · 3 comments · May be fixed by #2096
Open

[Feature]: remove async runtime dependency in batch processors #2066

ThomsonTan opened this issue Aug 29, 2024 · 3 comments · May be fixed by #2096
Labels
enhancement New feature or request triage:todo Needs to be traiged.

Comments

@ThomsonTan
Copy link
Contributor

Related Problems?

As mentioned in item 7 in #1968, the current bath processes requires an async runtime (see here, there are pros and cons for this dependency than creating a dedicated background thread to export for each batch processor, while the latter approach is implemented in both OpenTelemetry C++ and OpenTelemetry .NET SDKs.

  • Pros
    • No dedicated thread requirement.
    • Reuse the existing runtime if the application already depends on it.
  • Cons
    • Handle telemetry export in the application's async runtime and interfere with the business logic processing.
    • Force adding the dependence on async runtime which may not be available for some environment like IoT or others.
    • The dedicated background exporting thread number could be unbound.

Describe the solution you'd like:

Based on the current implementation in other SDKs and the pros and cons above, I suggest we switch to export in background thread instead of sending the export task to user configured async runtime.

Considered Alternatives

No response

Additional Context

No response

@ThomsonTan ThomsonTan added enhancement New feature or request triage:todo Needs to be traiged. labels Aug 29, 2024
@TommyCpp
Copy link
Contributor

TommyCpp commented Aug 30, 2024

Are we offering any async runtime in the background thread? If not, are we revising the exporting APIs to be sync?

@TommyCpp
Copy link
Contributor

TommyCpp commented Aug 30, 2024

The dedicated background exporting thread number could be unbound

It won't, since today we model the exporting as a task sent to runtime, the runtime will manage how many thread to use & when to schedule which task. Opentelemetry go uses similar setup. They don't need users to passing in async runtime because Golang have one official async runtime/task scheduler/goroutine scheulder/coroutine scheduler

Handle telemetry export in the application's async runtime and interfere with the business logic processing

Could you elaborate on this? Generally with async runtime, both tasks from business logic and telemetry exporting are managed by task schedule in async runtime. With background thread, the tasks from business logic and telemetry exporting thread are managed by OS thread scheduler.

Force adding the dependence on async runtime which may not be available for some environment like IoT or others

This is a valid point for adding a new batch processor for those environments. Not sure if it worth abadoning the support to allow users to bring their own runtime

@lalitb
Copy link
Member

lalitb commented Sep 9, 2024

Have done a POC for adding a custom thread runtime implementation for the Runtime and RuntimeChannel abstractions. This uses futures_executor for executing the future, and std::mpsc for channel (which can be abstracted further). Have also added a logs examples with batch processor using this custom runtime if someone wants to try.

https://github.com/open-telemetry/opentelemetry-rust/compare/main...lalitb:thread-runtime?expand=1

This creates a thread-pool and the motive is to multiplex different async tasks (for batch and periodic) across these threads. As of now, it uses a single available dedicated thread for each export (this needs to be fixed). Also, delay and interval schedulers execute on dedicated thread, which would be fixed. And the mpsc channel is used in unbound mode for now. This is just a draft, and I can make it more complete if this is something we want to look forward to,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage:todo Needs to be traiged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants