search

SmartThingsOSS / smartthings-brave

Experimental extensions for the Open Zipkin Brave library
9 stars 8 forks source link

Added support for tracing via SNS message attributes #22

Closed janderson007 closed 6 years ago

janderson007 commented 6 years ago

Adds a request handler that can be added to a AWS SNS client to add tracing to the message attributes when publishing a message to a topic.

cc @llinder

devinsba commented 6 years ago

FYI, we are tracking a similar feature for inclusion in zipkin-aws. I left the following comment with what I learned from looking over the X-Ray library: https://github.com/openzipkin/zipkin-aws/issues/53#issuecomment-346450852

Also, I know personally I'd be able to give time to get this merged into that repo if you wanted to try to make a similar PR there

janderson007 commented 6 years ago

@devinsba - As far as I can tell, XRay is just generically tracing the client request and the service response; for SNS/SQS, it doesn't trace the actual message. The consumer that receives the message will not be able to continue the trace, and ultimately, that is what we wanted.

For this SNS tracing to work, it's relying on

  1. a consumer that will pull the trace id from the MessageAttributes in order to continue the trace. (We are subscribing via SQS, so this is handled via the instrumentation of SQS that @llinder put together)
  2. RawMessageDelivery needs to be enabled for the SNS subscription, so that the MessageAttributes get propagated to the subscriber. (The application is responsible for this)

So our use case is a little bit specific, and this PR by itself might not be all that useful without the SQS tracing as well. I guess it depends if you want to be able to just trace through the API calls like XRay does, or if you want a deeper level of tracing that gets propagated through the actual messages.

devinsba commented 6 years ago

Ah, I missed the detail that this was metadata attachment. Thanks for the clarification! I still think these are valuable instrumentations if you all ever decide to upstream them into the "official" repos

llinder commented 6 years ago

Thanks for the feedback @devinsba! Having general Zipkin instrumentation for the AWS clients would be awesome and it looks like you have some good thoughts on the implementation already. As @janderson007 described, our use case is more about context propagation and not just API tracing but I think both have their place.

I'm fairly overwhelmed with work through mid January but will give this some thought and we can collaborate more via your document you started or on openzipkin Gitter channel.

janderson007 commented 6 years ago

I added a README.md and implemented the suggestions.

I also moved the injection of the trace attributes from beforeRequest and into beforeMarshalling. The request is actually marshalled before the beforeRequest, (I'd assumed the opposite) so any changes to the original request in beforeRequest do not get marshalled.

(The XRay library uses beforeRequest, but is directly setting HTTP Request headers rather than modifying the original request with message attributes)