[GitHub] [skywalking] ZackButcher commented on issue #5358: Unary alternative to `TraceSegmentReportService.collect`

[GitBox <gi...@apache.org>]

ZackButcher commented on issue #5358:
URL: https://github.com/apache/skywalking/issues/5358#issuecomment-677711581


   I think there's a little confusion here:
   - Primary purpose for this is not performance, but to make the client code easier in Envoy to forward data to skywalking
     - As I understand it, using a stream greatly complicates the code due to Envoy's threading model; @dio and @lizan know better than I how that works
   - Like Lizan says, `unary` is just a special case of `stream`
     - `unary` is a `stream` that has exactly 1 message sent and received; `stream` can be `n` message sent `m` message received
     - Because of this, `unary` can have slightly _worse_ performance than `stream` in many/most cases
   - Like Sheng says, there is no `one-way` (i.e. "fire and forget") calls in gRPC - `unary` gRPC requires a response (even if the response is empty)
     - This _can_ affect throughput in some cases, depending on how exactly the client and server are pooling threads that handle gRPC requests
     - If SW collector can receive `unary` data, push the request processing to an executor (thread pool), then return a response, you would not see any blocking/queueing on the gRPC stream (though you might see queueing of tasks to be done in your thread pool if agent is outpacing collector). For example, in golang gRPC creates goroutine per `unary` request to avoid this.
     - In general, gRPC avoids head of line blocking (@lizan can keep me honest here)
   - Like Dhi points out in the document he links, SW collector use case is not _really_ one that's intended for streaming gRPC (though in practice it performs better, so it makes sense to use it for most agents).
   
   > I think you will only have one channel and one stub for sending this.
   > My understanding, this is no real one-way call, as unary, you need to wait for the previous list sent completed.
   > My experience is for the agent case, streaming could perform better in a large data set. So, depending on the Envoy team.
   
   I think this is the key confusion - @dio can correct me - but I believe we'd have a stub per Envoy-worker-process, not just a single stub. This is part of what results in the simplification in code on Envoy side. Further, multiple unary RPCs with the same stub won't block each other. gRPC uses H2 multiplexing here; when it comes to literally writing the bytes to the wire, it uses a round-robin strategy across pending requests so that small requests "flow past" large ones. (Again, @lizan please correct me if I got some of the gRPC internals wrong.)
   
   Overall glad to see y'all are fine to add unary - thanks @wu-sheng!
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org