- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1k
 
make reactor indy-ready #14640
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
make reactor indy-ready #14640
Conversation
This reverts commit 3864cf9.
…nstrumentation into indy-reactor-2
…nstrumentation into indy-reactor-2
        
          
                ...emetry/javaagent/instrumentation/reactor/kafka/v1_0/DefaultKafkaReceiverInstrumentation.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @Advice.Argument(1) | ||
| BiConsumer<? super HttpClientResponse, ? super Throwable> originalResponseCallback) { | ||
| 
               | 
          ||
| // intermediate variables needed for inlined instrumentation | 
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.
was there some side effect that surfaced that caused issues with referencing the originals without making an intermediate?
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.
Yes, when removing those this makes the tests fail with missing spans. I remember having seen this elsewhere and thus applied this workaround after spending too much time doing diffs and partial reverts. It could be something worth investigating further as it might be related to bytebuddy or the way we use it.
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.
as far as I remember byte-buddy does not allow you to reassign a value to a read only argument
…nstrumentation into indy-reactor-2
| * Dedicated advice scope subclass that make instrumentation skip original method body using | ||
| * {@code skipOn = Runnable.class } which does not require to expose an extra type | 
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.
bit of a hack, but overall should be nicer than the alternatives
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, also it's quite rare (and probably the only instance) that we have to dynamically skip original method body while also overriding/wrapping arguments.
…nstrumentation into indy-reactor-2
Part of #13031 for reactor