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 + }