opencensus-scala icon indicating copy to clipboard operation
opencensus-scala copied to clipboard

Add support for akka-grpc

Open aperkins1310 opened this issue 7 years ago • 4 comments

Hi all,

Would you consider adding support for akka-gRPC? I've given it a bit of a try and to get something working was fairly straightforward, but I did come across one issue so far, which it would be nice to get your input on.

At the moment in: https://github.com/census-ecosystem/opencensus-scala/blob/a97018d6c6553e6ca42f872ae2aedce1079bb843/akka-http/src/main/scala/io/opencensus/scala/akka/http/utils/EndSpanResponse.scala#L37 there is a use of transformDataBytes, which throws an IllegalArgumentException when using gRPC. https://github.com/akka/akka-http/blob/3feb8b617bddaae28bf85c08bb2fc1f5c6901c8c/akka-http-core/src/main/scala/akka/http/scaladsl/model/HttpEntity.scala#L504

As far as I can tell, it is only used to finish the span in case of upstream failure or finish within the request -> response stream, and doesn't actually transform the bytes at all.

Therefore in EndSpanResponse, could those lines be switched for a simple:

    tracing.endSpan(span, StatusTranslator.translate(response.status.intValue()))
    response

The tests all pass, as I believe upstream failures or finishes are covered by other paths, but wanted to get your opinion as I may be missing something.

Once that’s cleared up I’d be happy to work towards an akka-grpc module, if that's something you'd be interested in?

aperkins1310 avatar Aug 01 '18 19:08 aperkins1310

Hey, sounds very interesting. I think we would be happy about that contribution! The transformDataBytes is there because we want to end the stream only after the response's Entity was consumed. This makes sure we do not end the span too early. I have not worked with akka-gRPC yet, but I think we should be able to find a solution :)

yannick-cw avatar Aug 01 '18 20:08 yannick-cw

Ah okay, yep having looked through the history I see it used to be what I suggested, and got updated. If you want to see the problem I started an example project to show this: https://github.com/aperkins1310/akka-grpc-quickstart-scala, running both the server and client should give you the exception in the server.

For a bit of context, akka-gRPC is currently using the akka-http server (running http2), and a wrapper around the gRPC default netty-based client for now. In my example, to get the server traced I made a copy of the TracingDirective object with the traceRequest methods not private. In the recordSuccess you can then swap between the standard EndSpanResponse which gives this error, or MyEndSpanResponse which makes the above change and then works.

I think the problem I've seen is more general than just akka-gRPC, in that any chunked response with trailer headers in the last chunk will throw this error, but I'm guessing that is quite rare. I couldn't find an immediate solution to this, but a few potential ones which you may have a better idea on :)

  • As a starting point, use the 'new' method I suggested (which is actually just old) for akka-gRPC alone. This wouldn't be as accurate but could be an easy option as a first iteration
  • Find another way of triggering the span ending after all bytes have been consumed - I can't find any other way of doing this other than transformDataBytes, but that may well be my lack of knowledge
  • 'Fix' transformDataBytes to work with metadata - My initial thought was that this could separate out the metadata from the bytes, transform the bytes and then reapply the metadata, but this seems a bit dodgy, and I'm not actually sure this would be possible regardless
  • Change akka-gRPC to not put metadata headers in their chunked responses. I'm fairly sure they are just conforming to the gRPC protocol there though: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md

It looks to me like having a akka-grpc module which starts with the simple option and then working towards finding a way of having a handler/callback on the Entity being consumed could work, but you may have a better idea of how easy/difficult having that handler could be

aperkins1310 avatar Aug 02 '18 08:08 aperkins1310

Mh also not sure if it is possible without transformDataBytes. I do agree to having a akka-grpc module which starts with the simple option. @Sebruck wdyt?

yannick-cw avatar Aug 03 '18 10:08 yannick-cw

@aperkins1310 I just saw that I totally missed to follow up on this one. If you are still interesting in adding an akka-grpc module to this project, I'd be super happy!

Sebruck avatar Nov 16 '18 15:11 Sebruck