From 0a0e3b0d16506a21add0ab32ccff6f76c894e313 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Thu, 30 Mar 2017 20:12:03 -0700 Subject: [PATCH] Refactor store code to be a little less ugly This makes the code a little bit cleaner and safer, as it moves lots of locks into the store package, but it could still use some work. --- filters/filter_fuzzy.go | 2 +- filters/filter_silence_author.go | 8 ++--- filters/filter_silence_jira.go | 9 ++--- filters/filter_test.go | 7 ++-- store/store.go | 60 +++++++++++++++----------------- store/store_test.go | 57 ++++++++++++++++++++++++++++++ timer.go | 17 ++------- views.go | 18 +++++----- 8 files changed, 107 insertions(+), 71 deletions(-) create mode 100644 store/store_test.go diff --git a/filters/filter_fuzzy.go b/filters/filter_fuzzy.go index 1cc0200a5..6c24a013d 100644 --- a/filters/filter_fuzzy.go +++ b/filters/filter_fuzzy.go @@ -42,7 +42,7 @@ func (filter *fuzzyFilter) Match(alert *models.UnseeAlert, matches int) bool { } if alert.Silenced != "" { - if silence, found := store.SilenceStore.Store[alert.Silenced]; found { + if silence := store.Store.GetSilence(alert.Silenced); silence != nil { if filter.Matcher.Compare(silence.Comment, filter.Value) { filter.Hits++ return true diff --git a/filters/filter_silence_author.go b/filters/filter_silence_author.go index a34e294d4..f2a389da6 100644 --- a/filters/filter_silence_author.go +++ b/filters/filter_silence_author.go @@ -16,11 +16,9 @@ func (filter *silenceAuthorFilter) Match(alert *models.UnseeAlert, matches int) if filter.IsValid { var isMatch bool if alert.Silenced != "" { - store.StoreLock.RLock() - if silence, found := store.SilenceStore.Store[alert.Silenced]; found { + if silence := store.Store.GetSilence(alert.Silenced); silence != nil { isMatch = filter.Matcher.Compare(filter.Value, silence.CreatedBy) } - store.StoreLock.RUnlock() } else { isMatch = filter.Matcher.Compare("", filter.Value) } @@ -42,8 +40,7 @@ func sinceAuthorAutocomplete(name string, operators []string, alerts []models.Un tokens := map[string]models.UnseeAutocomplete{} for _, alert := range alerts { if alert.Silenced != "" { - store.StoreLock.RLock() - if silence, found := store.SilenceStore.Store[alert.Silenced]; found && silence.CreatedBy != "" { + if silence := store.Store.GetSilence(alert.Silenced); silence != nil { for _, operator := range operators { token := fmt.Sprintf("%s%s%s", name, operator, silence.CreatedBy) tokens[token] = makeAC(token, []string{ @@ -54,7 +51,6 @@ func sinceAuthorAutocomplete(name string, operators []string, alerts []models.Un }) } } - store.StoreLock.RUnlock() } } acData := []models.UnseeAutocomplete{} diff --git a/filters/filter_silence_jira.go b/filters/filter_silence_jira.go index 690f0367c..5bb42ae64 100644 --- a/filters/filter_silence_jira.go +++ b/filters/filter_silence_jira.go @@ -16,11 +16,9 @@ func (filter *silenceJiraFilter) Match(alert *models.UnseeAlert, matches int) bo if filter.IsValid { var isMatch bool if alert.Silenced != "" { - store.StoreLock.RLock() - if silence, found := store.SilenceStore.Store[alert.Silenced]; found { + if silence := store.Store.GetSilence(alert.Silenced); silence != nil { isMatch = filter.Matcher.Compare(silence.JiraID, filter.Value) } - store.StoreLock.RUnlock() } else { isMatch = filter.Matcher.Compare("", filter.Value) } @@ -42,8 +40,8 @@ func sinceJiraIDAutocomplete(name string, operators []string, alerts []models.Un tokens := map[string]models.UnseeAutocomplete{} for _, alert := range alerts { if alert.Silenced != "" { - store.StoreLock.RLock() - if silence, found := store.SilenceStore.Store[alert.Silenced]; found && silence.JiraID != "" { + silence := store.Store.GetSilence(alert.Silenced) + if silence != nil && silence.JiraID != "" { for _, operator := range operators { token := fmt.Sprintf("%s%s%s", name, operator, silence.JiraID) tokens[token] = makeAC(token, []string{ @@ -54,7 +52,6 @@ func sinceJiraIDAutocomplete(name string, operators []string, alerts []models.Un }) } } - store.StoreLock.RUnlock() } } acData := []models.UnseeAutocomplete{} diff --git a/filters/filter_test.go b/filters/filter_test.go index 4013f31bb..cf7cb5bea 100644 --- a/filters/filter_test.go +++ b/filters/filter_test.go @@ -281,10 +281,11 @@ var tests = []filterTest{ func TestFilters(t *testing.T) { for _, ft := range tests { if &ft.Silence != nil { - store.SilenceStore.Store = map[string]models.UnseeSilence{} - store.SilenceStore.Store[ft.Silence.ID] = ft.Silence + store.Store.SetSilences(map[string]models.UnseeSilence{ + ft.Silence.ID: ft.Silence, + }) } else { - store.SilenceStore.Store = map[string]models.UnseeSilence{} + store.Store.SetSilences(map[string]models.UnseeSilence{}) } f := filters.NewFilter(ft.Expression) if f == nil { diff --git a/store/store.go b/store/store.go index 647010a1e..1870d15b4 100644 --- a/store/store.go +++ b/store/store.go @@ -2,45 +2,43 @@ package store import ( "sync" - "time" "github.com/cloudflare/unsee/models" ) -type alertStoreType struct { - Store []models.UnseeAlertGroup - Timestamp time.Time +type dataStore struct { + Lock sync.RWMutex + Alerts []models.UnseeAlertGroup + Silences map[string]models.UnseeSilence + Colors models.UnseeColorMap + Autocomplete []models.UnseeAutocomplete } -type silenceStoreType struct { - Store map[string]models.UnseeSilence - Timestamp time.Time +// Store will keep all Alertmanager data we collect +var Store = dataStore{} + +// GetSilence returns silence data for specific silence id or nil if not found +func (ds *dataStore) GetSilence(s string) *models.UnseeSilence { + ds.Lock.RLock() + defer ds.Lock.RUnlock() + if silence, found := ds.Silences[s]; found { + return &silence + } + return nil } -type colorStoreType struct { - Store models.UnseeColorMap - Timestamp time.Time +// SetSilences allows to update silence list stored internally +func (ds *dataStore) SetSilences(s map[string]models.UnseeSilence) { + ds.Lock.Lock() + defer ds.Lock.Unlock() + ds.Silences = s } -type autocompleteStore struct { - Store []models.UnseeAutocomplete - Timestamp time.Time +// Update will lock the store and update internal data +func (ds *dataStore) Update(alerts []models.UnseeAlertGroup, colors models.UnseeColorMap, autocomplete []models.UnseeAutocomplete) { + ds.Lock.Lock() + defer ds.Lock.Unlock() + ds.Alerts = alerts + ds.Colors = colors + ds.Autocomplete = autocomplete } - -var ( - // StoreLock guards access to all variables storing internal data - // (alerts, silences, colors, ac) - StoreLock = sync.RWMutex{} - - // AlertStore holds all alerts retrieved from Alertmanager - AlertStore = alertStoreType{} - - // SilenceStore holds all silences retrieved from Alertmanager - SilenceStore = silenceStoreType{} - - // ColorStore holds all color maps generated from alerts - ColorStore = colorStoreType{} - - // AutocompleteStore holds all autocomplete data generated from alerts - AutocompleteStore = autocompleteStore{} -) diff --git a/store/store_test.go b/store/store_test.go new file mode 100644 index 000000000..354aa89a4 --- /dev/null +++ b/store/store_test.go @@ -0,0 +1,57 @@ +package store_test + +import ( + "testing" + + "github.com/cloudflare/unsee/models" + "github.com/cloudflare/unsee/store" +) + +type silenceTest struct { + silences map[string]models.UnseeSilence + silenceId string + found bool +} + +var silenceTests = []silenceTest{ + silenceTest{ + silences: map[string]models.UnseeSilence{ + "1": models.UnseeSilence{}, + }, + silenceId: "1", + found: true, + }, + silenceTest{ + silences: map[string]models.UnseeSilence{ + "1": models.UnseeSilence{}, + "2": models.UnseeSilence{}, + "3": models.UnseeSilence{}, + }, + silenceId: "2", + found: true, + }, + silenceTest{ + silences: map[string]models.UnseeSilence{}, + silenceId: "1", + found: false, + }, + silenceTest{ + silences: map[string]models.UnseeSilence{ + "2": models.UnseeSilence{}, + "3": models.UnseeSilence{}, + }, + silenceId: "1", + found: false, + }, +} + +func TestSilences(t *testing.T) { + for _, testCase := range silenceTests { + store.Store.SetSilences(testCase.silences) + silence := store.Store.GetSilence(testCase.silenceId) + found := silence != nil + if found != testCase.found { + t.Errorf("GetSilence('%s') returned %v, %v was expected", testCase.silenceId, found, testCase.found) + } + } +} diff --git a/timer.go b/timer.go index 0359b4979..65f052f85 100644 --- a/timer.go +++ b/timer.go @@ -6,7 +6,6 @@ import ( "io" "runtime" "sort" - "time" "github.com/cloudflare/unsee/alertmanager" "github.com/cloudflare/unsee/config" @@ -56,10 +55,7 @@ func PullFromAlertmanager() { } } - store.StoreLock.Lock() - store.SilenceStore.Store = silenceStore - store.SilenceStore.Timestamp = time.Now() - store.StoreLock.Unlock() + store.Store.SetSilences(silenceStore) alertStore := []models.UnseeAlertGroup{} colorStore := make(models.UnseeColorMap) @@ -148,16 +144,7 @@ func PullFromAlertmanager() { metricAlerts.With(prometheus.Labels{"silenced": "false"}).Set(counterAlertsUnsilenced) metricAlertGroups.Set(float64(len(alertStore))) - now := time.Now() - - store.StoreLock.Lock() - store.AlertStore.Store = alertStore - store.AlertStore.Timestamp = now - store.ColorStore.Store = colorStore - store.ColorStore.Timestamp = now - store.AutocompleteStore.Store = acStore - store.AutocompleteStore.Timestamp = now - store.StoreLock.Unlock() + store.Store.Update(alertStore, colorStore, acStore) log.Info("Pull completed") apiCache.Flush() runtime.GC() diff --git a/views.go b/views.go index 50bebf4d7..72dd610f0 100644 --- a/views.go +++ b/views.go @@ -123,10 +123,10 @@ func alerts(c *gin.Context) { silences := map[string]models.UnseeSilence{} colors := models.UnseeColorMap{} counters := models.UnseeCountMap{} - store.StoreLock.RLock() + store.Store.Lock.RLock() var matches int - for _, ag := range store.AlertStore.Store { + for _, ag := range store.Store.Alerts { agCopy := models.UnseeAlertGroup{ ID: ag.ID, Labels: ag.Labels, @@ -155,8 +155,8 @@ func alerts(c *gin.Context) { io.WriteString(h, string(aj)) if alert.Silenced != "" { - if silence, found := store.SilenceStore.Store[alert.Silenced]; found { - silences[alert.Silenced] = silence + if silence := store.Store.GetSilence(alert.Silenced); silence != nil { + silences[alert.Silenced] = *silence } agCopy.SilencedCount++ countLabel(counters, "@silenced", "true") @@ -166,7 +166,7 @@ func alerts(c *gin.Context) { } for key, value := range alert.Labels { - if keyMap, foundKey := store.ColorStore.Store[key]; foundKey { + if keyMap, foundKey := store.Store.Colors[key]; foundKey { if color, foundColor := keyMap[value]; foundColor { if _, found := colors[key]; !found { colors[key] = map[string]models.UnseeLabelColor{} @@ -207,7 +207,7 @@ func alerts(c *gin.Context) { panic(err) } apiCache.Set(cacheKey, data, -1) - store.StoreLock.RUnlock() + store.Store.Lock.RUnlock() c.Data(http.StatusOK, gin.MIMEJSON, data.([]byte)) logAlertsView(c, "MIS", time.Since(start)) @@ -241,8 +241,8 @@ func autocomplete(c *gin.Context) { acData := []string{} - store.StoreLock.RLock() - for _, hint := range store.AutocompleteStore.Store { + store.Store.Lock.RLock() + for _, hint := range store.Store.Autocomplete { if strings.HasPrefix(strings.ToLower(hint.Value), strings.ToLower(term)) { acData = append(acData, hint.Value) } else { @@ -253,7 +253,7 @@ func autocomplete(c *gin.Context) { } } } - store.StoreLock.RUnlock() + store.Store.Lock.RUnlock() sort.Strings(acData) data, err := json.Marshal(acData)