send traceparent header to ES #1002
Conversation
There was a problem hiding this comment.
Looks good!
A couple of things:
- should we be setting the
tracestateheader too? Please check with the other agent devs if unsure - we also need to propagate the parent transaction's traceparent when there's a non-sampled transaction in the context, or if the span is dropped (max spans per transaction reached). That means we'll need to change the
!tx.Sampled()andspan.Dropped()cases above. Take a look at apmhttp's client wrapper for reference:apm-agent-go/module/apmhttp/client.go
Line 78 in 2045b7b
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
Propagate the parent transaction's traceparent and tracestate headers when there's a non-sampled transaction in the context, or if the span is dropped
| assert.Contains(t, headers, apmhttp.TracestateHeader) | ||
| } | ||
|
|
||
| func TestClientSpanDropped(t *testing.T) { |
There was a problem hiding this comment.
@axw this and TestClientTransactionUnsampled are copied from apmhttp/client_test.go. They do verify that we're setting the headers when a span is dropped and when a transaction is not sampled, but what do you think about paring them back to just verifying we have the headers? I'm happy either way since this is essentially doubling up on checking the calculation of the headers, vs. just making sure we're setting them.
There was a problem hiding this comment.
I think what you have here is good. I do think it's important to check the trace context of the transaction vs. span is used like you have in this test.
|
/test |
|
jenkins run the tests |
| if tx == nil || !tx.Sampled() { | ||
| apmhttp.SetHeaders(req, traceContext, false) |
There was a problem hiding this comment.
Should we default to propagating the legacy header?
There was a problem hiding this comment.
I think we can skip sending the legacy header. It's only for compatibility with old Elastic APM agents, so not relevant for the Elasticsearch use case where the plan is to extract the traceparent header and use the trace ID in logs.
| if tx == nil || !tx.Sampled() { | ||
| apmhttp.SetHeaders(req, traceContext, false) |
There was a problem hiding this comment.
I think we can skip sending the legacy header. It's only for compatibility with old Elastic APM agents, so not relevant for the Elasticsearch use case where the plan is to extract the traceparent header and use the trace ID in logs.
| assert.Contains(t, headers, apmhttp.TracestateHeader) | ||
| } | ||
|
|
||
| func TestClientSpanDropped(t *testing.T) { |
There was a problem hiding this comment.
I think what you have here is good. I do think it's important to check the trace context of the transaction vs. span is used like you have in this test.
Send the traceparent header to elasticsearch, now that ES parses it from incoming requests.
Closes #987