fix instrumentation module not loading silently when duplicate helper… #13005
Conversation
… classnames are detected. issue open-telemetry#9752 introduced a check that prevents an instrumentation module from being loaded when duplicate helper classes are detected. Such a case may arise when a class is detected by muzzle as a helper class and at the same time it is registered in additionalHelperClasses. Before open-telemetry#9752, the module would properly load. The proposed change will allow the instrumentation to load and log a warning. While it may be true that this is actually an issue in the instrumentation module, such cases are not obvious to find, there are modules in the field that have stopped working because this change and the fix should not lead to unintended behavior as we can expect the bytecode of the duplicates to be identical.
|
@SylvainJuge @JonasKunz any thoughts on this? thanks |
There was a problem hiding this comment.
LGTM, I don't think that in #9752 I made this check more strict intentionally.
Even if the helper classes happen to have a different bytecode, this very likely only causes the instrumentation to fail and the application itself should run fine, so a warning should be good enough.
With invokedynamic, injecting will become an edge case anyway, so we can expect this warning to pop up rarely.
| @@ -183,8 +184,11 @@ public DynamicType.Builder<?> transform( | |||
| HelperClassDefinition::getClassName, | |||
| helper -> () -> helper.getBytecode().getBytecode(), | |||
| (a, b) -> { | |||
| throw new IllegalStateException( | |||
| "Duplicate classnames for helper class detected!"); | |||
| logger.log( | |||
There was a problem hiding this comment.
Firstly I think there should also be a test for this. I have created a PR for your PR 3kraft#1 that you can use to add the test. Secondly I'm not sure that printing a warning here is useful. There isn't anything in the javadoc of
throw new IllegalStateException here as it was and ensure the uniqueness at an earlier point. Perhaps add .distinct() to
There was a problem hiding this comment.
Hi @laurit
thanks for reviewing and providing the test cases. I agree that the warning message would have been better if it included the offending class name, nevertheless at that point only the bytecode is available and thus I decided to warn with the offending module only.
When the class names are distincted, it is impossible for this case to happen and the merge function could be removed. It will throw an IllegalStateException anyways (https://docs.oracle.com/javase/8/docs/api/java/util/stream/Collectors.html#toMap-java.util.function.Function-java.util.function.Function-).
There was a problem hiding this comment.
When the class names are distincted, it is impossible for this case to happen and the merge function could be removed. It will throw an IllegalStateException anyways (https://docs.oracle.com/javase/8/docs/api/java/util/stream/Collectors.html#toMap-java.util.function.Function-java.util.function.Function-).
HelperInjector has another constructor that could be provided with input that contains duplicates, though that would most likely be a bug, so I think it is fine to leave the merge function as it is.
Add test for duplicate helper class
… classnames are detected.
issue #9752 introduced a check that prevents an instrumentation module from being loaded when duplicate helper classes are detected. Such a case may arise when a class is detected by muzzle as a helper class and at the same time it is registered in additionalHelperClasses. Before #9752, the module would properly load.
The proposed change will allow the instrumentation to load and log a warning. While it may be true that this is actually an issue in the instrumentation module, such cases are not obvious to find, there are modules in the field that have stopped working because this change and the fix should not lead to unintended behavior as we can expect the bytecode of the duplicates to be identical.