thriftbp: Report request/response payload sizes from client side #650
Conversation
We currently already have a middleware to report that from the server side, but that's simulated by reconstruct THeaderProtocols and read/write to them, which will not include the header size and will not reflect compressions (zlib, etc.). This client side reporting is done directly at the TTransport (TSocket) level, so it will reflect the exact number actually read/written at that level. If zlib is enabled in THeaderProtocol the size will reflect compressed payload. The only thing it will not reflect is potential SSL layer (e.g. when using TSSLSocket over TSocket, which we don't support yet).
fishy
requested review from
kylelemons,
konradreiche and
pacejackson
and removed request for
a team
why not? the server ones always come with the big caveat that they're estimations (see the doccomment here), and also the client metrics will have different labels compared to the server ones. we can potentially do the same thing on the server side in the future (make the server one more accurate), but that requires open up new apis from upstream thrift (in order to do that we need both of these things at the same time: 1) access to the underlying |
There was a problem hiding this comment.
the reason is that if you want to look at server bandwidth and see a mismatch between the client and the server, you might be rightfully confused. Having different names for the metrics could help to clarify the difference. I'm not sure how many people really look at bandwidth though, so very shrug.
We currently already have a middleware to report that from the server side, but that's simulated by reconstruct THeaderProtocols and read/write to them, which will not include the header size and will not reflect compressions (zlib, etc.).
This client side reporting is done directly at the TTransport (TSocket) level, so it will reflect the exact number actually read/written at that level. If zlib is enabled in THeaderProtocol the size will reflect compressed payload. The only thing it will not reflect is potential SSL layer (e.g. when using TSSLSocket over TSocket, which we don't support yet).