-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added resource pool wait time histogram metrics #5727
Added resource pool wait time histogram metrics #5727
Conversation
e291519
to
b891a76
Compare
go/pools/resource_pool.go
Outdated
@@ -325,6 +331,9 @@ func (rp *ResourcePool) SetCapacity(capacity int) error { | |||
func (rp *ResourcePool) recordWait(start time.Time) { | |||
rp.waitCount.Add(1) | |||
rp.waitTime.Add(time.Since(start)) | |||
if rp.name != "" { | |||
rp.waitStats.Record(rp.name+"ResourceWaitTime", start) |
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.
Doesn't the name get added automatically farther up the stack?
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.
Yes. As far as I know it is only used to publish stats.
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.
I would recommend passing in a logWait func(elapsed time.Dration)
instead. This will allow the name handling to live with the caller, which will be more readable.
Also, is this a production use case? I'm asking because there are other changes that are saying that we are over-reporting, and asking for more consolidations.
@sougou |
#5649 is one. I also know that vtgate over-reports on calls to vttablets. It's an issue if you have too many shards. I'm about to publish a proposal to help solve this problem. RFC will be out soon. |
@lcabancla see #5809. I think it alleviates the problem enough that this can be pushed through. Can you address the review comment? |
@sougou Yes, will work on the comment and update this. Thanks. |
d758fd1
to
5add09a
Compare
Signed-off-by: Lloyd Cabancla <[email protected]>
5add09a
to
5d172c4
Compare
Signed-off-by: Lloyd Cabancla <[email protected]>
@sougou Updated the PR per your comment. Please take a look. Thanks! |
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.
Looks like this feel through the cracks. Merging.
Currently, the vttablet currently only exposes resource pool wait count and aggregated wait times. Adding timing histogram metrics to the resource pool would provide information on the distribution of the wait time latencies. This is specially useful to in determining tail latencies.
Example output from
/debug/vars
(look forTransactionPoolResourceWaitTime
):