Fix gRPC client interceptor for channels reused across traces#539
Fix gRPC client interceptor for channels reused across traces#539c24t merged 4 commits intocensus-instrumentation:masterfrom
Conversation
|
I can reproduce the underlying problem reliably with a standard Flask integration using the FlaskMiddleware. This change does fix the Flask server for me. I don't fully understand what the intent was with instantiating the client interceptor this way (I looked at the git history, still unclear). It's clearly broken though for common server use cases. I may be missing some context and maybe there are other use cases where this made sense? Would love any hints on that. |
b741f93 to
9601b84
Compare
In a typical web server gRPC clients/channels are reused across multiple requests and hence across multiple traces. Previously the `OpenCensusClientInterceptor` was instantiated for each channel with the current tracer from the execution context. This would then lead to all rpcs going through that channel to have the same tracer, essentially grouping all rpcs under whatever happened to be the current trace when the channel was created. Instead instantiate `OpenCensusClientInterceptor` without a tracer by default. The current tracer will be retrieved from the execution context at the start of every rpc span. In addition `OpenCensusClientInterceptor` was manipulating thread-local state via the execution context. This seems unnecessary and misguided. The current span state is already managed by the spans/context tracers. Setting the tracer explicitly risks further subtle bugs. Also removes unused method `OpenCensusClientInterceptor._end_span_between_context`. Fixes census-instrumentation#182
9601b84 to
eef5aa4
Compare
I don't understand it either. If the goal was to be able to use Removing |
c24t
left a comment
There was a problem hiding this comment.
This seems like an unambiguously good change to me, let's see if @liyanhui1228 or @wkiser have any objections.
In a typical web server gRPC clients/channels are reused across multiple
requests and hence across multiple traces. Previously the
OpenCensusClientInterceptorwas instantiated for each channel with thecurrent tracer from the execution context. This would then lead to all
rpcs going through that channel to have the same tracer, essentially
grouping all rpcs under whatever happened to be the current trace when the
channel was created.
Instead instantiate
OpenCensusClientInterceptorwithout a tracer bydefault. The current tracer will be retrieved from the execution context at
the start of every rpc span.
In addition
OpenCensusClientInterceptorwas manipulating thread-local statevia the execution context. This seems unnecessary and misguided. The current
span state is already managed by the spans/context tracers. Setting the
tracer explicitly risks further subtle bugs.
Also removes unused method
OpenCensusClientInterceptor._end_span_between_context.Fixes #182