feat(v2): update Invoke to add retry count to context#462
feat(v2): update Invoke to add retry count to context#462quartzmo merged 5 commits intogoogleapis:mainfrom
Conversation
Expose the retry attempt count to the transport layer for tracing
shollyman
left a comment
shollyman
left a comment
There was a problem hiding this comment.
This kind of PR brings out the semantic demons in me.
We should be crystal clear here with the meanings and namings here. Are we attaching the attempt id, the attempt count, or the cumulative number of resend attempts? Each is a slightly different thing.
For example, the first RPC sent could have an attempt ID of 0 if we use a zero-based index, an attempt count of 1, and a resend count of 0.
The fact that the attribute and the option are differently named adds to my confusion and worry here.
| ctx = c | ||
| } | ||
|
|
||
| attempt := 0 |
There was a problem hiding this comment.
I think a zero-based attempt # can be a bit confusing.
The requirement it to track retry_count where the first attempt is not set and the subsequent requests are 1, 2, 3. I don't love that name, either.
In Rust we called it prior_attempt_count -- so the first one is 0. That might help?
Whatever you pick for the names, it's worth some docs to describe how it is meant to be used. So if you kept attempt you'd put:
"attempt is the index (0-based) of the request being made. The first request is attempt = 0 and retries are 1, 2, 3 etc. This is used to populate retry_count when attempt is higher than 0."
There was a problem hiding this comment.
Got it, will clarify that the value will be used for retry_count.
quartzmo
changed the title
|
Updated, PTAL. |
Co-authored-by: Wesley Tarle <westarle@google.com>
shollyman
left a comment
shollyman
left a comment
There was a problem hiding this comment.
Generally LGTM, just a comment trying to nail down the nuances for my own clarity.
| // On a second request (the first retry), retry count is 1. | ||
| func withRetryCount(ctx context.Context, retryCount int) context.Context { | ||
| // Add to gRPC metadata so it's visible to StatsHandlers | ||
| return metadata.AppendToOutgoingContext(ctx, "gcp.grpc.resend_count", strconv.Itoa(retryCount)) |
There was a problem hiding this comment.
I'm not super clear on the transport agnosticism here, as the key and comments indicate this is gRPC only. If we're going to have a different function per transport, it might be worth naming them based on the transport type. Then again this is not an exported func so this is a decision that can be punted to future PRs.
There was a problem hiding this comment.
We're using gRPC metadata for both transports. I can clarify that here if you like. It's also discussed somewhat in the design doc.
3 participants
Expose the retry attempt count to the transport layer for tracing