salemove / freddy

Messaging api supporting request-response and acknowledgements.
MIT License
9 stars 0 forks source link

Add tracing support #54

Closed indrekj closed 7 years ago

indrekj commented 7 years ago

See README changes for more information.

INF-982

ostap0207 commented 7 years ago

Is it possible that 2 messages come one after one in the same thread?

indrekj commented 7 years ago

@ostap0207 No, the current one must be processed before the new one is taken

deiwin commented 7 years ago

Can you explain why you chose to use this thread-local getter instead of e.g. providing the trace data as an argument to respond_to?

take-five commented 7 years ago

Can you explain why you chose to use this thread-local getter instead of e.g. providing the trace data as an argument to respond_to?

I think Indrek's approach is easier to integrate. With current implementation you don't have to change application code at all, only upgrade Freddy.

indrekj commented 7 years ago

Can you explain why you chose to use this thread-local getter instead of e.g. providing the trace data as an argument to respond_to?

Passing it to respond_to means that you have to carry this information around to also inject it into deliver_with_response and deliver calls. This means major changes in all of our services. I've seen this practice used also in zipkin tracer.

I usually avoid thread local variables and singletons like plague but I think it can make sense in this case and for example for loggers.

EDIT: yup, as @take-five said

deiwin commented 7 years ago

Well, it's simpler to integrate the same way how exposing variables as globals makes 2 separate parts of a system easier to integrate. It doesn't mean that it's immediately a good approach.

What about a system which has a global tracing management by default and then when you need to customize the tracing or w/e, you do something like freddy.with_trace {...}.respond_to?

indrekj commented 7 years ago

you can do (in readme also): Freddy.current_trace = Freddy::Traces::Trace.new(id: .., parent_id: ..., span_id: ...)

e.g. if you have rack middleware that handles tracing then on request you set the Freddy.current_trace.

deiwin commented 7 years ago

you can do (in readme also): Freddy.current_trace = Freddy::Traces::Trace.new(id: .., parent_id: ..., span_id: ...)

e.g. if you have rack middleware that handles tracing then on request you set the Freddy.current_trace.

Yes, I understand. But what I was suggesting was that you could support the exact same functionality without using singletons (essentially), while also keeping the feature of getting the traces for free with a freddy update.

willmore commented 7 years ago

@indrekj @deiwin can we merge and address singleton issue later (if needed)?

indrekj commented 7 years ago

What about a system which has a global tracing management by default and then when you need to customize the tracing or w/e, you do something like freddy.with_trace {...}.respond_to? ... Yes, I understand. But what I was suggesting was that you could support the exact same functionality without using singletons (essentially), while also keeping the feature of getting the traces for free with a freddy update.

freddy.with_trace {...}.respond_to will not work for most cases. Usually you will not create a responder for every request. It is more likely you create only one in the application scope and use it throughout the application life.

If you meant freddy.with_trace {...}.deliver_with_response then that can work but it looks very inconvenient for most applications. This would mean that the users have to get the tracing information from respond_to (as a trace object or a wrapped freddy object) and then pass it along to every deliver, deliver_with_response call.

Zipkin client for example also does this singleton approach (even though I don't like their api much).

I think this solution does not prevent later adding .with_trace or some similar solution.

deiwin commented 7 years ago

Discussed in person and agreed to stick with thread-local singletons.

However, also agreed to change the spans to be created on the requester's side instead of on the responder's side. Might also create another span on the responder's side to be able to separate the time spent on the wire from the time spent on processing the actual request.

indrekj commented 7 years ago

Decided to go over span and logging logic in the next PR. Merging this as it already gives benefit of seeing which services are interacting with each other.