Brave interceptors with netty

[chiragarora17]

Hi, Question regarding integrating brave with Netty. looking at brave-cxf3 project, I get that we need to implement some kind of ChannelOutboundHandlerAdapter or ChannelInboundHandlerAdapter which examines the request / response and then handle the request accordingly.

But since netty is based on channels, do we have to Implement something for the client too?

In cxf3 we store the ServerSpan object in the context... if we don't store the object in the span (if our server doesn't support state) then can we just perform responseInterceptor.handle and assume ZipKin will try to connect the dots?

[FreeSlaver]

@chiragarora17

But since netty is based on channels, do we have to Implement something for the client too?

Yes,you should.If don't implements,the client span can't generate and be recorded.

if we don't store the object in the span (if our server doesn't support state) then can we just perform responseInterceptor.handle and assume ZipKin will try to connect the dots?

What do you mean by store object in the span?What's the object? I think Zipkin can't connect the dots.

I will do something more to figure out this problem.

[codefromthecrypt]

Ps probably a good time to revisit this since we have a new instrumentation directory. If anyone wants to make a netty handler (http or otherwise) please report back.

On 5 May 2017 6:17 pm, "宋鑫" notifications@github.com wrote:

@chiragarora17 https://github.com/chiragarora17

But since netty is based on channels, do we have to Implement something for the client too?

Yes,you should.If don't implements,the client span can't generate and be recorded.

if we don't store the object in the span (if our server doesn't support state) then can we just perform responseInterceptor.handle and assume ZipKin will try to connect the dots?

What do you mean by store object in the span?What's the object? I think Zipkin can't connect the dots.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openzipkin/brave/issues/296#issuecomment-299430262, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD610sEF0ZwbsQZhzV3MKjpr1hQbQjUks5r2vcpgaJpZM4LMYFl .

[FreeSlaver]

@adriancole I am doing this work now,a netty http handler.

[codefromthecrypt]

@songxin1990 fyi latest master has refined the HttpServerHandler and client side, too. might want to take a look

[FreeSlaver]

@adriancole Sure,I will take a look at it and fit the convention.

[FreeSlaver]

@adriancole so,I should create a project under brave-instrumentation?

[codefromthecrypt]

@adriancole https://github.com/adriancole so,I should create a project under brave-instrumentation?

you can.. worst case is that discussion suggests we host it somewhere else. However, easiest to start with this.

[FreeSlaver]

many new code and lamda expression been used, so I need some time.

[codefromthecrypt]

Brave 4.3 is being synced now, but this can go in 4.3.1 or later depending on when you finish

[FreeSlaver]

@adriancole All the junit tests have been passed except the reportsSpanOnTransportException and addsErrorTagOnTransportException. Here are some problems about Netty exception interceptor:

1. If user override exceptionCaught(ChannelHandlerContext ctx, Throwable cause) method and don't call ctx.fireExceptionCaught(cause),then none of the next pipeline channelhandler will be acknowledged.
2. If user didn't call ctx.write(response) in exceptionCaught method,the brave response interceptor will not be invoked,the server span won't be generated.

Here is some suggestion for discussion:

//Do not use  SimpleChannelInboundHandler,it will release msg,then next pipeline won't be called
public class UserDefinedInHandler extends ChannelInboundHandlerAdapter {
  @Override
  public void channelReadComplete(ChannelHandlerContext ctx) throws Exception {
    ctx.flush();
  }
  @Override
  public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
    //process http request
  }
  /**
   * Here are two choices:
   * 1. Do not override this method,leave it to Brave response interceptor
   * 2. Process exceptions,then call ctx.fireExceptionCaught(cause) to propagate the exception to next pipeline.
   */
  /*@Override
  public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)throws Exception{

  }*/
}

Brave response interceptor:

//Use ChannelDuplexHandler will be better as write response will cause exception too.
public ChannelDuplexHandler createHttpResponseHandler() {
    return new ChannelOutboundHandlerAdapter() {
      @Override
      public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
          throws Exception {
        if (msg instanceof HttpResponse) {
          HttpResponse response =
              (HttpResponse) msg;
          responseInterceptor.handle(
              new HttpServerResponseAdapter(new NettyServerResponse(response)));
        }
        super.write(ctx, msg, promise);
      }
      @Override
      public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
          throws Exception {
        if(cause!=null){
          String message = cause.getMessage();
          if(message == null) {
            message = cause.getClass().getSimpleName();
            serverTracer.submitBinaryAnnotation(Constants.ERROR,message);
          }
        }
        super.exceptionCaught(ctx, cause);
      }
}

[FreeSlaver]

I just don't know how to intercept exceptions outside consistency,if user use different approach with exceptionCaught method showed above.

[codefromthecrypt]

can you raise a PR? easier for discussion this way, also easier for us to comment on certain lines..

[codefromthecrypt]

ps don't raise one against serverTracer api, rather the new one. we won't be adding any new apis based on servertracer

[FreeSlaver]

@adriancole Ok,I will raise a new PR using the new apis ASAP.

[codefromthecrypt]

@songxin1990 how's it going?

[FreeSlaver]

@adriancole After this weekend.I have gone to travel these days.

[codefromthecrypt]

@chiragarora17 interested? #441