Fix inner classes matching for line probes#4604
Merged
Merged
Conversation
When class are already loaded, we need to retransform them for passing through the DebuggerTransformer. But to detect which class need to be retransformed, we rely on the naming. However inner classes or Top-Level classes are in independent class files and the only way to associate them is the sourcefile attribute in each class file. So we need to track all dependent classes by their source file in order to avoid scanning all loaded classes to parse their sourcefile and try to match with actual probe definitions.
shatzi
approved these changes
Jan 26, 2023
shatzi
left a comment
shatzi
left a comment
Contributor
There was a problem hiding this comment.
Awesome stuff. few refactor suggestions
| sourceFile = sourceFile.substring(idx + 1); | ||
| } | ||
| List<String> additionalClasses = | ||
| SourceFileTrackingTransformer.INSTANCE.getClassNameBySourceFile(sourceFile); |
Contributor
There was a problem hiding this comment.
I don't like that use static INSTANCE and creating hidden dependencies that complicate how we do unit testing.
Lets clean this up to make more single responsibility classes
My take on this would be:
- Extract all getLoadedClasses logic from this class. maybe
ClassesToRetansformFinderthat gets list of changes, loaded classes and classFileMapping provider to do all this logic.
a. that would make the configurationComparer simpler
b. remote the dependency of SourceFileTrackingTransformer - ClassesToRetansformFinder can have a function "registerClassWithCustomSourceName(classname, sourceilfe)
a. then SourceFileTrackingTransformer can be created debuggerAgent an pass the instance of ClassesToRetansformFinder to it to register classes
b. configuration updater can use ClassesToRetansformFinder.getAllChangedClasses(changes)
WDYT?
| } | ||
| } | ||
|
|
||
| private String removeExtension(String fileName) { |
Contributor
There was a problem hiding this comment.
Maybe move all those helper functions to ClassFileHelper utility classes - to have a single place when we load file name, normalize it, etc.
| private static boolean lookupClass(Trie changedClasses, Class<?> clazz) { | ||
| String reversedTypeName = reverseStr(clazz.getName()); | ||
| // try first with FQN (java.lang.String) | ||
| if (changedClasses.contains(reversedTypeName)) { |
Contributor
There was a problem hiding this comment.
why not doing containsPrefix? as it the same we do at the end.
| if (sourceFile == null) { | ||
| return null; | ||
| } | ||
| String simpleClassName = className.substring(className.lastIndexOf('/') + 1); |
Contributor
There was a problem hiding this comment.
look like the normalize or removeExtention functions?
jpbempel
force-pushed
the
jpbempel/fix-kotlin-lambda
branch
from
January 27, 2023 14:49
ab594c2 to
f85142e
Compare
handle the resolution of classes that need to be retransformed based on SourceFile and probe definitions
jpbempel
force-pushed
the
jpbempel/fix-kotlin-lambda
branch
from
January 27, 2023 14:53
f85142e to
01ee5e0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What Does This Do
Allows the DebuggerTransformer to find inner classes when using file+line probe definitions.
Reporting of line not found was removed because it can be found latter by loading inner/top-level class.
Background
When class are already loaded, we need to retransform them for passing through the
DebuggerTransformer.But to detect which class need to be retransformed, we rely on the naming. However inner classes or Top-Level classes are independent class files and the only way to associate them is the sourcefile attribute in each class file.
Solution
Track all dependent classes by their source file, so it easier to find all related class given a filename.
This avoids scanning all loaded classes to parse their sourcefile and try to match with actual probe definitions.
Motivation
Support line probe in inner/top-level classes or Kotlin lambdas
Additional Notes