From 68833fa97807989ed34932ab040d6578fbcdfb66 Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Wed, 28 Mar 2018 11:31:33 +0200 Subject: [PATCH] dashboard: allow alerts to be saved for new/provisioned dashboards This changes the logic for the alert validation in the extractor. Validation is executed twice, once before attempting to save the alerts and once after saving the dashboard while saving the alerts to the db. The first validation will not require that the alert has a dashboard id which allows new dashboards with alerts to be saved. Fixes #11247 --- pkg/services/alerting/commands.go | 19 +- pkg/services/alerting/extractor.go | 97 +++--- pkg/services/alerting/extractor_test.go | 21 ++ .../alerting/test-data/dash-without-id.json | 281 ++++++++++++++++++ .../alerting/test-data/influxdb-alert.json | 2 +- 5 files changed, 366 insertions(+), 54 deletions(-) create mode 100644 pkg/services/alerting/test-data/dash-without-id.json diff --git a/pkg/services/alerting/commands.go b/pkg/services/alerting/commands.go index 2c145614751..02186d697ee 100644 --- a/pkg/services/alerting/commands.go +++ b/pkg/services/alerting/commands.go @@ -13,11 +13,7 @@ func init() { func validateDashboardAlerts(cmd *m.ValidateDashboardAlertsCommand) error { extractor := NewDashAlertExtractor(cmd.Dashboard, cmd.OrgId) - if _, err := extractor.GetAlerts(); err != nil { - return err - } - - return nil + return extractor.ValidateAlerts() } func updateDashboardAlerts(cmd *m.UpdateDashboardAlertsCommand) error { @@ -29,15 +25,12 @@ func updateDashboardAlerts(cmd *m.UpdateDashboardAlertsCommand) error { extractor := NewDashAlertExtractor(cmd.Dashboard, cmd.OrgId) - if alerts, err := extractor.GetAlerts(); err != nil { - return err - } else { - saveAlerts.Alerts = alerts - } - - if err := bus.Dispatch(&saveAlerts); err != nil { + alerts, err := extractor.GetAlerts() + if err != nil { return err } - return nil + saveAlerts.Alerts = alerts + + return bus.Dispatch(&saveAlerts) } diff --git a/pkg/services/alerting/extractor.go b/pkg/services/alerting/extractor.go index 2ae26c1a382..edd872b8fce 100644 --- a/pkg/services/alerting/extractor.go +++ b/pkg/services/alerting/extractor.go @@ -11,76 +11,78 @@ import ( m "github.com/grafana/grafana/pkg/models" ) +// DashAlertExtractor extracts alerts from the dashboard json type DashAlertExtractor struct { Dash *m.Dashboard - OrgId int64 + OrgID int64 log log.Logger } -func NewDashAlertExtractor(dash *m.Dashboard, orgId int64) *DashAlertExtractor { +// NewDashAlertExtractor returns a new DashAlertExtractor +func NewDashAlertExtractor(dash *m.Dashboard, orgID int64) *DashAlertExtractor { return &DashAlertExtractor{ Dash: dash, - OrgId: orgId, + OrgID: orgID, log: log.New("alerting.extractor"), } } -func (e *DashAlertExtractor) lookupDatasourceId(dsName string) (*m.DataSource, error) { +func (e *DashAlertExtractor) lookupDatasourceID(dsName string) (*m.DataSource, error) { if dsName == "" { - query := &m.GetDataSourcesQuery{OrgId: e.OrgId} + query := &m.GetDataSourcesQuery{OrgId: e.OrgID} if err := bus.Dispatch(query); err != nil { return nil, err - } else { - for _, ds := range query.Result { - if ds.IsDefault { - return ds, nil - } + } + + for _, ds := range query.Result { + if ds.IsDefault { + return ds, nil } } } else { - query := &m.GetDataSourceByNameQuery{Name: dsName, OrgId: e.OrgId} + query := &m.GetDataSourceByNameQuery{Name: dsName, OrgId: e.OrgID} if err := bus.Dispatch(query); err != nil { return nil, err - } else { - return query.Result, nil } + + return query.Result, nil } return nil, errors.New("Could not find datasource id for " + dsName) } -func findPanelQueryByRefId(panel *simplejson.Json, refId string) *simplejson.Json { +func findPanelQueryByRefID(panel *simplejson.Json, refID string) *simplejson.Json { for _, targetsObj := range panel.Get("targets").MustArray() { target := simplejson.NewFromAny(targetsObj) - if target.Get("refId").MustString() == refId { + if target.Get("refId").MustString() == refID { return target } } return nil } -func copyJson(in *simplejson.Json) (*simplejson.Json, error) { - rawJson, err := in.MarshalJSON() +func copyJSON(in *simplejson.Json) (*simplejson.Json, error) { + rawJSON, err := in.MarshalJSON() if err != nil { return nil, err } - return simplejson.NewJson(rawJson) + return simplejson.NewJson(rawJSON) } -func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json) ([]*m.Alert, error) { +func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, validateAlertFunc func(*m.Alert) bool) ([]*m.Alert, error) { alerts := make([]*m.Alert, 0) for _, panelObj := range jsonWithPanels.Get("panels").MustArray() { panel := simplejson.NewFromAny(panelObj) - collapsedJson, collapsed := panel.CheckGet("collapsed") + collapsedJSON, collapsed := panel.CheckGet("collapsed") // check if the panel is collapsed - if collapsed && collapsedJson.MustBool() { + if collapsed && collapsedJSON.MustBool() { // extract alerts from sub panels for collapsed panels - als, err := e.GetAlertFromPanels(panel) + als, err := e.getAlertFromPanels(panel, validateAlertFunc) if err != nil { return nil, err } @@ -95,7 +97,7 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json) continue } - panelId, err := panel.Get("id").Int64() + panelID, err := panel.Get("id").Int64() if err != nil { return nil, fmt.Errorf("panel id is required. err %v", err) } @@ -113,8 +115,8 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json) alert := &m.Alert{ DashboardId: e.Dash.Id, - OrgId: e.OrgId, - PanelId: panelId, + OrgId: e.OrgID, + PanelId: panelID, Id: jsonAlert.Get("id").MustInt64(), Name: jsonAlert.Get("name").MustString(), Handler: jsonAlert.Get("handler").MustInt64(), @@ -126,11 +128,11 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json) jsonCondition := simplejson.NewFromAny(condition) jsonQuery := jsonCondition.Get("query") - queryRefId := jsonQuery.Get("params").MustArray()[0].(string) - panelQuery := findPanelQueryByRefId(panel, queryRefId) + queryRefID := jsonQuery.Get("params").MustArray()[0].(string) + panelQuery := findPanelQueryByRefID(panel, queryRefID) if panelQuery == nil { - reason := fmt.Sprintf("Alert on PanelId: %v refers to query(%s) that cannot be found", alert.PanelId, queryRefId) + reason := fmt.Sprintf("Alert on PanelId: %v refers to query(%s) that cannot be found", alert.PanelId, queryRefID) return nil, ValidationError{Reason: reason} } @@ -141,12 +143,13 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json) dsName = panel.Get("datasource").MustString() } - if datasource, err := e.lookupDatasourceId(dsName); err != nil { + datasource, err := e.lookupDatasourceID(dsName) + if err != nil { return nil, err - } else { - jsonQuery.SetPath([]string{"datasourceId"}, datasource.Id) } + jsonQuery.SetPath([]string{"datasourceId"}, datasource.Id) + if interval, err := panel.Get("interval").String(); err == nil { panelQuery.Set("interval", interval) } @@ -162,21 +165,28 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json) return nil, err } - if alert.ValidToSave() { - alerts = append(alerts, alert) - } else { + if !validateAlertFunc(alert) { e.log.Debug("Invalid Alert Data. Dashboard, Org or Panel ID is not correct", "alertName", alert.Name, "panelId", alert.PanelId) return nil, m.ErrDashboardContainsInvalidAlertData } + + alerts = append(alerts, alert) } return alerts, nil } -func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) { - e.log.Debug("GetAlerts") +func validateAlertRule(alert *m.Alert) bool { + return alert.ValidToSave() +} - dashboardJson, err := copyJson(e.Dash.Data) +// GetAlerts extracts alerts from the dashboard json and does full validation on the alert json data +func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) { + return e.extractAlerts(validateAlertRule) +} + +func (e *DashAlertExtractor) extractAlerts(validateFunc func(alert *m.Alert) bool) ([]*m.Alert, error) { + dashboardJSON, err := copyJSON(e.Dash.Data) if err != nil { return nil, err } @@ -185,11 +195,11 @@ func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) { // We extract alerts from rows to be backwards compatible // with the old dashboard json model. - rows := dashboardJson.Get("rows").MustArray() + rows := dashboardJSON.Get("rows").MustArray() if len(rows) > 0 { for _, rowObj := range rows { row := simplejson.NewFromAny(rowObj) - a, err := e.GetAlertFromPanels(row) + a, err := e.getAlertFromPanels(row, validateFunc) if err != nil { return nil, err } @@ -197,7 +207,7 @@ func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) { alerts = append(alerts, a...) } } else { - a, err := e.GetAlertFromPanels(dashboardJson) + a, err := e.getAlertFromPanels(dashboardJSON, validateFunc) if err != nil { return nil, err } @@ -208,3 +218,10 @@ func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) { e.log.Debug("Extracted alerts from dashboard", "alertCount", len(alerts)) return alerts, nil } + +// ValidateAlerts validates alerts in the dashboard json but does not require a valid dashboard id +// in the first validation pass +func (e *DashAlertExtractor) ValidateAlerts() error { + _, err := e.extractAlerts(func(alert *m.Alert) bool { return alert.OrgId != 0 && alert.PanelId != 0 }) + return err +} diff --git a/pkg/services/alerting/extractor_test.go b/pkg/services/alerting/extractor_test.go index 3bda6c771fb..861e9b9cbfc 100644 --- a/pkg/services/alerting/extractor_test.go +++ b/pkg/services/alerting/extractor_test.go @@ -240,5 +240,26 @@ func TestAlertRuleExtraction(t *testing.T) { So(len(alerts), ShouldEqual, 4) }) }) + + Convey("Parse and validate dashboard without id and containing an alert", func() { + json, err := ioutil.ReadFile("./test-data/dash-without-id.json") + So(err, ShouldBeNil) + + dashJSON, err := simplejson.NewJson(json) + So(err, ShouldBeNil) + dash := m.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1) + + err = extractor.ValidateAlerts() + + Convey("Should validate without error", func() { + So(err, ShouldBeNil) + }) + + Convey("Should fail on save", func() { + _, err := extractor.GetAlerts() + So(err, ShouldEqual, m.ErrDashboardContainsInvalidAlertData) + }) + }) }) } diff --git a/pkg/services/alerting/test-data/dash-without-id.json b/pkg/services/alerting/test-data/dash-without-id.json new file mode 100644 index 00000000000..e0a212695d8 --- /dev/null +++ b/pkg/services/alerting/test-data/dash-without-id.json @@ -0,0 +1,281 @@ +{ + "title": "Influxdb", + "tags": [ + "apa" + ], + "style": "dark", + "timezone": "browser", + "editable": true, + "hideControls": false, + "sharedCrosshair": false, + "rows": [ + { + "collapse": false, + "editable": true, + "height": "450px", + "panels": [ + { + "alert": { + "conditions": [ + { + "evaluator": { + "params": [ + 10 + ], + "type": "gt" + }, + "query": { + "params": [ + "B", + "5m", + "now" + ] + }, + "reducer": { + "params": [], + "type": "avg" + }, + "type": "query" + } + ], + "frequency": "3s", + "handler": 1, + "name": "Influxdb", + "noDataState": "no_data", + "notifications": [ + { + "id": 6 + } + ] + }, + "alerting": {}, + "aliasColors": { + "logins.count.count": "#890F02" + }, + "bars": false, + "datasource": "InfluxDB", + "editable": true, + "error": false, + "fill": 1, + "grid": {}, + "id": 1, + "interval": ">10s", + "isNew": true, + "legend": { + "avg": false, + "current": false, + "max": false, + "min": false, + "show": true, + "total": false, + "values": false + }, + "lines": true, + "linewidth": 2, + "links": [], + "nullPointMode": "connected", + "percentage": false, + "pointradius": 5, + "points": false, + "renderer": "flot", + "seriesOverrides": [], + "span": 10, + "stack": false, + "steppedLine": false, + "targets": [ + { + "groupBy": [ + { + "params": [ + "$interval" + ], + "type": "time" + }, + { + "params": [ + "datacenter" + ], + "type": "tag" + }, + { + "params": [ + "none" + ], + "type": "fill" + } + ], + "hide": false, + "measurement": "logins.count", + "policy": "default", + "query": "SELECT 8 * count(\"value\") FROM \"logins.count\" WHERE $timeFilter GROUP BY time($interval), \"datacenter\" fill(none)", + "rawQuery": true, + "refId": "B", + "resultFormat": "time_series", + "select": [ + [ + { + "params": [ + "value" + ], + "type": "field" + }, + { + "params": [], + "type": "count" + } + ] + ], + "tags": [] + }, + { + "groupBy": [ + { + "params": [ + "$interval" + ], + "type": "time" + }, + { + "params": [ + "null" + ], + "type": "fill" + } + ], + "hide": true, + "measurement": "cpu", + "policy": "default", + "refId": "A", + "resultFormat": "time_series", + "select": [ + [ + { + "params": [ + "value" + ], + "type": "field" + }, + { + "params": [], + "type": "mean" + } + ], + [ + { + "params": [ + "value" + ], + "type": "field" + }, + { + "params": [], + "type": "sum" + } + ] + ], + "tags": [] + } + ], + "thresholds": [ + { + "colorMode": "critical", + "fill": true, + "line": true, + "op": "gt", + "value": 10 + } + ], + "timeFrom": null, + "timeShift": null, + "title": "Panel Title", + "tooltip": { + "msResolution": false, + "ordering": "alphabetical", + "shared": true, + "sort": 0, + "value_type": "cumulative" + }, + "type": "graph", + "xaxis": { + "mode": "time", + "name": null, + "show": true, + "values": [] + }, + "yaxes": [ + { + "format": "short", + "logBase": 1, + "max": null, + "min": null, + "show": true + }, + { + "format": "short", + "logBase": 1, + "max": null, + "min": null, + "show": true + } + ] + }, + { + "editable": true, + "error": false, + "id": 2, + "isNew": true, + "limit": 10, + "links": [], + "show": "current", + "span": 2, + "stateFilter": [ + "alerting" + ], + "title": "Alert status", + "type": "alertlist" + } + ], + "title": "Row" + } + ], + "time": { + "from": "now-5m", + "to": "now" + }, + "timepicker": { + "now": true, + "refresh_intervals": [ + "5s", + "10s", + "30s", + "1m", + "5m", + "15m", + "30m", + "1h", + "2h", + "1d" + ], + "time_options": [ + "5m", + "15m", + "1h", + "6h", + "12h", + "24h", + "2d", + "7d", + "30d" + ] + }, + "templating": { + "list": [] + }, + "annotations": { + "list": [] + }, + "schemaVersion": 13, + "version": 120, + "links": [], + "gnetId": null + } diff --git a/pkg/services/alerting/test-data/influxdb-alert.json b/pkg/services/alerting/test-data/influxdb-alert.json index 79ca355c5a1..fd6feb31a47 100644 --- a/pkg/services/alerting/test-data/influxdb-alert.json +++ b/pkg/services/alerting/test-data/influxdb-alert.json @@ -279,4 +279,4 @@ "version": 120, "links": [], "gnetId": null - } \ No newline at end of file + }