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

Associate value with class loader #10051

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Dec 12, 2023

Added value will behave as if it was stored in a field in the class loader object, meaning that the value can be garbage collected once the class loader is garbage collected and referencing the class loader from the value will not prevent garbage collector from collecting the class loader.
Hopefully this will help with the indy instrumentation as this will let us associate InstrumentationModuleClassLoader and whatever else is needed with the application class loader without the need to use weak references and fear that our stuff gets collected before the class loader.

@laurit laurit requested a review from a team December 12, 2023 12:14
@laurit
Copy link
Contributor Author

laurit commented Dec 12, 2023

@JonasKunz could you review whether this would be useful for #10015

@zeitlinger
Copy link
Member

👍

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.

I think this will simplify #10015 significantly:
We can then just eagerly create the InstrumentationModuleClassloader when a TypeInstrumentation is matched.
I'll use ClassLoaderValue<Map<String, InstrumentationModuleClassloader>>, where the String-keys are the module group names.

This also solves the TypePool question when injecting proxies: We'll be able to use a normal TypePool for the eagerly created InstrumentationModuleClassloader here.

Where it unfortunately doesn't help is with the TypePool for the muzzle-matcher, but that is okay I guess.

I'll updated my PR once this one is merged, nice work!

private static final Cache<ClassLoader, WeakReference<Map<Object, Object>>> data = Cache.weak();

public static Object get(ClassLoader classLoader, Object key) {
return getClassLoaderData(classLoader).get(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpick] might consider adding a boolean initialize parameter to getClassLoaderData so that we don't do the heavy initialization work when just checking for the presence of a key.

If initialize = false, getClassLoaderData will return null if the map for the classloader hasn't been initialized yet.

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.

👍

@trask trask merged commit a7e8ed8 into open-telemetry:main Dec 14, 2023
47 checks passed
elbiocaetano pushed a commit to elbiocaetano/opentelemetry-java-instrumentation that referenced this pull request Dec 19, 2023
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.

4 participants