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 VirtualField for associating netty listener with wrapper #5282

Merged
merged 2 commits into from
Jan 31, 2022

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Jan 31, 2022

Probably resolves #5265
To support removing netty listeners we need to be able to find the wrapper that we register instead of the original listener from the original listener. Currently we use a static weak cache for it. Removal of the elements from this cache depends on the gc, which might confuse users who inspect the heap dump and make them suspect that there is a memory leak.

@laurit laurit requested a review from a team January 31, 2022 13:11
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.

I didn't follow the connection to skipping transforming lambdas of injected helper classes?

@laurit
Copy link
Contributor Author

laurit commented Jan 31, 2022

@trask I should have added a comment for this. As far as I understand we don't transform helper classes, so not transforming their lambdas should really be the expected behaviour. Transforming lambdas of helper classes can fail because some bytebuddy matcher tries to resolved the class file of containing class of lambda, which does not exist, because it is an injected class. We make tests fail when there is a transformation failure. Adding a virtual field for GenericFutureListener causes such failures.

@trask
Copy link
Member

trask commented Jan 31, 2022

oh, i c, and non-lambda injected classes are skipped already because they live under "io.opentelemetry.javaagent.shaded" which we skip in GlobalIgnoredTypesConfigurer

would it work to move the isHelperClass check up to GlobalIgnoredTypesConfigurer?

@laurit
Copy link
Contributor Author

laurit commented Jan 31, 2022

oh, i c, and non-lambda injected classes are skipped already because they live under "io.opentelemetry.javaagent.shaded" which we skip in GlobalIgnoredTypesConfigurer

would it work to move the isHelperClass check up to GlobalIgnoredTypesConfigurer?

I thought it was because the injection happens during transformation and transformation isn't re-entered. Only helper classes from library modules have "shaded".
Using GlobalIgnoredTypesConfigurer would require some kind of heuristics as you need to configure it before classes are actually injected. We would need to use something like class name starts with "io.opentelemetry.javaagent." and ends with "$Lambda*"

@trask
Copy link
Member

trask commented Jan 31, 2022

oh of course, I forgot that GlobalIgnoredTypesConfigurer is just used to build up the trie 👍

@trask
Copy link
Member

trask commented Jan 31, 2022

oh yes, and this:

I thought it was because the injection happens during transformation and transformation isn't re-entered

and I missed that the check isn't whether the lambda class itself is a helper class, but rather it's whether the lambda's targetClass is a helper class.

btw, what is the targetClass? is it the "outer" class where the lambda lives? or the target interface of the lambda?

thx 😅

@laurit
Copy link
Contributor Author

laurit commented Jan 31, 2022

btw, what is the targetClass? is it the "outer" class where the lambda lives? or the target interface of the lambda?

thx 😅

targetClass is the class that declares the lambda. Not the best name. It has the same value as https://github.com/openjdk/jdk11/blob/37115c8ea4aff13a8148ee2b8832b20888a5d880/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java#L54

@trask trask merged commit fbf0076 into open-telemetry:main Jan 31, 2022
@laurit laurit deleted the netty-virtual-field branch February 1, 2022 11:02
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
…lemetry#5282)

* Use VirtualField for associating netty listener with wrapper

* Move ignoring lambas for injected classes to LambdaTransformer
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.

javaagent causes big heap on high load for Netty instrumentation
3 participants