search

openzipkin / brave

Java distributed tracing implementation compatible with Zipkin backend services.
Apache License 2.0
2.36k stars 714 forks source link

deprecate "grpc-trace-bin" #1178

Open codefromthecrypt opened 4 years ago

codefromthecrypt commented 4 years ago

gRPC's "grpc-trace-bin" was a special header made by the OpenCensus project and later removed, when OpenCensus was removed from gRPC.

As google integrated this into Envoy, there could be people using this propagation format https://github.com/grpc/grpc-java/blob/1bf5ad15272dd53e55bc37863ecaa1e3eb424bfa/xds/third_party/envoy/src/main/proto/envoy/config/trace/v2/trace.proto#L156

Note: "grpc-trace-bin" was one choice of a few, so presence of OpenCensus in envoy doesn't mean this will be in use.

Anyway I suggest we deprecate this format for removal as it was never documented and is also a dead format. In worst case, we can suggest a ServerInterceptor which converts it to "b3" format when present.

codefromthecrypt commented 4 years ago

fyi envoy people @dio @basvanbeek

dimi-nk commented 4 years ago

@adriancole is there a source that grpc-trace-bin has been abandoned? I can see it being used in grpc-java.

codefromthecrypt commented 4 years ago

@dimi-nk grpc-trace-bin was undocumented, but used by default when tracing enabled in grpc until this removed it https://github.com/grpc/grpc-java/pull/6577 gRPC dropped Census as a core dependency in v1.27. You can see in the source history from now, only maintenance since.

The format grpc-trace-bin was only used by census (even if a hole was punched in envoy for it). When gRPC removed census it broke our tests that used grpc-trace-bin, so we removed those.

Census mentions in their own README that they are in sunset mode https://github.com/census-instrumentation/opencensus-java#opencensus---a-stats-collection-and-distributed-tracing-framework

hope that helps

dimi-nk commented 4 years ago

Thanks @adriancole, that's perfect

codefromthecrypt commented 4 years ago

This deprecates it, but backfills tests as @dimi-nk noticed the feature broke https://github.com/openzipkin/brave/pull/1262

codefromthecrypt commented 4 years ago

@dimi-nk should be all good in Brave 5.13.0 just released, and apologies we missed this.