Feature/fixing zipkin spans#5370
Conversation
…el processing of multicast or recipientlist. The original ZipkinState object ends up getting shared by different exchange object copies resulting in issues during push/pop operations. Added interface to allow safe copy of properties which can provide deep object copies for safe copy operations
| @SuppressWarnings("unchecked") | ||
| private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) { | ||
| target.putAll(source); | ||
| source.entrySet().stream().forEach(entry -> { |
There was a problem hiding this comment.
Sorry we do not want this kind in the core. Can you maybe try to come up with a different solution.
There was a problem hiding this comment.
@davsclaus Are you suggesting to totally avoid changes in core, or avoid these specific changes in core? I initially did come up with another approach for camel-zipkin but it is far more complicated and honestly does not address the real problem.
The span problem I am pretty sure exists in camel opentracing and opentelemetry as well because both of them use camel-tracing and if you look at ActiveSpanManager it also uses Exchange property mechanism to set ACTIVE_SPAN_PROPERTY which would get corrupted when using parallelProcessing with multicast.
My suggested approach would easily fix issues in opentracing and opentelemetry as well.
There was a problem hiding this comment.
Yes we do not want core comprimised with this due to zipkin storing state like that. The core should be lean and fast.
There was a problem hiding this comment.
Instead of doing this expensive check always, then find out some way for zipkin et all to turn on a flag on the exchange (via ExtendedExchange) to tell it that it should do deep clone safe copy mode, then this is only in use when you use these special zipkin components.
Or instead of storing the zipkin state as exchange property, store it on some zipkin bean, that handles this and stores the data per exchange id or something.
There was a problem hiding this comment.
@davsclaus Thanks for the advice. I think setting the flag via ExtendedExchange would be the best option.
There was a problem hiding this comment.
I have incorporated your feedback and also an alternate approach and IMHO cleaner one has been implemented in another PR #5373
There was a problem hiding this comment.
Oh that is great work that you implemented two different solutions. I agree with you that the alternative is the better choice, so lets close this PR
3 participants
Fixing CAMEL-16509
When copying Exchange properties, provide a mechanism to set the copy object as property value instead of the original value object. This will allow the properties to be safely shared between Exchange objects operating in parallelProcessing and in this case ZipkinState will be easily shared without it getting corrupted.