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

Use app's thread context class loader for callbacks into app #9000

Merged
merged 8 commits into from
Aug 9, 2023
Merged

Use app's thread context class loader for callbacks into app #9000

merged 8 commits into from
Aug 9, 2023

Conversation

trask
Copy link
Member

@trask trask commented Jul 20, 2023

Wondering if there's any reason not to do this?

The test is not very realistic. The problem is that the agent's thread context class loader appears to be ClassLoader.getSystemClassLoader(), which works in simple unit tests where the app is also in the system class loader. Maybe a smoke test (in an app server with nested class loaders) would be better?

@trask trask requested a review from a team July 20, 2023 21:01
@mateuszrzeszutek
Copy link
Member

Is there a problem with how the gauge functions are run? Why would we want to touch the context CL in the first place?

@laurit
Copy link
Contributor

laurit commented Jul 21, 2023

This has come up previously in #7391 #7220 Probably it would be more correct to use the context class loader from the time when the instrument was registered.

@mateuszrzeszutek
Copy link
Member

Thanks, I completely forgot about that issue 😅 (I did remember removing the setContextClassLoader() calls though)

The same issue also applies to FunctionCounter and FunctionTimer, we should update them too. (And the OTel metrics API itself... though we might want to leave fixing that for later)

@mateuszrzeszutek
Copy link
Member

Probably it would be more correct to use the context class loader from the time when the instrument was registered.

Context classloader, and if not set then obj.getClass().getClassloader()?

@sebastian-hans-swm
Copy link

Thank you for taking up the issue! We had to disable a few interesting metrics due to this when moving to metrics collection via the agent. I'm really looking forward to re-enabling them.

@laurit
Copy link
Contributor

laurit commented Aug 2, 2023

@sebastian-hans-swm could you elaborate why it fails for you, are you doing something similar to #7220

@sebastian-hans-swm
Copy link

@laurit, we are using a Gauge to keep track of the number of Quartz jobs in our application. The Quartz API allows us to query its triggers from the database. Somewhere in the call chain Quartz tries to create a proxy for the JDBC Connection (the implementation of which is loaded lazily by Spring Boot magic) using the thread context class loader – and fails.

@trask
Copy link
Member Author

trask commented Aug 8, 2023

Probably it would be more correct to use the context class loader from the time when the instrument was registered.

done 👍

Context classloader, and if not set then obj.getClass().getClassloader()?

it seems more precise to only carry over the context class loader from registration to execution, but I could easily be convinced otherwise

The same issue also applies to FunctionCounter and FunctionTimer, we should update them too

added tests for these (they are covered by the same underlying change) 👍

(And the OTel metrics API itself... though we might want to leave fixing that for later)

opened open-telemetry/opentelemetry-java#5694 👍

@mateuszrzeszutek
Copy link
Member

it seems more precise to only carry over the context class loader from registration to execution, but I could easily be convinced otherwise

I don't have a strong opinion (in fact it's so weak it's barely there), I'd be fine with merging this and waiting for further bug reports.

@trask trask merged commit aab8817 into open-telemetry:main Aug 9, 2023
44 checks passed
@trask trask deleted the micrometer-gauge-thread-context-class-loader branch August 9, 2023 17:17
breedx-splk pushed a commit to breedx-splk/opentelemetry-java-instrumentation that referenced this pull request Aug 15, 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