Add automatic distributed trace to rack if ids are present #141
Conversation
|
If this works, it addresses my request #89 🎉 |
|
@cabello Tested and working we have AMQP between our services so in the call to that I put in the parent_span and parent_trace ids (in the fake html) and it comes through to the service and it all links up perfectly. |
|
@ufoot Do you need me to do anything else to this PR to get it merged? We are using changes in production and would love to get this merged so I do not have to manage my fork. |
| } | ||
|
|
||
| # Merge distributed trace ids if present | ||
| unless env['HTTP_X_DDTRACE_PARENT_TRACE_ID'].nil? || env['HTTP_X_DDTRACE_PARENT_SPAN_ID'].nil? |
There was a problem hiding this comment.
Have these headers been documented as the "canonical" headers to use for distributed traces? I've seen various values used throughout clients for different languages.
Would be nice if there was a strict contract for this between all languages.
There was a problem hiding this comment.
They are documented in http://www.rubydoc.info/github/DataDog/dd-trace-rb/ as the example for distributed tracing so I would say yes? I also want to add setting these in the net/http middleware which would mean you wouldn't even have to manually set them if it was included.
There was a problem hiding this comment.
As part of this patch I could add to the documentation that the rack middleware will automatically do distributed tracing based on these headers also?
There was a problem hiding this comment.
Hi all, indeed the plan is to have these headers be the same for all languages, obviously. Sorry for the delay, we're still discussing a few points here, as we know once this is officially released, there's no way back, any update would be a breaking change for you. Still, I understand the frustration of maintaining your fork @cabello ... BTW @whithajess this should probably be merged/rebased with some upcoming PR which changes the way we deal with the current span. I'm leaving a small comment in the code about this. Thanks for your patience, we'll get this one moving on, for sure!
There was a problem hiding this comment.
@ejholmes indeed there are still different values around but as I said just above, plan is really to have all of them aligned. There has been recent moves on Python recently concerning this: https://github.com/DataDog/dd-trace-py/blob/c609a9054c479b6e394eb807520567b000f1aace/ddtrace/contrib/aiohttp/middlewares.py#L12 and in Go too: https://github.com/DataDog/dd-trace-go/blob/f3e02e97f8a21336bac49d9071d27edd98885da3/tracer/contrib/tracegrpc/grpc.go#L17 and Ruby, with the help of this patch, should follow.
|
Looking forward to having this merged, I am tired of keeping my fork. 👍 |
| parent = @buffer.get() | ||
| span.set_parent(parent) | ||
| if span.parent_id.zero? || span.parent_id.nil? | ||
| parent = @buffer.get() |
There was a problem hiding this comment.
Side note, this would probably need to be merged with #145 but I could take care of that (the change concerning this buffer is not small, it... disappeared).
|
FYI, has been merged with #151 so master now has support for this. We only have packaging & QA tasks now before releasing this feature in the official gem. Thanks for your contributions @whithajess and @cabello , much appreciated. |
If trace and span ids are present in the request use them in the trace.