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

Set up virtual field transforms before otel sdk is initialized #12444

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Oct 15, 2024

Hopefully resolves #12303
Set up virtual field transforms for executors instrumentation before otel sdk is initialized. This should reduce usage of the fallback map.

@laurit laurit requested a review from a team as a code owner October 15, 2024 12:15
@trask
Copy link
Member

trask commented Oct 25, 2024

cc @JonasKunz @SylvainJuge in case one of you happens to have time to review

Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I just wonder where the actual memory leak in #12303 you think comes from?

In order for an entry in WeakHashMap to leak, the value (e.g. the Otel context) needs to have a transitive reference to the key with which it is stored in the map. I currently can't imagine how the Otel-context would have a reference to e.g. it's FutureTask? If you have any explanation that would be appreciated, but it shouldn't block this PR.

* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
* at any time.
*/
public interface EarlyInstrumentationModule extends Ordered {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any things an EarlyInstrumentationModule must not do in it's constructor / static initializers? E.g. logging maybe?

If yes, it would be great to add those restrictions to the javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are not fully set up when we start looking for EarlyInstrumentationModule instances there definitely will be things, such as reading configuration, that won't work. In this PR I moved calling ExperimentalConfig.get() out of the static initializer of InstrumentationModule. In my opinion it is too hard to predict what exactly can or can not be called. I would deal with these issues as they come up. There is a good chance there only the executor instrumentation will use EarlyInstrumentationModule.

@laurit
Copy link
Contributor Author

laurit commented Oct 25, 2024

LGTM, I just wonder where the actual memory leak in #12303 you think comes from?

In order for an entry in WeakHashMap to leak, the value (e.g. the Otel context) needs to have a transitive reference to the key with which it is stored in the map. I currently can't imagine how the Otel-context would have a reference to e.g. it's FutureTask? If you have any explanation that would be appreciated, but it shouldn't block this PR.

I don't think there is a leak in that sense. Without a heap dump we can only speculate what is going on. My guess is that everything in the weak map can be collected, but since the app is running with a large heap gc lets the virtual field maps grow very large. We could approach this either by avoiding the fallback map for virtual fields or by setting the field to null when we are done with. #12397 implements clearing virtual field on runnables. We'd also need to clear the virtual field on futures, but this is a difficult. Probably it would make sense to restrict the future instrumentation to more specific futures such as FutureTask where we could clear the state immediately when the value is set for the future. Or we could drop the future instrumentation altogether.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @JonasKunz!

@trask trask merged commit 68ee2f6 into open-telemetry:main Oct 25, 2024
56 checks passed
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

Successfully merging this pull request may close these issues.

Otel Java Agent Causing Heap Memory Leak Issue
3 participants