Remove EmitZipkinBatch([]*zipkincore.Span) from agent reporter

[pavolloffay]

Copied from See #1181 (comment)

Right now we have this flow:

udp server jaeger.thrift -> reporter::SubmitBatch
udp server zipkin.thrift -> reporter::SubmitZipkinSpans

tchannel reporter::SubmitBatch -> pass through to collector
tchannel reporter::SubmitZipkinSpans -> pass through to collector

grpc reporter::SubmitBatch -> convert to domain and send to collector
grpc reporter::SubmitZipkinSpans -> convert to jaeger.thrift, then to domain and send to collector

What I am proposing, is to eliminate reporter::SubmitZipkinSpans completely and have this:

udp server jaeger.thrift -> reporter::SubmitBatch
    tchannel reporter::SubmitBatch -> pass through to collector
    grpc reporter::SubmitBatch -> convert to domain and send to collector
udp server zipking.thrift -> convert to jaeger.thrift -> reporter::SubmitBatch

This way the Reporter only has a single method for Jaeger spans (in Thrift, for now).

Once we remove tchannel completely, we can push thrift->domain conversion into UDP servers as well, and Reporter interface effectively becomes == Collector service from proto IDL.

[pavolloffay]

Also remove duplicated metrics in reporter

jaeger_agent_tchannel_reporter_batches_submitted{format="jaeger"} 0
jaeger_agent_tchannel_reporter_batches_submitted{format="zipkin"} 0

[yurishkuro]

Also update changelog per #1180 (comment)

[pavolloffay]

@yurishkuro do you want why this has a list of batches? https://github.com/jaegertracing/jaeger/blob/master/thrift-gen/jaeger/tchan-jaeger.go#L39, The IDL defines only a single batch object.

[pavolloffay]

I would like to clarify the scope of this issue. Do we want to remove zipkin.thrift reporting on the agent plus collecting on the collector side?

#1186 would be simpler and more straight forward, otherwise, we will have to keep some parts of the zipkin path.

Also there is no direct converter from zipkin thrift to jaeger. We might go through model converter as workaround...

[yurishkuro]

@yurishkuro do you want why this has a list of batches?

Looks like a list: https://github.com/jaegertracing/jaeger-idl/blob/23730e3f90bf592e6518060c437171093dd3a1e4/thrift/jaeger.thrift#L83-L85

As to why - the thinking was that agent did not have to proxy batches 1-1, it could have pre-aggregated them. In order to support sending batches from multiple services we needed a list.

You're correct that if we do #1186 we can simply remove zipkin handling from the agent altogether. Maybe we can punt on it for now, as we will need time to move that code into internal repo, to maintain backwards compatibility (and it might be tricky to incorporate a whole new path like that without changes in the core).

[pavolloffay]

oh for some reason I was looking at agent.thrift where it is defined as a single object.

Just comment when we can go forward and remove it. If you can upgrade all agents we could just remove the submitbatches from zipkin.thrift, keep it on agent with empty impl. you would just implement it with sanitizers like in grpc path.

[yurishkuro]

I think that was my original proposal - still allow agent to accept zipkin over udp, but internally convert to jaeger before sending to collectors. Then it is still backwards compatible with legacy clients.