-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stats: add BeginTime to stats.End #1907
Conversation
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
I completed the CLA form. |
Client: true, | ||
Error: err, | ||
BeginTime: beginTime, | ||
EndTime: time.Now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that end time was not being set here.
Thanks for the PR. I'm fine with this change, but with the caveat that this API is experimental and will be redesigned in the somewhat-near future. There is a fundamental problem with this stats API and the capabilities of replay. I plan to address this in a gRFC at some point in the future. In a revamp, I don't intend to pass either begin or end time, since those are literally |
Are there preliminary comments about the "capabilities of replay" you mention? |
Sorry, I meant to say "retry", but it's specifically the replaying of messages that are a problem. The gRFC for retry is here for reference. The issue with stats is that we include the message payload in the call to the stats handler, but the payload is no longer owned by gRPC after the Generally, though, the stats API wasn't designed with retry in mind. E.g. there is no distinction between an RPC call being performed by the application and individual attempts being sent on the wire. |
Got it. That's fairly problematic. Thanks for letting me know. |
This change allows
stats.Handler
implementations easily determine duration.