fix(spanner): attempt latency for streaming call should capture the total latency till decoding of protos #11039
Conversation
rahul2393
force-pushed
the
fix-stream-attempt-latencies
branch
from
e895070 to
1bf1615
Compare
rahul2393
force-pushed
the
fix-stream-attempt-latencies
branch
from
1bf1615 to
82b19f0
Compare
There was a problem hiding this comment.
Do we have any way to test this? There are a lot of corner cases where we are retrying the query stream etc., and it is very hard to tell from just looking at the code that we actually cover all corner cases, and that it actually records the latency that we want it to record.
| } | ||
| } | ||
| defer func() { | ||
| if mt.method != "" { |
There was a problem hiding this comment.
This could use a comment. Does mt.method != "" mean that the operation completed? Why?
| } | ||
| statusCode, _ := convertToGrpcStatusErr(r.err) | ||
| mt.currOp.setStatus(statusCode.String()) | ||
| recordOperationCompletion(&mt) |
There was a problem hiding this comment.
This seems to indicate that we will record the latency of the last attempt that finished as the operation latency. That means that an operation that is retried will not contain the entire latency, but only the latency of the last attempt. Is that intentional? If so, could we document that in code to clarify?
There was a problem hiding this comment.
Actually the currOp start time is initalised in first line of Next() and in recordOperationCompletion we capture to total time including multiple attempts latencies
There was a problem hiding this comment.
Ack. I see now that there is a separate field for the operation start time.
…otal latency till decoding of protos.
rahul2393
force-pushed
the
fix-stream-attempt-latencies
branch
from
82b19f0 to
d9edbe3
Compare
rahul2393
force-pushed
the
fix-stream-attempt-latencies
branch
2 times, most recently
from
df2c835 to
fea6538
Compare
rahul2393
force-pushed
the
fix-stream-attempt-latencies
branch
from
fea6538 to
2404e56
Compare
No description provided.