Add garbage collection support to the Prometheus metrics adjuster#613
Add garbage collection support to the Prometheus metrics adjuster#613songy23 merged 8 commits intocensus-instrumentation:masterfrom dinooliva:metrics-adjuster
Conversation
Codecov Report
@@ Coverage Diff @@
## master #613 +/- ##
=========================================
Coverage ? 69.16%
=========================================
Files ? 92
Lines ? 6110
Branches ? 0
=========================================
Hits ? 4226
Misses ? 1664
Partials ? 220
Continue to review full report at Codecov.
|
|
@rghetia - ptal |
rghetia
left a comment
There was a problem hiding this comment.
Overall looks good.
One comment regarding Lock/UnLock. Typically you want Lock followed by defer UnLock. That is not always possible. But if it is then we should follow that.
| } | ||
| jm.lastGC = time.Now() | ||
| } | ||
| jm.Unlock() |
There was a problem hiding this comment.
jm.Unlock can be defer jm.Unlock right after jm.Lock
I would like to put tsm.Lock() inside tsm.gc() but I am not sure that would work. The question is will anothe goroutine will have tsm while jm is Locked above?
There was a problem hiding this comment.
Yes, I can move the tsm.lock() to tsm.gc().
It is is possible that the metricsadjuster could mark the tsm and it will be removed from the JobsMap when it didn't need to be but that would happen even with locking (in a slightly different order).
songy23
left a comment
There was a problem hiding this comment.
LGTM except minor nit. Please rebase.
| jm.Lock() | ||
| defer jm.Unlock() | ||
| // recheck lastGC to make sure gc is necessary | ||
| if current.Sub(jm.lastGC) > jm.gcInterval { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, the previous check is done before locking jm - this check is to verify that no other gc() has happened in the interim before we grab the lock. I'll add a comment to clarify what's going on.
No description provided.