Added an option to wrap current spring rabbit components #636
Conversation
without this change you can only explicitly create a RabbitTemplate or a SimpleRabbitListenerContainerFactory. In case of frameworks like Sleuth that want to wrap existing instances that's not possible. with this change in a hacky (RabbitTemplate) and non-hacky (SRLCF) way we're allowing to instrument existing objects. For RT we're hacking into a field of before post processors and injecting there the traced instrumentation. For SRLCF we're adding the traced advice to an existing factory
marcingrzejszczak
changed the title
| @@ -60,6 +68,36 @@ public RabbitTemplate newRabbitTemplate(ConnectionFactory connectionFactory) { | |||
| return rabbitTemplate; | |||
| } | |||
|
|
|||
| /** Instruments an existing rabbit template. */ | |||
| public RabbitTemplate fromRabbitTemplate(RabbitTemplate rabbitTemplate) { | |||
There was a problem hiding this comment.
nit: maybe decorateRabbitTemplate?
| Collection<MessagePostProcessor> processors = | ||
| (Collection<MessagePostProcessor>) field.get(rabbitTemplate); | ||
| List<MessagePostProcessor> newProcessors = new ArrayList<>(); | ||
| if (!processors.contains(tracingMessagePostProcessor)) { |
There was a problem hiding this comment.
not sure this will work as we don't do any deep equals etc. Maybe we should just look for an instance of?
| if (!currentChain.contains(tracingRabbitListenerAdvice)) { | ||
| chainList.add(tracingRabbitListenerAdvice); | ||
| } | ||
| if (chain != null) { |
There was a problem hiding this comment.
if this were null would line 126 NPE?
| if (chain != null) { | ||
| chainList.addAll(Arrays.asList(chain)); | ||
| } | ||
| return chainList.toArray(new Advice[] {}); |
There was a problem hiding this comment.
neat trick: new Advice[0]
|
|
||
| private Advice[] chainListWithTracing(Advice[] chain) { | ||
| if (chain == null) { | ||
| return chain; |
There was a problem hiding this comment.
do you want this? or do you want to return a singleton array with the tracingRabbitListenerAdvice?
|
will buff this on the way in. Thanks @marcingrzejszczak! |
| @@ -52,8 +54,10 @@ public SimpleRabbitListenerContainerFactory rabbitListenerContainerFactory( | |||
| ConnectionFactory connectionFactory, | |||
| SpringRabbitTracing springRabbitTracing | |||
| ) { | |||
| return springRabbitTracing.newSimpleMessageListenerContainerFactory(connectionFactory); | |||
| return springRabbitTracing.newSimpleRabbitListenerContainerFactory(connectionFactory); | |||
There was a problem hiding this comment.
@jonathan-lo heads up. fixing this typo in a patch on green
| public RabbitTemplate decorateRabbitTemplate(RabbitTemplate rabbitTemplate) { | ||
| // Skip out if we can't read the field for the existing post processors | ||
| if (beforePublishPostProcessorsField == null) return rabbitTemplate; |
There was a problem hiding this comment.
why not just set the value of the field with the processor?
There was a problem hiding this comment.
if this is null, we had a reflection error which I'm not assuming will get better :)
|
circle ci is flaking unrelatedly |
|
4.17.2 on the way |
without this change you can only explicitly create a RabbitTemplate or a SimpleRabbitListenerContainerFactory. In case of frameworks like Sleuth that want to wrap existing instances that's not possible.
with this change in a hacky (RabbitTemplate) and non-hacky (SRLCF) way we're allowing to instrument existing objects. For RT we're hacking into a field of before post processors and injecting there the traced instrumentation. For SRLCF we're adding the traced advice to an existing factory