Alerting: State Manager takes screenshots. #49338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JohnnyQQQQ
reviewed
May 20, 2022
joeblubaugh
commented
May 22, 2022
| // It's not an error if screenshots are disabled, or our rule isn't allowed to generate screenshots. | ||
| return nil | ||
| } else if err != nil { | ||
| st.log.Error("failed to create image", "error", err) |
There was a problem hiding this comment.
But there is double logging here, which we don't need. I'm going to remove this one and keep the more detailed one in the outer function.
joeblubaugh
commented
May 22, 2022
| @@ -1770,7 +1770,7 @@ func TestProcessEvalResults(t *testing.T) { | |||
|
|
|||
| for _, tc := range testCases { | |||
| ss := mockstore.NewSQLStoreMock() | |||
| st := state.NewManager(log.New("test_state_manager"), testMetrics.GetStateMetrics(), nil, nil, &store.FakeInstanceStore{}, ss, &dashboards.FakeDashboardService{}, &image.NoopImageService{}) | |||
| st := state.NewManager(log.New("test_state_manager"), testMetrics.GetStateMetrics(), nil, nil, &store.FakeInstanceStore{}, ss, &dashboards.FakeDashboardService{}, &image.NotAvailableImageService{}) | |||
There was a problem hiding this comment.
This was so I didn't have to modify a bunch of tests with zero-value image structs.
joeblubaugh
requested review from
zserge,
idafurjes and
ying-jeanne
and removed request for
a team
The State Manager will now take screenshots when an alert instance switches to an Alerting or Resolved state. Signed-off-by: Joe Blubaugh <joe.blubaugh@grafana.com>
joeblubaugh
force-pushed
the
joe/call-image-create-manager
branch
from
9fe09a0 to
223d4e8
Compare
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/19176 |
alexweav
approved these changes
May 22, 2022
| // 1. The alert state is transitioning into the "Alerting" state from something else. | ||
| // 2. The alert state has just transitioned to the resolved state. | ||
| // 3. The state is alerting and there is no screenshot annotation on the alert state. | ||
| func (st *Manager) maybeNewImage(ctx context.Context, alertRule *ngModels.AlertRule, state *State, oldState eval.State) error { |
There was a problem hiding this comment.
Suggested change
| func (st *Manager) maybeNewImage(ctx context.Context, alertRule *ngModels.AlertRule, state *State, oldState eval.State) error { | |
| func (st *Manager) maybeTakeScreenshot(ctx context.Context, alertRule *ngModels.AlertRule, state *State, oldState eval.State) error { |
Huge nit, ignore if you disagree. Just staying away from the new term since that usually implies a constructor in Go.
There was a problem hiding this comment.
This is great, much better name.
alexweav
reviewed
May 22, 2022
| PanelIDAnnotation = "__panelId__" | ||
| DashboardUIDAnnotation = "__dashboardUid__" | ||
| PanelIDAnnotation = "__panelId__" | ||
| ScreenshotTokenAnnotation = "__alertScreenshotToken__" |
There was a problem hiding this comment.
Suggested change
| ScreenshotTokenAnnotation = "__alertScreenshotToken__" | |
| ScreenshotTokenAnnotation = "__alertScreenshotToken__ //nolint:gosec" |
To fix the linter
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/19182 |
joeblubaugh
added
the
area/alerting/screenshots
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The State Manager will now take screenshots when an alert instance
switches to an Alerting or Resolved state.
Signed-off-by: Joe Blubaugh joe.blubaugh@grafana.com
Special notes for your reviewer:
This PR is based on joe/add-screenshots-to-ngalert, #49293 . Once that PR merges, I'll change the base to
main