-
Notifications
You must be signed in to change notification settings - Fork 398
Fix losing original karafka trace after iterating through the messages with distributed_tracing on #4876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix losing original karafka trace after iterating through the messages with distributed_tracing on #4876
Conversation
|
Err thanks for the patience, I've asked the team to take a look at this asap :) |
|
@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. |
|
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? |
|
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? |
54c704d to
4f1847d
Compare
|
Coming back to this now - apologies for the delay 🙇
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? 🙇 |
48c1d1f to
1d49933
Compare
p-datadog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
f8a9e45 to
c32d6a6
Compare
|
Hey Datadog team 👋 @vpellan @p-datadog @marcotc @ivoanjo 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). |
|
Hey @Drowze ! Thanks for pinging me, I'll take a look at it asap ! |
|
Hi @Drowze , I have this on my list as well. I am currently working on some time-sensitive fixes but after that is done (hopefully by end of week) I plan on getting this PR merged. We don't currently have automation around community PRs - this is essentially the blocker. |
|
Running the CI here: #5192 |
What does this PR do?
On the Karafka integration, when
distributed_tracingis on, the original trace was lost after iterating through the messages. This PR aims to fix thatand 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.