lightstep / lightstep-tracer-go

The Lightstep distributed tracing library for Go
https://lightstep.com
MIT License
98 stars 54 forks source link

inject collectorpb.CollectorServiceClient #215

Closed thepwagner closed 5 years ago

thepwagner commented 5 years ago

This adds the ability to inject a custom protobuf transport. This requires migrating protobuf conversion from collectorClients to the tracerImpl, which is a hard step away from non-Protobuf transports.

I'm running tracers in a network without access to the collector, so I'd like to bring my own transport API. The grpcCollectorClient is close, but crufty.

austinlparker commented 5 years ago

I thought we had actually removed everything but protobuf in the most recent release, but yeah, we're generally in favor of moving all of the tracers towards only using proto.

austinlparker commented 5 years ago

The changes look fine overall and I'm fine with the refactor if you want to take a stab at it, or we can merge as-is.

thepwagner commented 5 years ago

@austinlparker thanks for the feedback!

I've migrated protoConverter, reporterID and attributes from each collectorClient implementation to the tracerImpl. That leaves collectorClient.Translate() operating on an already converted protobuf request, but otherwise unchanged. (I toyed with removing reportRequest intermediary, but didn't want to mess with the semantics/error behaviour of Translate)

Some tangential changes snuck into the HTTP collector too:

thepwagner commented 5 years ago

Is there any timeline for merging/releasing this?

austinlparker commented 5 years ago

Whoops, slipped off my plate. I'll get to merging this today!

austinlparker commented 5 years ago

This is released in v0.17.0