Refactor: From extractAndClear .. inject approach to extract .. clearAndInject

[jeqo]

Draft to explore refactoring the approached used on messaging instrumentation (1) where context is first extracted and cleared from headers to later inject if needed, to an approach (2) where context is extracted to later clear context form headers if needed to then inject.

Messaging instrumentation has mainly 2 features on consumer side: onConsume that under (1) it extracts and clear headers, start/finish on-consume span, and injects context. And nextSpan that under (2) it extracts and clear headers, and creates a span from extracted context.

Current effects of approach (1):

• By clearing context after extracting we make sure context is not "leaked" and propagated downstream.
  • On on-consume feature we eventually inject context again, then this is not as critical.
  • On nextSpan feature this restrict users to have only one child from onConsume span as we remove headers. This (potentially) could restrict users on how to continue a trace. There could be scenarios where users do a batch of operations after a message is consumed, requiring multiple spans to be created from onConsume span.

Following approach (2) we can allow users to continue onConsume span multiple times.

A side-effect of approach (2) is to reduce impact on current JMS consumer implementation:

• JMS consumed message properties (i.e. headers) need to be "resetted" before injecting context. This currently requires to cache non-context properties, reset all properties, and write non-context properties again. All these to inject context later. Under approach (1) we do the reset after extracting context, to at the end of instrumentation inject new context, impacting both features onConsume and nextSpan
• With approach (2) we can move reset logic right before injecting. By doing this only onConsume we remove this overhead from nextSpan feature.

[codefromthecrypt]

at the layer of abstraction of JMS, I wouldn't encourage changing things based on features not defined there. For example, a potential of a batch operation to change the common-course "nextSpan" logic where the behavior already in place might be relied on.

Instead, I'd suggest aiming more precisely, looking at the layer of abstraction where the use case discussed is defined, then decide how to address that, whether that's via a hook or an alternate approach.

[codefromthecrypt]

PS one nice way to pickup whatever happens here is including a diff from here https://github.com/openzipkin/brave/blob/master/instrumentation/messaging/RATIONALE.md

[codefromthecrypt]

#1229 will drift this a bit

[jeqo]

@adriancole as JMS issues are not there anymore, I'd argue this change is not required anymore:

• breaking extract and clear into 2 steps was more of an enabler to solve ActiveMQ issue.
• only extracting context and not cleaning in nextSpan was a nice side-effect as it removes one intermediate span when trying to reuse (e.g. for batching); but the same can be achieved by creating a span with nextSpan and use that one as root for batching.
• as you mentioned, users might be relying on current nextSpan behavior.

I'm ok closing this if there's no additional comments :)

[codefromthecrypt]

thanks for the feedback and follow up!

…

On Tue, Jul 7, 2020, 5:20 PM Jorge Esteban Quilcate Otoya < ***@***.***> wrote: @adriancole <https://github.com/adriancole> as JMS issues are not there anymore, I'd argue this change is not required anymore: - breaking extract and clear into 2 steps was more of an enabler to solve ActiveMQ issue. - only extracting context and not cleaning in nextSpan was a *nice* side-effect as it removes one intermediate span when trying to reuse (e.g. for batching); but the same can be achieved by creating a span with nextSpan and use that one as root for batching. - as you mentioned, users might be relying on current nextSpan behavior. I'm ok closing this if there's no additional comments :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1047 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPVV7XHNHIUYARATN4OPTR2LSEXANCNFSM4JU3SCTQ> .