add flag to not share trace on consumption

[jeqo]

fix #1032

Context:

Kafka Consumers obtain records from a Kafka topics by polling from an offset. By design, poll operation return an array of records, to be processed usually in a loop.

This poll operation is traced and create an initial span on the consumer application. Right before creating this span, incoming record is checked to see if it has a trace context or not. If it does, poll span will be created to continue a record's trace. If it doesn't, it will create a poll span to be shared with other records that doesn't have a trace context in its headers.

This PR is aimed to allow users to opt-out this mechanism of creating a shared span for records without a trace context. Instead, a poll span will be created by record.

I'd like to propose the following defaults:

• Kafka Consumer: shareTraceOnConsumption=true to don't break current behavior on consumers; as plain Kafka Consumers are usually used to sink data somewhere.
• Kafka Streams: shareTraceOnConsumption=false as Kafka Streams usually propagate context to other topics, leading to even larger traces that if batched, parent trace will be too big.

[jeqo]

cc @jorgheymans

[anuraaga]

I think it'd be nice to indicate that this applies to new traces in the name itself. shareNewTraceOnConsumption?

Though for me it's easier to grok with the opposite - splitNewTraceOnConsumption. What do you think?

[anuraaga]

Also I know the class is called consumer but I think receive might be easier to understand.

[jeqo]

Thanks! I'm not convinced about naming either hehe. I'll rephrase my explanation to have a better background for naming.

[jorgheymans]

tend to agree, since shareTraceOnConsumption is the current behaviour it might stand out more if the flag refers to enabling the opposite behaviour . splitTraceOnConsumption , spanPerMessage, noSpanPerBatch,

Above names are still slightly misleading as they could make ppl (like me) think that it allows you to break a trace, whereas the flag only applies when no parent trace context is around. spanPerMessageWhenNoParentTraceFound ?

[jeqo]

would shareRootSpanOnReceive or rootSpanOnReceiveBatch be a better name? (I think span...WhenNoParentTrace ~= rootSpan)

[jeqo]

btw @jorgheymans I think we should have a similar flag to do not continue a trace on the consumer side (instead that removing headers on the producer), that would be related with this one. for instance: if producer is instrumented and injecting trace context, on the consumer side we can decide to do not continue the same trace context, and just create a new root span. Then, this flag can help to opt-out of sharing that root span between consumed messages in a batch.

Will tackle this on an additional PR as it will impact other messaging instrumentation.

[jorgheymans]

@jeqo so maybe this should be bundled together as one feature then, where the separate-span-if-no-existing-root and force-start-new-trace are just different strategies of this feature ? This 'something' would allow one to relatively easily define new processing like 'if the message header contains value X we want to start a new trace, otherwise join existing'. Or is that a bridge too far ?

[jeqo]

I'd say that (1) starting a new trace on consume is independent from (2) reusing/sharing rootSpan when consuming a batch. Where (1) is shared across messaging instrumentations, probably should be added at that level of abstraction; and (2) is specific to Kafka so far as we don't have other consumer instrumentation polling batches, but could eventually be moved to messaging as well. So, I'd take these as 2 diff features.

About if (header=x) then start new trace, I haven't thought about this as initial use-case. My first answer will be to do this once you have consumed a message, as part of your code. But we can discuss this further here #1039

[jeqo]

@anuraaga, I've rephrased API comments, and added context on the PR. Let me know if its clearer now to focus on flag name.

[anuraaga]

Thanks!

[jeqo]

@anuraaga @jorgheymans , I've applied latest changes based on your feedback. Let me know if it's ready to merge :)

[jorgheymans]

minor nit otherwise good to go 👍

[jeqo]

thanks for your feedback! I'll go ahead and merge this one 👍