feat(pool): Add EmptyAcquireWaitTime stats #34
Conversation
This measures time for resource acquisition for cases when pool is empty and it had to wait or construct new resource.
|
This is to implement what was discussed in jackc/pgx#1669 |
| @@ -363,11 +373,13 @@ func (p *Pool[T]) acquire(ctx context.Context) (*Resource[T], error) { | |||
|
|
|||
| // If a resource is available in the pool. | |||
| if res := p.tryAcquireIdleResource(); res != nil { | |||
| waitTime := time.Duration(nanotime() - startNano) | |||
There was a problem hiding this comment.
Not directly related to your PR as you are reusing something that existed, but I'm curious
Any reason to use startNano := nanotime, and not a start := time.Now() and here a time.Now().Since(start)?
There was a problem hiding this comment.
IDK, as you said I am reusing previous approach to get time. nanotime is provided via linking so I assume there is some platform specific trickery to make it cheaper to call maybe?
There was a problem hiding this comment.
Yes, directly calling nanotime is a micro optimization.
On my machine an uncontested Acquire and Release cycle takes 103ns with time.Now but only 56ns with nanotime directly.
There was a problem hiding this comment.
OK. So maybe a comment about it could be interesting, because the pattern is not obvious.
Or maybe I missed it
|
It looks fine. But maybe add some tests just to sanity check it. |
redbaron
force-pushed
the
empty-acquire-time-stats
branch
from
95fa753 to
865f1d3
Compare
pool_test.go
Outdated
| @@ -692,6 +693,7 @@ func TestPoolStatSuccessfulAcquireCounters(t *testing.T) { | |||
| assert.Equal(t, int64(2), stat.AcquireCount()) | |||
| assert.Equal(t, int64(1), stat.EmptyAcquireCount()) | |||
| assert.True(t, stat.AcquireDuration() > lastAcquireDuration) | |||
| assert.True(t, stat.EmptyAcquireWaitTime() < stat.AcquireDuration()) | |||
There was a problem hiding this comment.
assert.Greater should be preferred to compare things. The error reported if it fails will be clearer
You should also change the one on previous line.
There was a problem hiding this comment.
I would like to recommend you using testifylint available via golangci-lint.
I can help if you create an issue for me, assign it me, and you are not in a hurry 🤣
pool_test.go
Outdated
| @@ -682,6 +682,7 @@ func TestPoolStatSuccessfulAcquireCounters(t *testing.T) { | |||
| assert.Equal(t, int64(1), stat.AcquireCount()) | |||
| assert.Equal(t, int64(1), stat.EmptyAcquireCount()) | |||
| assert.True(t, stat.AcquireDuration() > 0, "expected stat.AcquireDuration() > 0 but %v", stat.AcquireDuration()) | |||
pool_test.go
Outdated
| @@ -712,6 +714,7 @@ func TestPoolStatSuccessfulAcquireCounters(t *testing.T) { | |||
| assert.Equal(t, int64(4), stat.AcquireCount()) | |||
| assert.Equal(t, int64(2), stat.EmptyAcquireCount()) | |||
| assert.True(t, stat.AcquireDuration() > lastAcquireDuration) | |||
| assert.True(t, stat.EmptyAcquireWaitTime() < stat.AcquireDuration()) | |||
|
@jackc , I have updated existing tests to add checks for new stats |
This measures time for resource acquisition for cases when pool is empty and it had to wait or construct new resource.