> > I don't know of any libraries doing this wrong. > >From what I can tell, both OpenTracing[1] and the OpenTelemetry[2] interceptors are doing it the way I wrote in my pseudo-code, which I believe we agreed was "wrong" (activating the client span on both paths).
[1] https://github.com/opentracing-contrib/java-grpc/blob/master/src/main/java/io/opentracing/contrib/grpc/TracingClientInterceptor.java [2] https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/grpc-1.5/library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_5/client/TracingClientInterceptor.java > No, there's no write-up. I honestly don't know what would be put in a > general-purpose document. I do think there could be something like "this is > how to integrate gRPC with MDC". But really, it seems best to just have a > repo with some code somewhere with utilities instead of each user > reinventing the wheel. > Well my first suggestion was to improve the Javadoc to clarify the placement / ordering of the client interceptor (i.e.: the fact that a client interceptor's method will be ). This, I believe, would also qualify as a "general-purpose" document, see below. > > Maybe the documentation could make that more clear. Maybe we should also > downplay which runs "first" and emphasize placement further or closer to > network/application. > Right, I think this might be what a "general-purpose" document might cover. Thinking about interceptors as a stack is probably the best way, e.g.: interceptors are (conceptually) pushed onto a stack on the outbound path and then popped on the inbound path. With experience, it's evident that this is desirable, but it's also not necessarily intuitive for newcomers. Furthermore, interceptors are typically part of "framework-type" code so they're usually pretty important to "get right" and a unit test of a single interceptor will not make this behaviour evident; so documentation would go a long way IMO. > > It can, by intercepting both the Call+Listener. But generally it is >>> nonsensical to do so, since in one case it would call the library and in >>> the other the application. >>> >> >> Yes, I agree with that. But my particular goal is to instrument the gRPC >> interceptors themselves. The application is "shielded" from this because it >> has its own "request-local" state that it will restore when the callback >> reaches it. >> >> For context, my particular goal is to make sure that some logging >> interceptor has visibility into the current span so it can add the span's >> ID to the log statements for correlation purposes. In practice, we do this >> through the "MDC" logging feature, but it's conceptually the same. So it >> seems important that this "tracing interceptor" be "before" the logging one >> in both directions. >> > > I see. MDC. I do think you should split the problem into two different > parts: MDC-propagation and Span-creation. Those should be separate > components and be considered separately. And they have to be considered > separately for client-side and server-side. > > For client-side, I think you could have an MDC-propagation interceptor > that sits very close to the network. Any interceptor that modifies MDC > would modify the downward calls (Interceptor, ClientCall) and restore the > parent value on the upward calls (Listener) (as discussed elsewhere). > Right, this is our current state for client interception (in interceptForward order): tracing | MDC | logging (each one is a separate ClientInterceptor). The issue is that the logging interceptor is first on the inbound path, so it doesn't have its MDC setup. So we ended up having to setup another interceptor to re-initialize the MDC on the inbound path, e.g.: tracing | MDC | logging | MDC' It does indeed make sense to have an interceptor as close to the network as possible, but it also does not seem sufficient to have a single interceptor if we also expect the logging interceptor to also do things on the outbound path. > For server-side, I think you set the Span on the downward calls > (Interceptor, Listener)... and you could restore the original value on the > upward calls (ServerCall), but it seems nothing may care. It seems there > wouldn't be any MDC propagation here. > Right, similarly, our setup looks like this (in interceptForward order): tracing | MDC | logging, but that ended up working fine since we don't currently do anything with ServerCall per-say. But indeed, it seems like we'd have the same problem here. > > Does that sound like it would work? > It certainly clarifies things. In our case, I believe some interceptor concerns do need to be split in 2 pieces that coordinate with one-another. This is obviously a design decision of ours and isn't directly applicable to other users... > Okay, so it is a "log the RPC" interceptor. I don't know if this is on > client-side or server-side. I think splitting the MDC propagation from the > span creation addresses this on client-side. Server-side is harder because > there's nothing that would *guarantee* the "correct" span to be active in > the MDC when the server responds; the interceptor may need to save the span > in the request path to use it in the response path. > Yes, thanks for the help, this did in fact clarify things enough for us to address the problem. Cheers -- You received this message because you are subscribed to the Google Groups "grpc.io" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/CAFYs7S8VMaQoGk49OGuze5b8o%2BG5V5n85_BHcWEkynUd07TsYw%40mail.gmail.com.
