From ec4698fb960f56645e7af08c65abd4e05531d819 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Fri, 12 Oct 2018 22:53:28 -0700 Subject: [PATCH] alerting: propagate alert validation issues to the user instead of just 'invalid alert data' message --- pkg/api/dashboard.go | 5 +++-- pkg/api/dashboard_test.go | 3 ++- pkg/models/dashboards.go | 1 - pkg/services/alerting/extractor.go | 11 +++++------ pkg/services/alerting/extractor_test.go | 2 +- pkg/services/alerting/rule.go | 8 ++++---- pkg/services/dashboards/dashboard_service.go | 8 ++++++-- pkg/services/dashboards/dashboard_service_test.go | 4 ++-- 8 files changed, 23 insertions(+), 19 deletions(-) diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index d65598f6e5e..02248334b9c 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -6,6 +6,7 @@ import ( "os" "path" + "github.com/grafana/grafana/pkg/services/alerting" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/api/dtos" @@ -251,8 +252,8 @@ func PostDashboard(c *m.ReqContext, cmd m.SaveDashboardCommand) Response { return Error(403, err.Error(), err) } - if err == m.ErrDashboardContainsInvalidAlertData { - return Error(500, "Invalid alert data. Cannot save dashboard", err) + if validationErr, ok := err.(alerting.ValidationError); ok { + return Error(422, validationErr.Error(), nil) } if err != nil { diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index 283a9b5f12c..2726623c242 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/alerting" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/setting" @@ -725,7 +726,7 @@ func TestDashboardApiEndpoint(t *testing.T) { {SaveError: m.ErrDashboardVersionMismatch, ExpectedStatusCode: 412}, {SaveError: m.ErrDashboardTitleEmpty, ExpectedStatusCode: 400}, {SaveError: m.ErrDashboardFolderCannotHaveParent, ExpectedStatusCode: 400}, - {SaveError: m.ErrDashboardContainsInvalidAlertData, ExpectedStatusCode: 500}, + {SaveError: alerting.ValidationError{Reason: "Mu"}, ExpectedStatusCode: 422}, {SaveError: m.ErrDashboardFailedToUpdateAlertData, ExpectedStatusCode: 500}, {SaveError: m.ErrDashboardFailedGenerateUniqueUid, ExpectedStatusCode: 500}, {SaveError: m.ErrDashboardTypeMismatch, ExpectedStatusCode: 400}, diff --git a/pkg/models/dashboards.go b/pkg/models/dashboards.go index 4b84d840113..e8aebb1d1f4 100644 --- a/pkg/models/dashboards.go +++ b/pkg/models/dashboards.go @@ -21,7 +21,6 @@ var ( ErrDashboardVersionMismatch = errors.New("The dashboard has been changed by someone else") ErrDashboardTitleEmpty = errors.New("Dashboard title cannot be empty") ErrDashboardFolderCannotHaveParent = errors.New("A Dashboard Folder cannot be added to another folder") - ErrDashboardContainsInvalidAlertData = errors.New("Invalid alert data. Cannot save dashboard") ErrDashboardFailedToUpdateAlertData = errors.New("Failed to save alert data") ErrDashboardsWithSameSlugExists = errors.New("Multiple dashboards with the same slug exists") ErrDashboardFailedGenerateUniqueUid = errors.New("Failed to generate unique dashboard id") diff --git a/pkg/services/alerting/extractor.go b/pkg/services/alerting/extractor.go index 229092e217b..edfab2dedee 100644 --- a/pkg/services/alerting/extractor.go +++ b/pkg/services/alerting/extractor.go @@ -82,8 +82,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, if collapsed && collapsedJSON.MustBool() { // extract alerts from sub panels for collapsed panels - alertSlice, err := e.getAlertFromPanels(panel, - validateAlertFunc) + alertSlice, err := e.getAlertFromPanels(panel, validateAlertFunc) if err != nil { return nil, err } @@ -100,7 +99,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, panelID, err := panel.Get("id").Int64() if err != nil { - return nil, fmt.Errorf("panel id is required. err %v", err) + return nil, ValidationError{Reason: "A numeric panel id property is missing"} } // backward compatibility check, can be removed later @@ -146,7 +145,8 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, datasource, err := e.lookupDatasourceID(dsName) if err != nil { - return nil, err + e.log.Debug("Error looking up datasource", "error", err) + return nil, ValidationError{Reason: fmt.Sprintf("Data source used by alert rule not found, alertName=%v, datasource=%s", alert.Name, dsName)} } jsonQuery.SetPath([]string{"datasourceId"}, datasource.Id) @@ -167,8 +167,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, } 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 + return nil, ValidationError{Reason: fmt.Sprintf("Panel id is not correct, alertName=%v, panelId=%v", alert.Name, alert.PanelId)} } alerts = append(alerts, alert) diff --git a/pkg/services/alerting/extractor_test.go b/pkg/services/alerting/extractor_test.go index c7212e48174..e2dc01a1181 100644 --- a/pkg/services/alerting/extractor_test.go +++ b/pkg/services/alerting/extractor_test.go @@ -258,7 +258,7 @@ func TestAlertRuleExtraction(t *testing.T) { Convey("Should fail on save", func() { _, err := extractor.GetAlerts() - So(err, ShouldEqual, m.ErrDashboardContainsInvalidAlertData) + So(err.Error(), ShouldEqual, "Alert validation error: Panel id is not correct, alertName=Influxdb, panelId=1") }) }) }) diff --git a/pkg/services/alerting/rule.go b/pkg/services/alerting/rule.go index d13924c2a17..f820fae5ae4 100644 --- a/pkg/services/alerting/rule.go +++ b/pkg/services/alerting/rule.go @@ -36,13 +36,13 @@ type ValidationError struct { } func (e ValidationError) Error() string { - extraInfo := "" + extraInfo := e.Reason if e.Alertid != 0 { extraInfo = fmt.Sprintf("%s AlertId: %v", extraInfo, e.Alertid) } if e.PanelId != 0 { - extraInfo = fmt.Sprintf("%s PanelId: %v ", extraInfo, e.PanelId) + extraInfo = fmt.Sprintf("%s PanelId: %v", extraInfo, e.PanelId) } if e.DashboardId != 0 { @@ -50,10 +50,10 @@ func (e ValidationError) Error() string { } if e.Err != nil { - return fmt.Sprintf("%s %s%s", e.Err.Error(), e.Reason, extraInfo) + return fmt.Sprintf("Alert validation error: %s%s", e.Err.Error(), extraInfo) } - return fmt.Sprintf("Failed to extract alert.Reason: %s %s", e.Reason, extraInfo) + return fmt.Sprintf("Alert validation error: %s", extraInfo) } var ( diff --git a/pkg/services/dashboards/dashboard_service.go b/pkg/services/dashboards/dashboard_service.go index 278421e6be7..8eb7f4a6e72 100644 --- a/pkg/services/dashboards/dashboard_service.go +++ b/pkg/services/dashboards/dashboard_service.go @@ -5,6 +5,7 @@ import ( "time" "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/util" @@ -25,7 +26,9 @@ type DashboardProvisioningService interface { // NewService factory for creating a new dashboard service var NewService = func() DashboardService { - return &dashboardServiceImpl{} + return &dashboardServiceImpl{ + log: log.New("dashboard-service"), + } } // NewProvisioningService factory for creating a new dashboard provisioning service @@ -45,6 +48,7 @@ type SaveDashboardDTO struct { type dashboardServiceImpl struct { orgId int64 user *models.SignedInUser + log log.Logger } func (dr *dashboardServiceImpl) GetProvisionedDashboardData(name string) ([]*models.DashboardProvisioning, error) { @@ -89,7 +93,7 @@ func (dr *dashboardServiceImpl) buildSaveDashboardCommand(dto *SaveDashboardDTO, } if err := bus.Dispatch(&validateAlertsCmd); err != nil { - return nil, models.ErrDashboardContainsInvalidAlertData + return nil, err } } diff --git a/pkg/services/dashboards/dashboard_service_test.go b/pkg/services/dashboards/dashboard_service_test.go index f9d487f625c..b8300a5af8d 100644 --- a/pkg/services/dashboards/dashboard_service_test.go +++ b/pkg/services/dashboards/dashboard_service_test.go @@ -117,12 +117,12 @@ func TestDashboardService(t *testing.T) { }) bus.AddHandler("test", func(cmd *models.ValidateDashboardAlertsCommand) error { - return errors.New("error") + return errors.New("Alert validation error") }) dto.Dashboard = models.NewDashboard("Dash") _, err := service.SaveDashboard(dto) - So(err, ShouldEqual, models.ErrDashboardContainsInvalidAlertData) + So(err.Error(), ShouldEqual, "Alert validation error") }) })