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

Prevent infinite traces when automatically instrumenting OkHttp #5886

Closed
LikeTheSalad opened this issue Oct 6, 2023 · 2 comments
Closed
Labels
Feature Request Suggest an idea for this project

Comments

@LikeTheSalad
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Adding automatic instrumentation support for OkHttp calls to the Android repo is done at compile time, so the classes involved (in this case OkHttp ones) get their code changed in order to create traces for every HTTP request. The problem with that is that some OTel exporters make use of OkHttp, which would cause a new trace to be created for a new export, getting into an infinite loop of exports and traces.

This isn't a problem for plain-java automatic instrumentation, as they have a separation between the classloader that belongs to the host app, and the classloader for the OTel agent. It is my understanding that each classloader can have its own copy of OkHttp, so when the host app's classloader gets instrumented, it doesn't affect the agent's copy of OkHttp. However, in Android we don't have a way to modify an app's bytecode at runtime, so it's done at compile time where a single copy of OkHttp gets packaged into the app so it would affect all its usages, including OTel's.

Describe the solution you'd like
After taking a look at this issue and this PR, I think a good solution could be to add some key in the Context when using OkHttp from OTel, so that we can later check it out in the code that automatically creates traces in OkHttp, so that we can ignore calls that come from OTel exporters.

Describe alternatives you've considered
There were a couple of alternatives discussed in yesterday's Java SIG:

  • Changing the thread name to denote that the current thread used by OkHttp belongs to OTel, so that we can later check the thread name before creating a trace. - I have concerns with this approach because the OTel Java SDK uses OkHttp's enqueue method which seems to rename the thread that runs the call, before going through the interceptors which is where we create a trace.
  • Setting some value in the ThreadLocal that we could check later before creating a trace. - This seems similar to adding some value to the Context. There's one benefit of using the Context instead, which is that there are already mechanisms in place to preserve the Context across an OkHttp thread change, meaning that, in theory, we could set a Context key in here for example, and that would be carried over to the thread managed by OkHttp later. If we go with the ThreadLocal approach, we'd have to somehow move its value to another thread or we'd have to intercept the thread that OkHttp will use and set the keys directly into it.

Additional context
There was a concern raised in yesterday's SIG about using a Context key, which is related to performance penalties of doing so for every OkHttp request. I think it's a valid concern, although I'm not sure how much it applies to a client use-case, definitely would be a problem for a server that would need to check a Context key for every incoming request, but for a client, I'm not sure, it's probably worth doing a benchmark on it to have a better idea of how long it takes to retrieve a value from a Context.

@LikeTheSalad
Copy link
Contributor Author

This is a context key approach used in JS.

@jack-berg
Copy link
Member

@LikeTheSalad I believe this is resolved in #5918, please re-open if I'm mistaken.

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