Add sql connection pool metrics #8071
Conversation
|
|
vanhtuan0409
changed the title
bergundy
changed the title
Head branch was pushed to by a user without write access
| started int32 | ||
| stopped int32 |
There was a problem hiding this comment.
Can use atomic bools here: https://pkg.go.dev/sync/atomic#Bool
There was a problem hiding this comment.
But also wondering why this would be started multiple times since it's only instantiated in the handle constructor and immediately started there.
There was a problem hiding this comment.
I switched it to atomic.Bool in 921be96
Yeah, Start will probably only be called once, but Stop might be called multiple times, which I'm not sure about. Just keep it symmetric with the stop.
Before the guard check, the Go race detector complains about a race condition in WaitGroup. I'm adding these to resolve the race condition
| if atomic.LoadInt32(&r.stopped) == 1 { | ||
| return | ||
| } |
There was a problem hiding this comment.
A but redundant IMHO but no objection to keep this.
There was a problem hiding this comment.
sure enough. I'm keeping this at the moment
Co-authored-by: Roey Berman <[email protected]>
Co-authored-by: Roey Berman <[email protected]>
| reporter.started.Store(false) | ||
| reporter.stopped.Store(false) |
There was a problem hiding this comment.
No need, the zero value is false.
| if !r.started.CompareAndSwap(false, true) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Still don't feel like we need this but 🤷
| r.wg.Add(1) | ||
| go r.run() |
There was a problem hiding this comment.
| r.wg.Add(1) | |
| go r.run() | |
| r.wg.Go(r.run) |
| } | ||
|
|
||
| func (r *DBMetricsReporter) run() { | ||
| defer r.wg.Done() |
There was a problem hiding this comment.
oh nice. I just know about this method. Unfortunately, it is only available with go 1.25. Temporal is using go 1.24 instead
There was a problem hiding this comment.
That's not true anymore
Line 3 in 16b1e29
|
the CI is failing. Do I need to rebase or checking anything @bergundy . Look like some docker service is missing |
|
I merged main into the PR branch, lets see if that fixes CI. |
|
All good. It is great working with you |
|
I'm merging, would have liked some of the small comments to have been addressed but there's nothing critical blocking this right now. Thanks for the contribution. |
|
|
||
| func (r *DBMetricsReporter) report() { | ||
| db := r.handle.db.Load() | ||
| if db == nil { |
There was a problem hiding this comment.
@bergundy should stats go to zero if this occurs?
There was a problem hiding this comment.
Hmm... this is just an edge case. I would say it's undefined. Most of the time the gauge will stay at the last value for the host.
What changed?
Adding sql connection pool metrics from stdlib
StatsWhy?
Expose more metrics for config fine tuning
How did you test it?