Add concurrency to the fetchData function #781
Conversation
Signed-off-by: Justin Kolberg <amd.prophet@gmail.com>
amdprophet
force-pushed
the
fetchData-concurrency
branch
from
978f2c0 to
043a62b
Compare
Codecov Report
@@ Coverage Diff @@
## master #781 +/- ##
==========================================
- Coverage 22.27% 21.59% -0.68%
==========================================
Files 28 28
Lines 2375 2454 +79
==========================================
+ Hits 529 530 +1
- Misses 1781 1859 +78
Partials 65 65
Continue to review full report at Codecov.
|
Signed-off-by: Justin Kolberg <amd.prophet@gmail.com>
Signed-off-by: Justin Kolberg <amd.prophet@gmail.com>
Signed-off-by: Justin Kolberg <amd.prophet@gmail.com>
There was a problem hiding this comment.
Some notes from an online-discussion I had with Justin:
-
Test coverage for the daemon package is quite low, so these types of changes will need cautious manual testing.
-
While there is room for improvement in the implementation, it would require substantial refactoring of not only this PR but likely also parts of Uchiwa.
-
Uchiwa is in maintenance mode, so it's not clear that there would be a good cost/benefit ratio to doing this work.
-
The fetcher methods need to be mutex locked for their duration (excluding the GetFoo methods at the top of each fetcher)
-
The PR should be tested manually with the race detector enabled, to try to tease out any concurrency bugs that might exist.
-
While channels could be employed here, the centralized state inherent in the design would limit their usefulness.
uchiwa/daemon/daemon.go
Outdated
| go d.fetchAggregates() | ||
|
|
||
| if f.enterprise { | ||
| go d.fetchEnterpriseMetrics() |
There was a problem hiding this comment.
Fixed in the latest commit.
uchiwa/daemon/daemon.go
Outdated
| } | ||
|
|
||
| // fetch sensu data from the datacenter | ||
| wg.Add(8) |
There was a problem hiding this comment.
Fixed in the latest commit.
There was a problem hiding this comment.
Do defer mu.Unlock() immediately after locking mutexes, to make sure they are unlocked properly no matter what happens in the function afterwards.
Do this for wg.Done() as well. Even if the code is currently correct, defer tends to harden the code against future breakage, if a maintainer adds another return point to the function.
uchiwa/daemon/daemon.go
Outdated
|
|
||
| func (d *DatacenterSnapshotFetcher) fetchSilenced() { | ||
| silenced, err := d.datacenter.GetSilenced() | ||
| d.mutex.Lock() |
There was a problem hiding this comment.
A better pattern is
d.mutex.Lock()
defer d.mutex.Unlock()
This ensures that the mutex is always unlocked.
uchiwa/daemon/daemon.go
Outdated
| } | ||
|
|
||
| d.mutex.Unlock() | ||
| d.wg.Done() |
There was a problem hiding this comment.
defer d.wg.Done() at the top of the function.
There was a problem hiding this comment.
Changed - and also made use of the mutex in fetchEvents().
Signed-off-by: Justin Kolberg <amd.prophet@gmail.com>
Signed-off-by: Justin Kolberg amd.prophet@gmail.com
Description
Adds concurrency to
fetchData()indaemon.go. Sensu API performance becomes the main limiting factor in Uchiwa performance with this change.Related Issue
#750
Motivation and Context
Uchiwa becomes very slow with one or more large Sensu installations.
How Has This Been Tested?
Benchmarked against three staged Sensu datacenters:
Old:
New:
Screenshots (if appropriate): N/A
Types of changes
Checklist: