search

rinq / rinq-go

A cross-language command bus and distributed ephemeral data store.
Other
17 stars 1 forks source link

Include Rinq's traceID in opentracing span tags. #160

Closed koden-km closed 6 years ago

koden-km commented 6 years ago

Fixes #128

koden-km commented 6 years ago

I'm not sure how to get a traceID in local session SetAsyncHandler? src/internal/localsession.session.go func (s *Session) SetAsyncHandler(h rinq.AsyncHandler) error {

koden-km commented 6 years ago

I'm not sure about adding the trace to the span for notifier here: src/rinqamqp/internal/notifyamqp/notifier.go func (n *notifier) NotifyUnicast( and func (n *notifier) NotifyMulticast(.

I don't have the span it gets packed?

jmalloc commented 6 years ago

I'm not sure how to get a traceID in local session SetAsyncHandler?

The closure inside is passed a ctx, which is guaranteed to contain the trace ID by that point, so you can fetch it with trace.Get(ctx).

jmalloc commented 6 years ago

I'm not sure about adding the trace to the span for notifier here: src/rinqamqp/internal/notifyamqp/notifier.go func (n notifier) NotifyUnicast( and func (n notifier) NotifyMulticast(.

I don't think you'll need to because the methods of localsession.Session that use these will have already done it.

koden-km commented 6 years ago

OK I have added the trace id to the SetAsyncHandler using trace.Get(ctx). I checked the NotifyXxx methods and you are correct, they already have the trace set earlier in the Session. I think that is all of those covered now.

I wasn't sure whether to add the extra blank line of spacing for those calls to opentr.AddTraceID(). I just tried to match the existing style and there seemed to be a blank line before the error check lines. I can remove that extra blank line if you prefer them packed tighter?

jmalloc commented 6 years ago

I can remove that extra blank line if you prefer them packed tighter?

I am trying to care less about this kind of trivial stuff. If you run make prepare and it doesn't change it, then it's good!

koden-km commented 6 years ago

OK I will fix those up now.

jmalloc commented 6 years ago

Before I merge this, I'd like for you to spin up Jaeger under Docker and try out each of these scenarios.

http://jaeger.readthedocs.io/en/latest/getting_started/#all-in-one-docker-image

There's no Rinq environment variable for configuring OpenTracing, so you'll need to configure the client yourself.

This is the client we use for Jaeger: https://github.com/jaegertracing/jaeger-client-go

That Jaeger client is just one possible implementation of the opentracing interfaces that Rinq uses.