Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

brave-census bridge #626

Closed
codefromthecrypt opened this issue Mar 1, 2018 · 4 comments
Closed

brave-census bridge #626

codefromthecrypt opened this issue Mar 1, 2018 · 4 comments

Comments

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Mar 1, 2018

@jorgheymans currently uses brave-instrumentation-grpc and is looking to interop with census for the grpc layer. His application architecture looks like

incoming=rest -> mid tier grpc/kafka/ldap -> outgoing=rest

So, if census-grpc uniformly handles that layer (still reporting to zipkin). Assuming brave doesn't implement the census api layer (which I suppose it could), the essence of this integration are the following:

  • use grpc context as brave's CurrentTraceContext so that propagation works
  • When reading the current trace context, look at census to see if there's an upstream span and convert that accordingly to a brave TraceContext.
  • When writing the current trace context, make a fake Census span which delegates to the current brave span.

In looking at census grpc instrumentation, the parent is only read (not modified), so this should work. It won't work with advanced features like brave's extra field propagation, and it won't propagate things absent in census like the parent ID. However I still think it should work somewhat.

A design note: it would be cleaner if census didn't propagate a span rather just the trace context. Since it propagates the span, we have to create a span object to integrate, which is heavier (because it is stateful type) and makes the integration approach clunky due to this (like us needing to make a more expensive fake span just to get the 'current parent' code which only reads the reference to work). If census had a mode where it only propagated the trace context (impling a recording function based on trace context internally), this integration would be neater imho.

So the alternates I'm aware of are:

  • brave implementing census tracer api (which makes sense if you are sending to zipkin anyway as the signal loss is the same as exporting to zipkin)
  • brave implementing the same grpc tracer as census (much easier, but doesn't actually integrate with census)
@codefromthecrypt
Copy link
Member Author

PS what led to this was some curiosity about latency in a gRPC app. The brave grpc integration still uses interceptor approach (as we are back compat all the way till grpc 1.3). We haven't done a grpc tracer implementation for reasons including it seems the api isn't stable yet, and maybe census integration can solve this.

@jorgheymans
Copy link
Contributor

Thanks for drafting your thoughts on this, as said let me know if there is something you want me to test.

codefromthecrypt pushed a commit that referenced this issue Apr 11, 2018
This prefers the OpenCensus binary formatted trace header, falling back
to Brave defaults if absent, upon server requests. On the client side,
both are written.

This allows interop with gRPC services that don't use B3 (for example,
the default census module). This also acknowledges that the OpenCensus
binary format is fairly contained within gRPC, so doesn't need to become
a top-level propagation format, for example used with non-gRPC services
as yet.

Redundantly encoding this format has some cost to it, notable 14
character header name "grpc-trace-bin" and the 38 character base64
encoded binary context value.

Note that this only allows black-box interop with Census. It does not
(yet) interop with census in the same process as Brave. However, this
should be a helpful step to those especially currently using Brave, but
trying out Census for non-java services.

See #626
@codefromthecrypt
Copy link
Member Author

first start #688

@codefromthecrypt
Copy link
Member Author

census no longer is a dependency of grpc, as of the latest version. I think this is not something we will progress as census is also supposed to be switched out with open telemetry once that's out of alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants