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

Proposal: Expose a Method to Check if an Executor is Already Wrapped by OpenTelemetry #6955

Open
ShadabGentle opened this issue Dec 13, 2024 · 1 comment
Labels
Feature Request Suggest an idea for this project

Comments

@ShadabGentle
Copy link

Description
Currently, the io.opentelemetry.context.CurrentContextExecutorService class, used to wrap ExecutorService with tracing context propagation, is non-public. As a result, there is no public API to determine if an ExecutorService is already wrapped. This limitation makes it challenging to avoid excessive wrapping or recursive wrapping in scenarios where multiple components may apply OpenTelemetry wrapping independently.

Use Case
In our application, we use OpenTelemetry to enable tracing across multiple services. During the setup of our custom Aspect for MQTT consumers, we encountered a scenario where multiple layers could potentially wrap an ExecutorService. Since CurrentContextExecutorService is non-public, there is no way to check if an executor is already wrapped.

The absence of such a mechanism leads to potential issues:

1. Performance Overhead: Re-wrapping executors multiple times unnecessarily adds extra layers and CPU/memory usage.
2. Circular Dependencies: Repeated wrapping can result in unintended behaviors, including stack overflow exceptions in some cases.
A public method to check if an executor is already wrapped would allow developers to handle this elegantly and avoid redundant wrapping.

Proposed Solution
Expose a utility method or make CurrentContextExecutorService public, enabling developers to check if an 'ExecutorService' is already wrapped. For example:

public static boolean isContextWrapped(Executor executor) {
    return executor instanceof CurrentContextExecutorService;
}

Alternatively, add a public method in Context:

public static boolean isTaskWrapping(Executor executor) {
    // Internal check for wrapping
}

This approach would make it straightforward for developers to ensure their wrapping logic is idempotent.

Potential Impact
1. Improved Developer Experience: Simplifies scenarios where multiple layers apply OpenTelemetry wrapping, improving usability.
2. Avoid Performance Overhead: Ensures that executors are wrapped only once, reducing unnecessary overhead.
3. Enhances Debugging: Provides better insight for developers when debugging issues related to executor wrapping.

Steps to Reproduce the Issue

  1. Create an ExecutorService.
  2. Wrap the executor using Context.taskWrapping(executor).
  3. Attempt to check if the executor is already wrapped without relying on non-public classes.

Request for Feedback
I would appreciate feedback on this proposal and happy to contribute a pull request if maintainers are open to this enhancement.

Call to Action
Please consider this feature request for inclusion in a future release of OpenTelemetry. Let us know if there's an alternative approach we can take to solve this issue.

@ShadabGentle ShadabGentle added the Feature Request Suggest an idea for this project label Dec 13, 2024
@jack-berg
Copy link
Member

Another options would be to adjust the behavior of the utility methods that provide the wrapped Executors to not duplicate the wrapping. I.e. something like:

default ExecutorService wrap(ExecutorService executor) {
  if (executor instanceof ContextExecutorService) {
    return executor;
  }
  return new ContextExecutorService(this, executor);
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

2 participants