IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-02-25 18:55:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

Alerting: Break dependency between state and image packages (#58381)

Browse Source 
* Refactor state and manager to not depend directly on image interface

* Move generic errors to models package

* Move NotAvailableImageService to state as its only references are in state tests

* Move NoopImageService to state package

* Move mock to state package

* Fix linter error

* Fix comment styling

* Fix a couple added references introduced by rebase

* Empty commit to kick build
This commit is contained in:
Alexander Weaver 2022-11-09 15:06:49 -06:00 committed by GitHub
parent bad4f28d0d
commit 2bfdda5b68
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 108 additions and 109 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
pkg/services/ngalert/image/mock.go
View File

@ -1,51 +0,0 @@
// Code generated by MockGen. DO NOT EDIT.
// Source: github.com/grafana/grafana/pkg/services/ngalert/image (interfaces: ImageService)
// Package image is a generated GoMock package.
package image
import (
context "context"
reflect "reflect"
gomock "github.com/golang/mock/gomock"
models "github.com/grafana/grafana/pkg/services/ngalert/models"
)
// MockImageService is a mock of ImageService interface.
type MockImageService struct {
ctrl *gomock.Controller
recorder *MockImageServiceMockRecorder
}
// MockImageServiceMockRecorder is the mock recorder for MockImageService.
type MockImageServiceMockRecorder struct {
mock *MockImageService
}
// NewMockImageService creates a new mock instance.
func NewMockImageService(ctrl *gomock.Controller) *MockImageService {
mock := &MockImageService{ctrl: ctrl}
mock.recorder = &MockImageServiceMockRecorder{mock}
return mock
}
// EXPECT returns an object that allows the caller to indicate expected use.
func (m *MockImageService) EXPECT() *MockImageServiceMockRecorder {
return m.recorder
}
// NewImage mocks base method.
func (m *MockImageService) NewImage(arg0 context.Context, arg1 *models.AlertRule) (*models.Image, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "NewImage", arg0, arg1)
ret0, _ := ret[0].(*models.Image)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// NewImage indicates an expected call of NewImage.
func (mr *MockImageServiceMockRecorder) NewImage(arg0, arg1 interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewImage", reflect.TypeOf((*MockImageService)(nil).NewImage), arg0, arg1)
}

35
pkg/services/ngalert/image/service.go
View File

@ -25,16 +25,6 @@ const (
screenshotTimeout = 10 * time.Second
)
var (
// ErrNoDashboard is returned when the alert rule does not have a Dashboard UID
// in its annotations or the dashboard does not exist.
ErrNoDashboard = errors.New("no dashboard")
// ErrNoPanel is returned when the alert rule does not have a PanelID in its
// annotations.
ErrNoPanel = errors.New("no panel")
)
// DeleteExpiredService is a service to delete expired images.
type DeleteExpiredService struct {
store store.ImageAdminStore
@ -48,7 +38,6 @@ func ProvideDeleteExpiredService(store *store.DBstore) *DeleteExpiredService {
return &DeleteExpiredService{store: store}
}
//go:generate mockgen -destination=mock.go -package=image github.com/grafana/grafana/pkg/services/ngalert/image ImageService
type ImageService interface {
// NewImage returns a new image for the alert instance.
NewImage(ctx context.Context, r *models.AlertRule) (*models.Image, error)
@ -120,16 +109,16 @@ func NewScreenshotImageServiceFromCfg(cfg *setting.Cfg, db *store.DBstore, ds da
//
// The alert rule must be associated with a dashboard panel for a screenshot to be
// taken. If the alert rule does not have a Dashboard UID in its annotations,
// or the dashboard does not exist, an ErrNoDashboard error is returned. If the
// or the dashboard does not exist, an models.ErrNoDashboard error is returned. If the
// alert rule has a Dashboard UID and the dashboard exists, but does not have a
// Panel ID in its annotations then an ErrNoPanel error is returned.
// Panel ID in its annotations then an models.ErrNoPanel error is returned.
func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRule) (*models.Image, error) {
if r.DashboardUID == nil || *r.DashboardUID == "" {
return nil, ErrNoDashboard
return nil, models.ErrNoDashboard
}
if r.PanelID == nil || *r.PanelID == 0 {
return nil, ErrNoPanel
return nil, models.ErrNoPanel
}
opts := screenshot.ScreenshotOptions{
@ -167,7 +156,7 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRu
screenshot, err := s.limiter.Do(ctx, opts, s.screenshots.Take)
if err != nil {
if errors.Is(err, dashboards.ErrDashboardNotFound) {
return nil, ErrNoDashboard
return nil, models.ErrNoDashboard
}
return nil, err
}
@ -204,17 +193,3 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRu
return &image, nil
}
// NotAvailableImageService is a service that returns ErrScreenshotsUnavailable.
type NotAvailableImageService struct{}
func (s *NotAvailableImageService) NewImage(_ context.Context, _ *models.AlertRule) (*models.Image, error) {
return nil, screenshot.ErrScreenshotsUnavailable
}
// NoopImageService is a no-op image service.
type NoopImageService struct{}
func (s *NoopImageService) NewImage(_ context.Context, _ *models.AlertRule) (*models.Image, error) {
return &models.Image{}, nil
}

7
pkg/services/ngalert/models/alert_rule.go
View File

@ -26,6 +26,13 @@ var (
ErrAlertRuleFailedValidation = errors.New("invalid alert rule")
ErrAlertRuleUniqueConstraintViolation = errors.New("a conflicting alert rule is found: rule title under the same organisation and folder should be unique")
ErrQuotaReached = errors.New("quota has been exceeded")
// ErrNoDashboard is returned when the alert rule does not have a Dashboard UID
// in its annotations or the dashboard does not exist.
ErrNoDashboard = errors.New("no dashboard")
// ErrNoPanel is returned when the alert rule does not have a PanelID in its
// annotations.
ErrNoPanel = errors.New("no panel")
)
// swagger:enum NoDataState

5
pkg/services/ngalert/schedule/schedule_unit_test.go
View File

@ -23,7 +23,6 @@ import (
"github.com/grafana/grafana/pkg/expr"
"github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
"github.com/grafana/grafana/pkg/services/ngalert/eval"
"github.com/grafana/grafana/pkg/services/ngalert/image"
"github.com/grafana/grafana/pkg/services/ngalert/metrics"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/ngalert/state"
@ -62,7 +61,7 @@ func TestProcessTicks(t *testing.T) {
Metrics: testMetrics.GetSchedulerMetrics(),
AlertSender: notifier,
}
st := state.NewManager(testMetrics.GetStateMetrics(), nil, nil, &image.NoopImageService{}, mockedClock, &state.FakeHistorian{})
st := state.NewManager(testMetrics.GetStateMetrics(), nil, nil, &state.NoopImageService{}, mockedClock, &state.FakeHistorian{})
appUrl := &url.URL{
Scheme: "http",
@ -682,7 +681,7 @@ func setupScheduler(t *testing.T, rs *fakeRulesStore, is *state.FakeInstanceStor
AlertSender: senderMock,
}
st := state.NewManager(m.GetStateMetrics(), nil, is, &image.NoopImageService{}, mockedClock, &state.FakeHistorian{})
st := state.NewManager(m.GetStateMetrics(), nil, is, &state.NoopImageService{}, mockedClock, &state.FakeHistorian{})
return NewScheduler(schedCfg, appUrl, st)
}

51
pkg/services/ngalert/state/image_mock.go Normal file
View File

@ -0,0 +1,51 @@
// Code generated by MockGen. DO NOT EDIT.
// Source: github.com/grafana/grafana/pkg/services/ngalert/state (interfaces: ImageCapturer)
// Package state is a generated GoMock package.
package state
import (
context "context"
reflect "reflect"
gomock "github.com/golang/mock/gomock"
models "github.com/grafana/grafana/pkg/services/ngalert/models"
)
// MockImageCapturer is a mock of ImageCapturer interface.
type MockImageCapturer struct {
ctrl *gomock.Controller
recorder *MockImageCapturerMockRecorder
}
// MockImageCapturerMockRecorder is the mock recorder for MockImageCapturer.
type MockImageCapturerMockRecorder struct {
mock *MockImageCapturer
}
// NewMockImageCapturer creates a new mock instance.
func NewMockImageCapturer(ctrl *gomock.Controller) *MockImageCapturer {
mock := &MockImageCapturer{ctrl: ctrl}
mock.recorder = &MockImageCapturerMockRecorder{mock}
return mock
}
// EXPECT returns an object that allows the caller to indicate expected use.
func (m *MockImageCapturer) EXPECT() *MockImageCapturerMockRecorder {
return m.recorder
}
// NewImage mocks base method.
func (m *MockImageCapturer) NewImage(arg0 context.Context, arg1 *models.AlertRule) (*models.Image, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "NewImage", arg0, arg1)
ret0, _ := ret[0].(*models.Image)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// NewImage indicates an expected call of NewImage.
func (mr *MockImageCapturerMockRecorder) NewImage(arg0, arg1 interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewImage", reflect.TypeOf((*MockImageCapturer)(nil).NewImage), arg0, arg1)
}

11
pkg/services/ngalert/state/manager.go
View File

@ -10,7 +10,6 @@ import (
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/ngalert/eval"
"github.com/grafana/grafana/pkg/services/ngalert/image"
"github.com/grafana/grafana/pkg/services/ngalert/metrics"
ngModels "github.com/grafana/grafana/pkg/services/ngalert/models"
)
@ -35,19 +34,19 @@ type Manager struct {
ResendDelay time.Duration
instanceStore InstanceStore
imageService image.ImageService
images ImageCapturer
historian Historian
externalURL *url.URL
}
func NewManager(metrics *metrics.State, externalURL *url.URL, instanceStore InstanceStore, imageService image.ImageService, clock clock.Clock, historian Historian) *Manager {
func NewManager(metrics *metrics.State, externalURL *url.URL, instanceStore InstanceStore, images ImageCapturer, clock clock.Clock, historian Historian) *Manager {
return &Manager{
cache: newCache(),
ResendDelay: ResendDelay, // TODO: make this configurable
log: log.New("ngalert.state.manager"),
metrics: metrics,
instanceStore: instanceStore,
imageService: imageService,
images: images,
historian: historian,
clock: clock,
externalURL: externalURL,
@ -244,7 +243,7 @@ func (st *Manager) setNextState(ctx context.Context, alertRule *ngModels.AlertRu
currentState.Resolved = oldState == eval.Alerting && currentState.State == eval.Normal
if shouldTakeImage(currentState.State, oldState, currentState.Image, currentState.Resolved) {
image, err := takeImage(ctx, st.imageService, alertRule)
image, err := takeImage(ctx, st.images, alertRule)
if err != nil {
logger.Warn("Failed to take an image",
"dashboard", alertRule.DashboardUID,
@ -371,7 +370,7 @@ func (st *Manager) staleResultsHandler(ctx context.Context, logger log.Logger, r
// If there is no resolved image for this rule then take one
if resolvedImage == nil {
image, err := takeImage(ctx, st.imageService, r)
image, err := takeImage(ctx, st.images, r)
if err != nil {
logger.Warn("Failed to take an image",
"dashboard", r.DashboardUID,

13
pkg/services/ngalert/state/manager_test.go
View File

@ -22,7 +22,6 @@ import (
"github.com/grafana/grafana/pkg/services/annotations/annotationstest"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/ngalert/eval"
"github.com/grafana/grafana/pkg/services/ngalert/image"
"github.com/grafana/grafana/pkg/services/ngalert/metrics"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/ngalert/state"
@ -103,7 +102,7 @@ func TestWarmStateCache(t *testing.T) {
Labels: labels,
}
_ = dbstore.SaveAlertInstances(ctx, instance2)
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &image.NoopImageService{}, clock.NewMock(), &state.FakeHistorian{})
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &state.NoopImageService{}, clock.NewMock(), &state.FakeHistorian{})
st.Warm(ctx, dbstore)
t.Run("instance cache has expected entries", func(t *testing.T) {
@ -127,7 +126,7 @@ func TestDashboardAnnotations(t *testing.T) {
fakeAnnoRepo := annotationstest.NewFakeAnnotationsRepo()
hist := historian.NewAnnotationHistorian(fakeAnnoRepo, &dashboards.FakeDashboardService{})
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &image.NoopImageService{}, clock.New(), hist)
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &state.NoopImageService{}, clock.New(), hist)
const mainOrgID int64 = 1
@ -2108,7 +2107,7 @@ func TestProcessEvalResults(t *testing.T) {
for _, tc := range testCases {
fakeAnnoRepo := annotationstest.NewFakeAnnotationsRepo()
hist := historian.NewAnnotationHistorian(fakeAnnoRepo, &dashboards.FakeDashboardService{})
st := state.NewManager(testMetrics.GetStateMetrics(), nil, &state.FakeInstanceStore{}, &image.NotAvailableImageService{}, clock.New(), hist)
st := state.NewManager(testMetrics.GetStateMetrics(), nil, &state.FakeInstanceStore{}, &state.NotAvailableImageService{}, clock.New(), hist)
t.Run(tc.desc, func(t *testing.T) {
for _, res := range tc.evalResults {
_ = st.ProcessEvalResults(context.Background(), evaluationTime, tc.alertRule, res, data.Labels{
@ -2135,7 +2134,7 @@ func TestProcessEvalResults(t *testing.T) {
t.Run("should save state to database", func(t *testing.T) {
instanceStore := &state.FakeInstanceStore{}
clk := clock.New()
st := state.NewManager(testMetrics.GetStateMetrics(), nil, instanceStore, &image.NotAvailableImageService{}, clk, &state.FakeHistorian{})
st := state.NewManager(testMetrics.GetStateMetrics(), nil, instanceStore, &state.NotAvailableImageService{}, clk, &state.FakeHistorian{})
rule := models.AlertRuleGen()()
var results = eval.GenerateResults(rand.Intn(4)+1, eval.ResultGen(eval.WithEvaluatedAt(clk.Now())))
@ -2264,7 +2263,7 @@ func TestStaleResultsHandler(t *testing.T) {
for _, tc := range testCases {
ctx := context.Background()
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &image.NoopImageService{}, clock.New(), &state.FakeHistorian{})
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &state.NoopImageService{}, clock.New(), &state.FakeHistorian{})
st.Warm(ctx, dbstore)
existingStatesForRule := st.GetStatesForRuleUID(rule.OrgID, rule.UID)
@ -2326,7 +2325,7 @@ func TestStaleResults(t *testing.T) {
clk := clock.NewMock()
clk.Set(time.Now())
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &image.NoopImageService{}, clk, &state.FakeHistorian{})
st := state.NewManager(testMetrics.GetStateMetrics(), nil, dbstore, &state.NoopImageService{}, clk, &state.FakeHistorian{})
orgID := rand.Int63()
rule := tests.CreateTestAlertRule(t, ctx, dbstore, 10, orgID)

7
pkg/services/ngalert/state/persist.go
View File

@ -25,3 +25,10 @@ type Historian interface {
// RecordStates writes a number of state transitions for a given rule to state history.
RecordStates(ctx context.Context, rule *models.AlertRule, states []StateTransition)
}
// ImageCapturer captures images.
//
//go:generate mockgen -destination=image_mock.go -package=state github.com/grafana/grafana/pkg/services/ngalert/state ImageCapturer
type ImageCapturer interface {
NewImage(ctx context.Context, r *models.AlertRule) (*models.Image, error)
}

7
pkg/services/ngalert/state/state.go
View File

@ -12,7 +12,6 @@ import (
"github.com/grafana/grafana/pkg/expr"
"github.com/grafana/grafana/pkg/services/ngalert/eval"
"github.com/grafana/grafana/pkg/services/ngalert/image"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/screenshot"
)
@ -334,12 +333,12 @@ func shouldTakeImage(state, previousState eval.State, previousImage *models.Imag
// takeImage takes an image for the alert rule. It returns nil if screenshots are disabled or
// the rule is not associated with a dashboard panel.
func takeImage(ctx context.Context, s image.ImageService, r *models.AlertRule) (*models.Image, error) {
func takeImage(ctx context.Context, s ImageCapturer, r *models.AlertRule) (*models.Image, error) {
img, err := s.NewImage(ctx, r)
if err != nil {
if errors.Is(err, screenshot.ErrScreenshotsUnavailable) ||
errors.Is(err, image.ErrNoDashboard) ||
errors.Is(err, image.ErrNoPanel) {
errors.Is(err, models.ErrNoDashboard) ||
errors.Is(err, models.ErrNoPanel) {
return nil, nil
}
return nil, err

15
pkg/services/ngalert/state/state_test.go
View File

@ -10,7 +10,6 @@ import (
"github.com/golang/mock/gomock"
"github.com/grafana/grafana/pkg/services/ngalert/eval"
"github.com/grafana/grafana/pkg/services/ngalert/image"
ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/screenshot"
"github.com/stretchr/testify/assert"
@ -358,9 +357,9 @@ func TestTakeImage(t *testing.T) {
ctx := context.Background()
r := ngmodels.AlertRule{}
s := image.NewMockImageService(ctrl)
s := NewMockImageCapturer(ctrl)
s.EXPECT().NewImage(ctx, &r).Return(nil, image.ErrNoDashboard)
s.EXPECT().NewImage(ctx, &r).Return(nil, ngmodels.ErrNoDashboard)
image, err := takeImage(ctx, s, &r)
assert.NoError(t, err)
assert.Nil(t, image)
@ -372,9 +371,9 @@ func TestTakeImage(t *testing.T) {
ctx := context.Background()
r := ngmodels.AlertRule{DashboardUID: ptr.String("foo")}
s := image.NewMockImageService(ctrl)
s := NewMockImageCapturer(ctrl)
s.EXPECT().NewImage(ctx, &r).Return(nil, image.ErrNoPanel)
s.EXPECT().NewImage(ctx, &r).Return(nil, ngmodels.ErrNoPanel)
image, err := takeImage(ctx, s, &r)
assert.NoError(t, err)
assert.Nil(t, image)
@ -386,7 +385,7 @@ func TestTakeImage(t *testing.T) {
ctx := context.Background()
r := ngmodels.AlertRule{DashboardUID: ptr.String("foo"), PanelID: ptr.Int64(1)}
s := image.NewMockImageService(ctrl)
s := NewMockImageCapturer(ctrl)
s.EXPECT().NewImage(ctx, &r).Return(nil, screenshot.ErrScreenshotsUnavailable)
image, err := takeImage(ctx, s, &r)
@ -400,7 +399,7 @@ func TestTakeImage(t *testing.T) {
ctx := context.Background()
r := ngmodels.AlertRule{DashboardUID: ptr.String("foo"), PanelID: ptr.Int64(1)}
s := image.NewMockImageService(ctrl)
s := NewMockImageCapturer(ctrl)
s.EXPECT().NewImage(ctx, &r).Return(nil, errors.New("unknown error"))
image, err := takeImage(ctx, s, &r)
@ -414,7 +413,7 @@ func TestTakeImage(t *testing.T) {
ctx := context.Background()
r := ngmodels.AlertRule{DashboardUID: ptr.String("foo"), PanelID: ptr.Int64(1)}
s := image.NewMockImageService(ctrl)
s := NewMockImageCapturer(ctrl)
s.EXPECT().NewImage(ctx, &r).Return(&ngmodels.Image{Path: "foo.png"}, nil)
image, err := takeImage(ctx, s, &r)

15
pkg/services/ngalert/state/testing.go
View File

@ -5,6 +5,7 @@ import (
"sync"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/screenshot"
)
var _ InstanceStore = &FakeInstanceStore{}
@ -50,3 +51,17 @@ type FakeHistorian struct{}
func (f *FakeHistorian) RecordStates(ctx context.Context, rule *models.AlertRule, states []StateTransition) {
}
// NotAvailableImageService is a service that returns ErrScreenshotsUnavailable.
type NotAvailableImageService struct{}
func (s *NotAvailableImageService) NewImage(_ context.Context, _ *models.AlertRule) (*models.Image, error) {
return nil, screenshot.ErrScreenshotsUnavailable
}
// NoopImageService is a no-op image service.
type NoopImageService struct{}
func (s *NoopImageService) NewImage(_ context.Context, _ *models.AlertRule) (*models.Image, error) {
return &models.Image{}, nil
}