Fix losing original karafka trace after iterating through the messages with distributed_tracing on

[Drowze]

What does this PR do?

On the Karafka integration, when distributed_tracing is on, the original trace was lost after iterating through the messages. This PR aims to fix that and also add span links linking the message traces with the parent consumer trace (so it's easier to find each other using the Datadog APM UI).

Fixes #4873

Motivation:
Fix and improve the Datadog Karafka support.

Change log entry

Additional Notes:

How to test the change?
See #4873 for a full testing snippet.

[Drowze]

Hey @marcotc @ivoanjo can you have a quick look here please? 🙇

Would like to get this merged to not conflict with other Karafka/Waterdrop pull requests (e.g.: #4874 - and also I want to open a new PR soon about turning off distributed tracing on a per-topic basis, see here)

[ivoanjo]

Err thanks for the patience, I've asked the team to take a look at this asap :)

[marcotc]

@Drowze I took a first pass and I need to allocate proper time to understand how we best want to present this kind of workflow (span links, directly linking, something else).

I have scheduled time to take a good look at this next week.

[p-datadog]

The repair to #4876 LGTM. Span links do not appear to be used in dd-trace-rb except in one place where an open telemetry trace is converted (mapped?) to a datadog trace. @Drowze how do you feel about moving the span link code into a separate PR and just leaving the fix in this PR?

[p-datadog]

Regarding span links: I read https://docs.datadoghq.com/tracing/trace_collection/span_links/ to try to understand their usage. The documentation page says the span links are for relationships other than parent/child. In this PR they are used for parent/child relationship? What is the actual gain/benefit?

[p-datadog]

Looking at this PR again I don't understand the fix. The problem is the parent trace gets reset somehow, therefore I would expect the fix to repair the reset. How is adding links fixing the reset?

[Drowze]

Coming back to this now - apologies for the delay 🙇

do you feel about moving the span link code into a separate PR and just leaving the fix in this PR?

Sure - sounds good to me. I understand that span links are not widely used at the moment and using them here just made the PR a bit confusing to review, so I'll move that part into a separate pull request - and we can continue discussing span links there.

@p-datadog I just rebased the PR with latest master and removed the code referring to span links - can you have another look please? 🙇

[p-datadog]

Thanks @Drowze for splitting the PR.

I think this PR look good but I am not an expert in the tracing side, I'll try to find another reviewer.

[p-datadog]

In the other test, is consumer (the middle span, if I am understanding the logic correctly) expected to be a child of producer? Is that asserted somewhere?

[Drowze]

In these tests, the consumer span is never expected to be a child of any span. Referring to each test individually:

1. when the message has tracing headers, when distributed tracing is enabled, it continues the span that produced the message

   • consumer is a root span and it doesn't have any child spans
   • producer is a root span and it has one child span from iterating the messages(*)

2. when the message has tracing headers, when distributed tracing is not enabled, it does not continue the span that produced the message

   • consumer is a root span and it has one child span from iterating the messages(*)
   • producer is a root span and it doesn't have any child spans

Hope that makes sense! 🙇

---

(*) we create spans from iterating the messages as the karafka integration patches ::Karafka::Messages::Messages#each

[Drowze]

(*) we create spans from iterating the messages as the karafka integration patches ::Karafka::Messages::Messages#each

I think that this might not be clear enough from our tests... The messages are being iterated here:

# this creates a new span
expect(messages).to all(be_a(::Karafka::Messages::Message))

# ...

# in these expectations, `span` refers to the span created from iterating the messages
expect(span.parent_id).to eq(consumer_span.id)
expect(span.trace_id).to eq(consumer_trace.id)

# because of these `let`s (from the beginning of the file)
let(:span) do
  spans.find { |s| s.name == span_name }
end
let(:span_name) { Datadog::Tracing::Contrib::Karafka::Ext::SPAN_MESSAGE_CONSUME }

I'll update the test now to use an explicit each and add a comment about it.

[Drowze]

Just updated the test with comments better expliciting the test intentions - please let me know if it's more clear now 🙇

---

note: I didn't add an explicit each because this same pattern of relying on expect(...).to all(...) to create a span is used across pretty much all the tests in that file

[p-datadog]

Thanks, I think I understand what you are saying, but it's strange to me that the iterating span changed from being under the producer to being under the consumer but there is no relationship between the producer and the consumer. Might be a question outside the scope of this PR.

[Drowze]

Iterating the message does not really change any spans - it's continuing a trace that changes the parent/child relationship of spans.
In other words:

• when distributed_tracing=true, the message span is a continuation of the span that created it (if any) (i.e.: the message span turns into a child of the span who produced the message)
• when distributed_tracing=false, the message span is just a regular span (which may have a parent - which in the case of theses tests is the consumer span who's iterating the messages)

Distributed tracing gets confusing quickly 😅

[Drowze]

just fyi: I found this issue describing a similar use-case, so I've included our own use-case there as well (along with an example & screenshots)

[Drowze]

Hey Datadog team 👋 @vpellan @p-datadog @marcotc @ivoanjo
Is there anything I can do to help moving this PR forward?

As of now we're having to use our own dd-trace-rb fork due to some issues/missing features around the Karafka/WaterDrop integration (our fork is the latest dd-trace-rb release (v2.22.0), plus this PR, #5120 and #5107).