feat(monitor): improve power monitor with dynamic collection based on interval #2016
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## reboot #2016 +/- ##
==========================================
- Coverage 93.78% 93.16% -0.63%
==========================================
Files 21 21
Lines 1159 1200 +41
==========================================
+ Hits 1087 1118 +31
- Misses 57 65 +8
- Partials 15 17 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sthaha
force-pushed
the
feat-monitor-with-interval
branch
7 times, most recently
from
105ea67 to
9472e01
Compare
sthaha
changed the title
sthaha
force-pushed
the
feat-monitor-with-interval
branch
2 times, most recently
from
daa47b3 to
da3e8d2
Compare
internal/monitor/monitor.go
Outdated
| if err := pm.collectData(); err != nil { | ||
| pm.logger.Error("Failed to collect power data", "error", err) | ||
| } | ||
| pm.scheduleNextCollection() |
There was a problem hiding this comment.
call collectionLoop() here?
There was a problem hiding this comment.
I find this "easier" to reason ...
-
collectionLoopstarts the collection and thenschedules Nextif there is an interval otherwise it exits -
scheduleNext() schedules the next collection (recursive) and aborts if canceled. I.E this is called only if there is an interval.
|
please rebase to |
sthaha
force-pushed
the
feat-monitor-with-interval
branch
3 times, most recently
from
e217121 to
7badc4a
Compare
internal/monitor/monitor.go
Outdated
| default: | ||
| pm.logger.Debug("Data channel is full, dropping signal") |
There was a problem hiding this comment.
nit: no need to say dropping signal , seems something negative has happened.
internal/monitor/monitor.go
Outdated
| } | ||
| } | ||
|
|
||
| func (pm *PowerMonitor) Run(ctx context.Context) error { | ||
| pm.logger.Info("Monitor is running...") | ||
| go pm.collectionLoop() // NOTE: runs in background |
There was a problem hiding this comment.
there is no need to run this in separate go routine, its lifetime is short and is not a loop.
There was a problem hiding this comment.
its lifetime will be long as soon as we add the process and containers and pods - right? collectionLoop runs an initial collection (time consuming) and then schedules next.
--- second thought ---
We can avoid the go call since Run does is to wait on context after the collection is started.
There was a problem hiding this comment.
this function does first collection, schedules timer, and returns.
| timer := pm.clock.After(pm.interval) | ||
| go func() { | ||
| select { | ||
| case <-timer: |
There was a problem hiding this comment.
we can get the current time from this channel, can avoid calling Now() in refresh data
There was a problem hiding this comment.
Now() should refer to run and not when the collection started, so caching/ reusing the current time isn't IMHO the right approach. Also, lets keep the args to a function as minimum as possible.
This refactor enhances the power monitoring system with the following improvements: - Implement dynamic collection scheduling with configurable intervals - Add option to disable automatic collection (interval=0) for on-demand only mode - Use a context for clean cancellation of collection goroutines Signed-off-by: Sunil Thaha <[email protected]>
sthaha
force-pushed
the
feat-monitor-with-interval
branch
from
7badc4a to
1b33e5b
Compare
vimalk78
merged commit 8ab13c6
into
sustainable-computing-io:reboot
This refactor enhances the monitor as follows: