From 49257ec296c4529d4a69b20695d7fdc9fa451514 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Fri, 15 Mar 2019 10:49:33 +0000 Subject: [PATCH 1/3] fix(api): correctly deduplicate silences Right now deduplication only works when there's a single silence for the entire group, but not if the group is silences with multiple silences. This change fixes the logic so that we look at silences covering the entire group and move those into shared namespace --- internal/models/api.go | 47 +++++++++++++++------------- internal/models/api_test.go | 61 ++++++++++++++++++++++++++++++++----- 2 files changed, 79 insertions(+), 29 deletions(-) diff --git a/internal/models/api.go b/internal/models/api.go index ff2d40ba3..1805da55d 100644 --- a/internal/models/api.go +++ b/internal/models/api.go @@ -36,9 +36,9 @@ type LabelsColorMap map[string]map[string]LabelColors // APIAlertGroupSharedMaps defines shared part of APIAlertGroup type APIAlertGroupSharedMaps struct { - Annotations Annotations `json:"annotations"` - Labels map[string]string `json:"labels"` - Silences map[string]string `json:"silences"` + Annotations Annotations `json:"annotations"` + Labels map[string]string `json:"labels"` + Silences map[string][]string `json:"silences"` } // APIAlertGroup is how AlertGroup is returned in the API response @@ -140,42 +140,47 @@ func (ag *APIAlertGroup) dedupAnnotations() { } func (ag *APIAlertGroup) dedupSilences() { - ag.Shared.Silences = map[string]string{} + ag.Shared.Silences = map[string][]string{} - silencesByCluster := map[string][]string{} + silencesByCluster := map[string]map[string]int{} for _, alert := range ag.Alerts { if alert.State != AlertStateSuppressed { // if we find any alert that's not silenced then we can break early return } + // process each cluster only once, rather than each alertmanager instance + clusters := []string{} for _, am := range alert.Alertmanager { + if slices.StringInSlice(clusters, am.Cluster) { + continue + } + clusters = append(clusters, am.Cluster) for _, silenceID := range am.SilencedBy { _, ok := silencesByCluster[am.Cluster] if !ok { - silencesByCluster[am.Cluster] = []string{} + silencesByCluster[am.Cluster] = map[string]int{} } - if !slices.StringInSlice(silencesByCluster[am.Cluster], silenceID) { - silencesByCluster[am.Cluster] = append(silencesByCluster[am.Cluster], silenceID) + _, ok = silencesByCluster[am.Cluster][silenceID] + if !ok { + silencesByCluster[am.Cluster][silenceID] = 0 } + silencesByCluster[am.Cluster][silenceID]++ } } } - // only deduplicate if all alerts are silenced with the same silence from a - // single cluster - if len(silencesByCluster) != 1 { - return - } - - // now check that all alerts are silenced with the same silenceID - for cluster, silences := range silencesByCluster { - for _, silenceID := range silences { - if silenceID != silences[0] { - return + totalAlerts := len(ag.Alerts) + for cluster, silenceCountMap := range silencesByCluster { + for silenceID, affectedAlertsCount := range silenceCountMap { + if affectedAlertsCount == totalAlerts { + _, ok := ag.Shared.Silences[cluster] + if !ok { + ag.Shared.Silences[cluster] = []string{} + } + ag.Shared.Silences[cluster] = append(ag.Shared.Silences[cluster], silenceID) } } - ag.Shared.Silences[cluster] = silences[0] } } @@ -193,7 +198,7 @@ func (ag *APIAlertGroup) DedupSharedMaps() { ag.Shared = APIAlertGroupSharedMaps{ Labels: map[string]string{}, Annotations: Annotations{}, - Silences: map[string]string{}, + Silences: map[string][]string{}, } } } diff --git a/internal/models/api_test.go b/internal/models/api_test.go index ef42a2d71..8e1797798 100644 --- a/internal/models/api_test.go +++ b/internal/models/api_test.go @@ -12,7 +12,7 @@ import ( func TestDedupSharedMaps(t *testing.T) { am := models.AlertmanagerInstance{ Cluster: "fakeCluster", - SilencedBy: []string{"fakeSilenceID"}, + SilencedBy: []string{"fakeSilence1", "fakeSilence2"}, } ag := models.APIAlertGroup{ AlertGroup: models.AlertGroup{ @@ -37,7 +37,7 @@ func TestDedupSharedMaps(t *testing.T) { "job": "node_exporter", "instance": "1", }, - Alertmanager: []models.AlertmanagerInstance{am}, + Alertmanager: []models.AlertmanagerInstance{am, am}, }, models.Alert{ State: models.AlertStateSuppressed, @@ -52,7 +52,7 @@ func TestDedupSharedMaps(t *testing.T) { "job": "node_exporter", "instance": "2", }, - Alertmanager: []models.AlertmanagerInstance{am}, + Alertmanager: []models.AlertmanagerInstance{am, am}, }, models.Alert{ State: models.AlertStateSuppressed, @@ -67,7 +67,7 @@ func TestDedupSharedMaps(t *testing.T) { "job": "blackbox", "instance": "3", }, - Alertmanager: []models.AlertmanagerInstance{am}, + Alertmanager: []models.AlertmanagerInstance{am, am}, }, }, }, @@ -105,7 +105,21 @@ func TestDedupSharedMaps(t *testing.T) { "endsAt": "0001-01-01T00:00:00Z", "source": "", "silencedBy": [ - "fakeSilenceID" + "fakeSilence1", + "fakeSilence2" + ], + "inhibitedBy": null + }, + { + "name": "", + "cluster": "fakeCluster", + "state": "", + "startsAt": "0001-01-01T00:00:00Z", + "endsAt": "0001-01-01T00:00:00Z", + "source": "", + "silencedBy": [ + "fakeSilence1", + "fakeSilence2" ], "inhibitedBy": null } @@ -130,7 +144,21 @@ func TestDedupSharedMaps(t *testing.T) { "endsAt": "0001-01-01T00:00:00Z", "source": "", "silencedBy": [ - "fakeSilenceID" + "fakeSilence1", + "fakeSilence2" + ], + "inhibitedBy": null + }, + { + "name": "", + "cluster": "fakeCluster", + "state": "", + "startsAt": "0001-01-01T00:00:00Z", + "endsAt": "0001-01-01T00:00:00Z", + "source": "", + "silencedBy": [ + "fakeSilence1", + "fakeSilence2" ], "inhibitedBy": null } @@ -155,7 +183,21 @@ func TestDedupSharedMaps(t *testing.T) { "endsAt": "0001-01-01T00:00:00Z", "source": "", "silencedBy": [ - "fakeSilenceID" + "fakeSilence1", + "fakeSilence2" + ], + "inhibitedBy": null + }, + { + "name": "", + "cluster": "fakeCluster", + "state": "", + "startsAt": "0001-01-01T00:00:00Z", + "endsAt": "0001-01-01T00:00:00Z", + "source": "", + "silencedBy": [ + "fakeSilence1", + "fakeSilence2" ], "inhibitedBy": null } @@ -178,7 +220,10 @@ func TestDedupSharedMaps(t *testing.T) { ], "labels": {}, "silences": { - "fakeCluster": "fakeSilenceID" + "fakeCluster": [ + "fakeSilence1", + "fakeSilence2" + ] } } }` From a642dd7aff71a26014f7df605fe0fc7d2f6ac6e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Fri, 15 Mar 2019 11:25:30 +0000 Subject: [PATCH 2/3] fix(ui): add margin around silence fallback --- .../AlertGroup/Silence/__snapshots__/index.test.js.snap | 2 +- ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/__snapshots__/index.test.js.snap b/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/__snapshots__/index.test.js.snap index 9d27be0a8..09d9f902d 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/__snapshots__/index.test.js.snap +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/__snapshots__/index.test.js.snap @@ -2,7 +2,7 @@ exports[` matches snapshot when data is not present in alertStore 1`] = ` " -
+
Silenced by default/4cf5fd82-1edd-4169-99d1-ff8415e72179 diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/index.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/index.js index 7945806e6..694bf85a3 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/index.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Silence/index.js @@ -177,7 +177,7 @@ SilenceDetails.propTypes = { // const FallbackSilenceDesciption = ({ alertmanagerName, silenceID }) => { return ( -
+
Silenced by {alertmanagerName}/{silenceID} From 4a751843fbc6feb867bb819a95a82775437e6f40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Fri, 15 Mar 2019 11:26:46 +0000 Subject: [PATCH 3/3] fix(ui): shared silences is now a list API changed, so UI needs to follow --- .../Grid/AlertGrid/AlertGroup/Alert/index.js | 24 +++++++------- .../AlertGrid/AlertGroup/Alert/index.test.js | 17 +++++++++- .../AlertGrid/AlertGroup/GroupFooter/index.js | 31 ++++++++++--------- .../AlertGroup/GroupFooter/index.test.js | 4 +-- ui/src/Models/API.js | 2 +- 5 files changed, 49 insertions(+), 29 deletions(-) diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js index 04bbdf4fd..72db47a43 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.js @@ -49,22 +49,24 @@ const Alert = observer( BorderClassMap[alert.state] || "border-warning" ]; - let silences = {}; - for (let am of alert.alertmanager) { + const silences = {}; + for (const am of alert.alertmanager) { if (!silences[am.cluster]) { silences[am.cluster] = { alertmanager: am, - silences: [] + silences: [ + ...new Set( + am.silencedBy.filter( + silenceID => + !( + group.shared.silences[am.cluster] && + group.shared.silences[am.cluster].includes(silenceID) + ) + ) + ) + ] }; } - for (let silenceID of am.silencedBy) { - if ( - !silences[am.cluster].silences.includes(silenceID) && - !(group.shared.silences[am.cluster] === silenceID) - ) { - silences[am.cluster].silences.push(silenceID); - } - } } return ( diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.test.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.test.js index 158d53c64..047ed8aba 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.test.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/Alert/index.test.js @@ -105,7 +105,7 @@ describe("", () => { it("renders a silence if alert is silenced", () => { const alert = MockedAlert(); alert.alertmanager[0].silencedBy = ["silence123456789"]; - const group = MockAlertGroup({}, [alert], [], {}, {}); + const group = MockAlertGroup({}, [alert], [], {}, { default: [] }); const tree = MountedAlert(alert, group, false, false); const silence = tree.find("Silence"); expect(silence).toHaveLength(1); @@ -143,6 +143,21 @@ describe("", () => { expect(silence.html()).toMatch(/silence123456789/); }); + it("doesn't render shared silences", () => { + const alert = MockedAlert(); + alert.alertmanager[0].silencedBy = ["silence123456789"]; + const group = MockAlertGroup( + {}, + [alert], + [], + {}, + { default: ["silence123456789"] } + ); + const tree = MountedAlert(alert, group, false, false); + const silence = tree.find("Silence"); + expect(silence).toHaveLength(0); + }); + it("uses BorderClassMap.active when @state=active", () => { const alert = MockedAlert(); alert.state = "active"; diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.js index 1b723e44f..c24feb89a 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.js @@ -67,20 +67,23 @@ const GroupFooter = observer( {Object.keys(group.shared.silences).length === 0 ? null : (
{Object.entries(group.shared.silences).map( - ([cluster, silenceID]) => ( - - a.alertmanager.filter(am => am.cluster === cluster)[0] - )[0] - } - silenceID={silenceID} - afterUpdate={afterUpdate} - /> - ) + ([cluster, silences]) => + silences.map(silenceID => ( + + a.alertmanager.filter( + am => am.cluster === cluster + )[0] + )[0] + } + silenceID={silenceID} + afterUpdate={afterUpdate} + /> + )) )}
)} diff --git a/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.test.js b/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.test.js index 03b2c2446..0849fc0ea 100644 --- a/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.test.js +++ b/ui/src/Components/Grid/AlertGrid/AlertGroup/GroupFooter/index.test.js @@ -80,7 +80,7 @@ describe("", () => { for (const id of Object.keys(group.alerts)) { group.alerts[id].alertmanager[0].silencedBy = ["123456789"]; } - group.shared.silences = { default: "123456789" }; + group.shared.silences = { default: ["123456789"] }; const tree = MountedGroupFooter().find("GroupFooter"); expect(tree.find("Silence")).toHaveLength(1); }); @@ -89,7 +89,7 @@ describe("", () => { for (const id of Object.keys(group.alerts)) { group.alerts[id].alertmanager[0].silencedBy = ["123456789"]; } - group.shared.silences = { default: "123456789" }; + group.shared.silences = { default: ["123456789"] }; alertStore.data.silences = { default: { diff --git a/ui/src/Models/API.js b/ui/src/Models/API.js index c973fc102..7ed7ee72b 100644 --- a/ui/src/Models/API.js +++ b/ui/src/Models/API.js @@ -45,7 +45,7 @@ const APIGroup = PropTypes.exact({ shared: PropTypes.exact({ annotations: PropTypes.arrayOf(Annotation).isRequired, labels: PropTypes.object.isRequired, - silences: PropTypes.object.isRequired + silences: PropTypes.objectOf(PropTypes.arrayOf(PropTypes.string)).isRequired }).isRequired });