Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add capability for invokedynamic InstrumentationModules to inject proxies #9565
Add capability for invokedynamic InstrumentationModules to inject proxies #9565
Changes from 11 commits
25a8763
c3a6a13
7a03962
9209cc6
a54e730
57da000
1aa563e
d48563c
43fc618
80dee2f
5adf59e
ac77780
6b9f958
466ca1b
1b3352b
85829bb
4e541ca
0a540a2
cbe27b3
0a3b736
a788f7c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is entirely correct. The reason why there are multiple modules is that muzzle check is done per module. Having sqs and sns instrumentation allows running muzzle checks for these separately so that the main instrumentation module can still apply even if sqs and sns dependencies are missing. Why this works for you when you dump these into one module is because you have disabled the muzzle checks for the indy instrumentation modules, if you'd add back the muzzle check code then I suspect doing this would cause muzzle failures (or if it doesn't then runtime failures when previously there would have been a muzzle failure). I think that using aws instrumentation to test the proxy code was an unfortunate complication because the multimodule thing going on there complicates things. These 3 modules are currently interacting so that if the muzzle checks for the optional modules pass they'll inject the classes that the main module will dynamically discover them. With indy these 3 module are isolated from each other so the trick of dynamically discovering classes injected by another module won't work. The easiest way around this is probably to have one instrumentation class loader per application class loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you could choose some other instrumentation besides aws for testing the proxy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for explaining this!
My prior understanding was that the AWS instrumentation is simply about injecting the library instrumentation as is and using the SQS / SNS discovery built into that library instrumentation. However, for the agent instrumentation we also want to protect agains the case of an SQS / SNS version mismatch, correct?
Sounds good, but I'd propose that the default should be that each
InstrumentationModule
gets its own classloader. I would expect that in most cases, especially with external extensions, this isolation is a feature, not a bug. This allows extensions to freely use utility libraries (e.g. apache commons, guava) without having to fear collisions.I'd propose for
InstrumentationModules
to optionally declare a "module group name". Modules with the same group name instrumenting the same app-classloader would get put into the sameInstrumentationModuleClassLoader
. What do you think about this approach?If you agree, I'd shortly outline this as a TODO comment in the AWS module so that we don't forget about.
Agree, I'll revert the AWS changes and choose a different instrumentation to include in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I had a look at both the apache-dubbo and azur instrumentation and noticed that their tests don't fail currently with
testIndy
even though they should? I tried messing with the instrumentations (e.g. changing the resource paths) and the tests still were green. I'm thinking that something must be wrong with the tests there?I decided to use the log4j instrumentation in this PR, because that one actually fails without the proxying with indy enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least with dubbo the issue is that the javaagent instrumentation has
implementation(project(":instrumentation:apache-dubbo-2.7:library-autoconfigure"))
which leaks over into tests so the tests will pass even without the javaagent instrumentation because they just pick up the manual instrumentation library that gets automatically configuredThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a PR to fix dubbo tests, azure tests (ok I ran only azure-core-1.14) fail for me, perhaps you didn't notice that they have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you run those, I feel like I'm missing something.
What I did was:
isIndyModule()
override from the 1.14AzureSdkInstrumentationModule
AzureSdkTest
from the IDE with-PtestIndy=true
and verified that the module is actually loaded as IndyModule -> the test still passesBtw what is the correct way of running selected tests from the command line?
I tried
./gradlew :instrumentation:azure-core:azure-core-1.14:javaagent:test -PtestIndy=true -i
but that seems to just pick up recent test result from some kind of cache, even if I execute aclean
beforehand:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From command line I think you have to at least do
:instrumentation:azure-core:azure-core-1.14:javaagent:cleanTest
before and then run:instrumentation:azure-core:azure-core-1.14:javaagent:test --no-build-cache
I retested and it still fails for me as expected when running from ide. I also removed the
isIndyModule
method fromAzureSdkInstrumentationModule
and added-PtestIndy=true
to the run options in idea. I ran the test on main not this branch if it matters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have a winner there, that was the difference to make them fail for me too.
I think this can be explained by
InstrumentationModule.registerHelperResources
being non-functional for indy-modules on main. In this PR that functionally has been added back.So for this PR the azure tests should fail because the classes referenced by the injected resources are not supposed to be present in the instrumented classloader, but apparently they are.
I suspect that this then is a testing classpath issue similar to dubbo.
This shouldn't block us on this PR. I'll try to verify my hypothesis and try to open a PR doing the same thing for azure what you did for dubbo.