Add racecar integration
#268
Conversation
p-lambert
added
do-not-merge/WIP
p-lambert
force-pushed
the
pedro/add-racecar-integration
branch
from
8f5aa2b
to
c0ff401
Compare
| require_relative 'tracer' | ||
|
|
||
| ::Racecar.singleton_class.class_eval do | ||
| alias_method :__instrumenter, :instrumenter |
There was a problem hiding this comment.
Would it be possible to make this work now that the start_ notifications are emitted?
p-lambert
force-pushed
the
pedro/add-racecar-integration
branch
3 times, most recently
from
a8ec3fc
to
b878e26
Compare
| ::ActiveSupport::Notifications.subscribe('start_process_batch.racecar', &method(:start)) | ||
| ::ActiveSupport::Notifications.subscribe('start_process_message.racecar', &method(:start)) | ||
| ::ActiveSupport::Notifications.subscribe('process_batch.racecar', &method(:finish)) | ||
| ::ActiveSupport::Notifications.subscribe('process_message.racecar', &method(:finish)) |
There was a problem hiding this comment.
I think you need to separate message processing from batch processing – at least give the spans different names.
There was a problem hiding this comment.
Hey @dasch! Apologies for the late reply. I'm not sure if I fully understand the problem here. As far as I understand each worker either implements a process or process_batch method, correct? So each span will belong to a resource that univocally represents either a batch or a message message consumer.
There was a problem hiding this comment.
That's true, but a service can change from one to the other, and often will – batching is a performance optimization. Naming the top-level span either process_message or process_batch would make it clear what the trace represents.
There was a problem hiding this comment.
I see! I'll make that change then!
| span.set_tag('kafka.consumer', payload[:consumer_class]) | ||
| span.set_tag('kafka.partition', payload[:partition]) | ||
| span.set_tag('kafka.offset', payload[:offset]) if payload.key?(:offset) | ||
| span.set_tag('kafka.first_offset', payload[:first_offset]) if payload.key?(:first_offset) |
There was a problem hiding this comment.
This mixes up batch processing and individual message processing. For the former, you'd be interested in
- The start and end offsets
- The batch size
For the latter you're just interested in the offset of that specific message.
There was a problem hiding this comment.
@dasch I'm conditionally setting the fields kafka.offset and kafka.first_offset.
- For the
process_message.racecarevent you provide a payload withconsumer_class,topic,partitionandoffsetso these are set in the span accordingly. - For the
process_batch.racecar, event you provide a payload withconsumer_class,topic,partitionandfirst_offsetwhich are the fields set in batch consumer spans.
Let me know if that makes sense!
There was a problem hiding this comment.
Sure, I was just confused by both message and batch processing was mixed together. For batch processing, the number of messages is obviously important.
There was a problem hiding this comment.
So do you plan to update the instrumentation payload with that field?
There was a problem hiding this comment.
Ah, thought I already had that! I'll add it now.
There was a problem hiding this comment.
You can add the field conditionally, I'll merge this ASAP.
p-lambert
force-pushed
the
pedro/add-racecar-integration
branch
from
99b313d
to
ce91385
Compare
p-lambert
force-pushed
the
pedro/add-racecar-integration
branch
from
ce91385
to
21924ea
Compare
This PR provides instrumentation support for racecar.
Usage