zio-grpc icon indicating copy to clipboard operation
zio-grpc copied to clipboard

Propagate current gRPC Context inside call environment

Open ak0rz opened this issue 4 years ago • 7 comments

Hello, @thesamet

I've encountered a problem while integrating my service with a third-party (opentracing) ServerCallInterceptor, which propagates actual Span via gRPC Context mechanism.

Unfortunately, Context.current() is unavailable inside service implementation because it's backed by ThreadLocal inside gRPC and needs to be propagated from calling gRPC thread.

This tiny change allows us to capture current Context inside RequestContext environment.

I didn't know what to do with Scala.js dependency and added a dummy definition for it just like you did for Metadata in scalapb-grpcweb.

Hope you can look into it soon.

Cheers, Andrey

ak0rz avatar Sep 22 '20 11:09 ak0rz

Hey Andrey, thanks for putting this together. I looked into io.grpc.Context in the past, and if I recall correctly it does require special handling with ZIO due to it relying on thread local storage. For this reason, I would prefer to avoid introducing it to zio-grpc public API.

In the past, @frekw reported success integrating with zio-telementry, see some example code in #59 - can you look into this and check if this approach will be useful?

thesamet avatar Sep 22 '20 15:09 thesamet

Hey Andrey, thanks for putting this together. I looked into io.grpc.Context in the past, and if I recall correctly it does require special handling with ZIO due to it relying on thread local storage. For this reason, I would prefer to avoid introducing it to zio-grpc public API.

In the past, @frekw reported success integrating with zio-telementry, see some example code in #59 - can you look into this and check if this approach will be useful?

Actually, default Context.Storage is backed by ThreadLocal, but Context itself is not. Context is an effectively immutable storage for propagating custom payloads like tracing information or security principals. I'm aware of meaningless of using static Context.current() inside of user code of zio-grpc, but I'm proposing to capture current state inside ZServerCallHandler.startCall (which is the last point that is getting executed inside gRPC thread pool) and propagate an effectively immutable instance of current Context to environment of user code.

We may also introduce a wrapper like SafeMetadata if it would be appropriate. I'll look into it today.

ak0rz avatar Sep 23 '20 08:09 ak0rz

@thesamet I've made it even better.

  1. Introduced package-private SafeContext trait for interop with JS (goodbye, dummy io.grpc.Context!)
  2. Made an SafeContext.Key[R, E, A] which should carry an effect for the default value (may be useful if you wish to compute value from an environment like falling back to root span in telemetry or indicate an appropriate error if needed).

What do you think about this approach?

ak0rz avatar Sep 23 '20 09:09 ak0rz

Taking a step back - I assume you need the context so you can extract the span and add additional traces from within your service - is that right? I prefer that we also evaluate a solution where we can integrate with zio-telemetry so we end up with a solution that tracing can be done more consistently in the ZIO ecosystem.

thesamet avatar Sep 23 '20 15:09 thesamet

Taking a step back - I assume you need the context so you can extract the span and add additional traces from within your service - is that right? I prefer that we also evaluate a solution where we can integrate with zio-telemetry so we end up with a solution that tracing can be done more consistently in the ZIO ecosystem.

Well, that's not the only purpose to use Context. If we rely somehow on grpc-java, it may be useful to have benefits of low-level access to protocol flow via interceptors. Context is a just a most convenient way to propagate something from them.

I think zio-telemetry integration must be done separately and may rely on requirement of SafeContext.

ak0rz avatar Sep 23 '20 16:09 ak0rz

@thesamet It's been a while since we've looked into this issue.

Do you still think it's a bad idea to introduce abstract API over which we may implement zio-telemetry integration or maybe this change is good enough to go?

ak0rz avatar Sep 28 '20 09:09 ak0rz

I am hesitant about introducing this change. The value add is that it enables using existing grpc-java interceptors, on other other hand, it introduces a class that only partially works and we need to make some effort to hide it.

The criteria I am looking for is whether there are real-world use cases that absolutely requires access to the Context. I haven't had a chance to look into it yet, but for opentracing, I think we get away by implementing our own ZServerInterceptor in zio.

thesamet avatar Sep 28 '20 14:09 thesamet