salemove / jaeger-client-ruby

OpenTracing Tracer implementation for Jaeger in Ruby
MIT License
62 stars 38 forks source link

Move inject & extract logic to their own classes #15

Closed meinac closed 5 years ago

meinac commented 5 years ago

Injecting and extracting trace IDs have been moved to their own classes and DSL implemented to register injectors and extractors.

indrekj commented 5 years ago

Can you elaborate what's the reason for this? Would #14 also satisfy your requirements?

meinac commented 5 years ago

The reason is, people may want to use their own injectors and extractors. With the current implementation there is no good way to use your own. Actually the pr you mentioned is why this way would be nice. People can easily register their injectors and extractors as they want, like so;

Jaeger::Client::Injector.register('MY_FORMAT', MyInjectorClass)

Also Java client has the same approach.

indrekj commented 5 years ago

I'm just curios which injectors/extractors do you require?

With the #14 you can do propagation_codec: YouCodecImplementation with inject and extract methods, so looks like it should also satisfy it.

meinac commented 5 years ago

We are using kafka with our own message schema which already has trace id attribute and I don't want to change that. #14 also works but it requires you to use different clients because with that implementation you can only use one codec class but with this implementation you can register a new one or override already existing one. As far as I see thats the only big difference. Of course you can create a subclass of already existing codec class and override inject and extract methods accordingly but it requires more work and error prone. I think giving a way to register codecs should be the way to go, you can also put other codecs from other pr to default supported codecs list but of course its up to you, mine is just recommendation.

indrekj commented 5 years ago

I don't think it requires that much extra work. So basically you want to enable a new format. So you can do:

FORMAT_KAFKA = 999
class YourPropagationCodec < Jaeger::Client::PropagationCodec::JaegerCodec
  def self.inject(span_context, format, carrier) 
    return super if format != FORMAT_KAFKA

    # do your thing
  end

  def self.extract(format, carrier)
    return super if format != FORMAT_KAFKA

    # do your thing
  end
end

This interface is the same as OpenTracing::Tracer #inject and #extract.

vs:

FORMAT_KAFKA = 999

KafkaInjector = Class.new do
  def self.inject(span_context, carrier)
    # do your thing
  end
end
Jaeger::Client::Injector.register(FORMAT_KAFKA, KafkaInjector)

KafkaExtractor = Class.new do
  def self.extract(carrier)
    # do your thing
  end
end
Jaeger::Client::Extractor.register(FORMAT_KAFKA, KafkaExtractor)

I'm not really sold with either one of them yet. They actually seem a little different. #14 is about adding a completely new codec (b3). In your case you actually want to add a new format. So it feels like it isn't this PR or the other, but rather they are complementing each other. In your case you could do http header injection with b3 header format and text map injection with jaeger format, which may actually be also useful.

jaeger-python has just propagation flag, but jaeger-java has a more extendable interface.

Anyway, it's past midnight here. I will keep both PRs open for a day or two and think a bit more about this. I will also check out how jaeger-java codec extraction/injection looks in practice.

meinac commented 5 years ago

Even though both approaches look similar, they differ when you try to use global_tracer. With the other approach you have to have different clients or one client with a proxy codec class. Otherwise both are useful. I would keep the new codecs from other pr and stick with this registration interface because it's easy to document and easy to extend.

indrekj commented 5 years ago

ca8299a7b09b216fd34a67340e40f8d459ec274f injectors and extractors can now be specified when initializing the tracer