Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

Add support for akka-grpc #60

Open
aperkins1310 opened this issue Aug 1, 2018 · 4 comments
Open

Add support for akka-grpc #60

aperkins1310 opened this issue Aug 1, 2018 · 4 comments

Comments

@aperkins1310
Copy link

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:

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?

@yannick-cw
Copy link
Contributor

yannick-cw commented Aug 1, 2018

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 :)

@aperkins1310
Copy link
Author

aperkins1310 commented Aug 2, 2018

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

@yannick-cw
Copy link
Contributor

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?

@Sebruck
Copy link
Member

Sebruck commented Nov 16, 2018

@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!

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

No branches or pull requests

3 participants