retry: add onRetry callback

[ericwenn]

Currently (v1.2.2) the retry interceptor logs retries to the trace, see https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/retry/retry.go#L49

It would be great if we could hook in our own logger here via options. Something along the lines of:

type OnRetryCallback func(ctx context.Context, attempt uint, err error)

func WithOnRetryCallback(fn OnRetryCallback) CallOption

Additional context
We have our own request logger for gRPC Client calls, which we are currently wrapping outside retry.UnaryClientInterceptor, to only log the "final" response (after potential retries). We only retry Unavailable status codes. This is great as we are not alerted for any intermittent Unavailable status codes, but we have come across situations where we think a request should have been retried but can't really know what the retry interceptor is doing.

** Maintainer edit**: merged this with #375 and updated description.

[johanbrandhorst]

This seems reasonable, but would you be willing to add it to the v2 branch instead? We're moving away from adding new features to master.

[ericwenn]

I see, completely missed there was a separate v2 track going on with this repository, my bad.
Just for understanding, would this mean the timeline of this feature (should it happen) would be tied to the release of v2 of this library?

[johanbrandhorst]

Yes. We're working on v2 right now, you can track the progress on the issue tracker.

[ericwenn]

v2 is fine by me, do you need any action from me?

[johanbrandhorst]

We'd love to have you implement this feature on the v2 branch :). We're probably not going to get around to it ourselves, we're pretty contribution-driven. You've already found the place to add it I believe so if you can put up a PR with a test that ought to be enough to have this merged 😄.

[ericwenn]

Gotcha, will look into it! thanks for swift replies.

[johanbrandhorst]

I merged #375 into this, I think this is still a good proposal, but we can make it more generic than just for logging. The signatures stay the same but we'd call it something more generic.

[flapjack103]

Thanks for this addition! I recently added the new call option WithOnRetryCallback to be passed to the UnaryClientInterceptor and request errors are invoking the callback function but what I'm seeing is that this is invoked even for non-retriable status codes. This contradicts the documentation.

Looking at where this callback function is invoked, it seems it ought be done after the isRetriable check.
Let me know if I should open a separate issue for this.

[johanbrandhorst]

Hi, I think that should be a separate issue, and it seems to me that this issue can be closed 😄.