aws / aws-xray-sdk-go

AWS X-Ray SDK for the Go programming language.
Apache License 2.0
278 stars 118 forks source link

Existing sampling decision ignored when creating new segments with no `http.Request` (SQS -> ECS tracing) #350

Open okonos opened 2 years ago

okonos commented 2 years ago

To implement tracing on an ECS worker processing SQS messages, we use the NewSegmentFromHeader function:

https://github.com/aws/aws-xray-sdk-go/blob/3b7709c6db4285afc07af03cc0a657c800a8c93f/xray/segment.go#L315

Since there's no http.Request available in a SQS processing context, a nil is passed instead. This causes the existing sampling decision to be ignored and a new one requested instead:

https://github.com/aws/aws-xray-sdk-go/blob/3b7709c6db4285afc07af03cc0a657c800a8c93f/xray/segment.go#L101-L103

This results in some segments missing and possibly in depleting the sampling reservoir, which could be used by other traces.

Looking through the docs and code there doesn't seem to be any alternative way of creating a segment from an existing tracing header. A workaround we've come up with is to pass an empty request (&http.Request{URL: &url.URL{}}) to the NewSegmentFromHeader function. This could preferably be solved on the SDK level, though. A relatively simple solution would seem to be to remove the r == nil condition from:

https://github.com/aws/aws-xray-sdk-go/blob/3b7709c6db4285afc07af03cc0a657c800a8c93f/xray/segment.go#L101

and instead fill the sampling.Request with HTTP request related data in both if branches only when it's available. After all, the HTTP request is only used when requesting the sampling decision.

Any insight into the issue and proposed fix would be much appreciated.

Possibly related issue: #234

bhautikpip commented 2 years ago

Yes, I agree with the temporary fix you suggested and suggestion you provided for permanent fix as well. I will mark this as an enhancement. Yeah, we can probably remove the r == nil check to include use cases like yours.