-
Notifications
You must be signed in to change notification settings - Fork 935
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
indy instrumentation - leftovers migration #13074
Merged
Merged
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
c911faa
lambda
SylvainJuge 26e79f9
inject proxies for indy
SylvainJuge 7f3ae3b
kotlinx + integration tests
SylvainJuge 9183992
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge 29c99dd
inject missing classes for indy
SylvainJuge 45ead47
do not inject proxies for lambda
SylvainJuge ba30ba8
move back comment in the expected location
SylvainJuge eb945b8
remove duplication
SylvainJuge eba7e20
add jpms compatible class file transformer
SylvainJuge 7a1cbc7
remove lambda java9 module
SylvainJuge 590f86e
fix pebkc
SylvainJuge a8cbb24
fix lambda instrumentation, must not redefine them
SylvainJuge 403caaa
fix javadoc
SylvainJuge 506cc62
fix things again
SylvainJuge da1c13d
fix it again
SylvainJuge c4350d0
make the comments a bit better
SylvainJuge 1d54a14
fix typo
SylvainJuge 03aa810
avoid injecting classes for kotlinxcoroutines
SylvainJuge 2ed4e96
remove commented code
SylvainJuge 77a4f00
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge d9cd13f
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge 5e2ef89
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge 20856e3
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge a38763f
revert commented code that was there for a reason
SylvainJuge b77428c
add LambdaTransformer interface + rename things
SylvainJuge bc785b9
fix typos and comments
SylvainJuge File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
20 changes: 0 additions & 20 deletions
20
instrumentation/internal/internal-lambda-java9/javaagent/build.gradle.kts
This file was deleted.
Oops, something went wrong.
30 changes: 0 additions & 30 deletions
30
...va/io/opentelemetry/javaagent/instrumentation/internal/lambda/Java9LambdaTransformer.java
This file was deleted.
Oops, something went wrong.
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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 didn't immediately get why you wanted to change this, but I guess it is to reduce the helper classes and push more logic into the agent code. I think this is good idea and getting rid for the extra module for java9 is nice. What I'd change is that I'd probably also rename
ClassFileTransformerHolder
as it is only for lambdas as you noted. Also I'd consider not usingClassFileTransformer
here. Passing thetargetClass
argument to the transform method confused me a bit astransform
should have the class there only when retransforming/redefining which we aren't doing here. You could have an interface inClassFileTransformerHolder
that takes the necessary arguments. What do you think?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 agree on the
ClassFileTransformerHolder
rename as it's definitely only used for lambdas.I initially tried to simplify the design/API but that requires to introduce a new interface, so I thought maybe keep reusing
ClassFileTransformer
could be an acceptable compromise, but I agree that it might be a bit confusing as the implementations details leak into the callers as shown by the quantity of comments to use it the way it's intended.I'll do another attempt by introducing a new interface to see if it helps improve this.