cronet: report statsTraceCtx.clientOutboundHeaders() #4768
Conversation
| // Now that the stream is ready, call the listener's onReady callback if | ||
| // appropriate. | ||
| state.onStreamAllocated(); | ||
| statsTraceCtx.clientOutboundHeaders(); |
There was a problem hiding this comment.
At what point are the headers written to the socket?
There was a problem hiding this comment.
It looks like I misread the implementation here. They may or may not have been written to socket by this point, and the Cronet API doesn't offer notification w.r.t. when the write actually occurs: source is at https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java?l=491&rcl=c1141f37e26d971f835f5958b2eec7627286616b
Given this, it probably doesn't make sense to report clientOutboundHeaders here, which means Cronet transport will never log this event. If this event is only applicable to certain (i.e., non-Cronet) transports, could we consider removing the assert at https://github.com/grpc/grpc-java/blob/master/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java#L1868 in the interop test?
There was a problem hiding this comment.
I see there is onWriteCompleted() callback, with a ByteBuffer passed in. Will there be a ByteBuffer for headers that will receive such notification?
There was a problem hiding this comment.
I don't think so. Headers are provided to the stream by a separate mechanism, without a ByteBuffer.
There was a problem hiding this comment.
We can probably just mark the first onWriteCompleted() as the most accurate time for clientOutboundHeaders(). It's better than nothing.
There was a problem hiding this comment.
Sgtm, thanks. Done
2 participants
This brings Cronet client's stats reporting inline with InProcess, Netty, and OkHttp.
This was caught by AbstractInteropTest#assertClientStatsTrace when running the interop tests with a Cronet transport.