Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Graph: Make old graph panel thresholds work even if ngalert is enabled #38918

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Graph: Make old graph panel thresholds work even if ngalert is enabled #38918

merged 3 commits into from
Sep 15, 2021

Conversation

domasx2
Copy link
Contributor

@domasx2 domasx2 commented Sep 7, 2021 •
edited

What this PR does / why we need it:

With #33850 I mistakenly entirely disabled threshold visualization feature for graph panels. The intention was, if ngalert is enabled, to not show threshold for old panel alert which might still be on the panel model. Determining threshold value for any ng alert(s) connected to the panel is not implemented yet.

This PR undoes the change and insted does not set panel.alert if ngalert is enabled.

This PR makes sure panel.alert is ignored if ngalert is enabled.

However, I think there is a problem: it seems that panel alert threshold was persisted to panel.thresholds. So even if panel.alert is no longer there, the threshold is still shown. While initially this threshold kinda makes sense because panel alert was converted to ng alert which is still attached to the panel, it is no longer synchronized with any changes to the ngalert. I think this would be a source of confusion in the future. However if we delete or disable the threshold, user won't be able to edit thresholds manually.

Maybe ngalert migration should clear panel.thresholds for any panels that have a panel alert? If ngalert flag is unsert, I think ThresholdMapper would populate it again

@hugohaggmark @torkelo WDYT, how to proceed here?

Which issue(s) this PR fixes:

Fixes #38596

Special notes for your reviewer:

@domasx2 domasx2 added this to the 8.1.4 milestone Sep 7, 2021
@domasx2 domasx2 mentioned this pull request Sep 7, 2021
Closed
@domasx2 domasx2 changed the title Graph: make old graph panel thresholds work even if ngalert is enabled Draft: Graph: make old graph panel thresholds work even if ngalert is enabled Sep 7, 2021
@hugohaggmark
Copy link
Contributor

Maybe we should use DashboardMigrator for this? This is a bit out of my knowledge of dashboards, graph and thresholds so I'm not sure at all.

@domasx2
Copy link
Contributor Author

domasx2 commented Sep 7, 2021

Hm, now I'm thinking this PR solution isn't great either... If we remove alert from panel model and user saves it again, alert will be gone, right? Then if ngalert is unflipped, existing panel alert is lost

@domasx2
Copy link
Contributor Author

domasx2 commented Sep 7, 2021

Maybe we should use DashboardMigrator for this?

It runs every time right? Would have to remove thresholds only once to not kill those added by user later

@hugohaggmark
Copy link
Contributor

Maybe we should use DashboardMigrator for this?

It runs every time right? Would have to remove thresholds only once to not kill those added by user later

Yes, but only upgrades once for each version

@domasx2
Copy link
Contributor Author

domasx2 commented Sep 7, 2021

Maybe we should use DashboardMigrator for this?

It runs every time right? Would have to remove thresholds only once to not kill those added by user later

Yes, but only upgrades once for each version

Hm, we should do the migration when ngalert flag is flipped though...

I guess the key question is: once ngalert is flipped and panel alerts are migrated, should the threshold (previously copied from panel alert) remain on panel config and still be visible & user editable, or should it be deleted?
Consider that migrated alerts are now entirely separate from panels, and editing them will not update panel threshold, if any panel is linked.

@torkelo would appreciate any opinion 🙇

@domasx2 domasx2 requested a review from davkal September 8, 2021 14:42
@davkal
Copy link
Contributor

davkal commented Sep 8, 2021

Let's not delete anything until we have a migration feature that won't deprive the user of the visible threshold. Note that this can also be shipped in a later release when thresholds can be dynamically drawn based on the rule, then we can remove the explicit threshold line based on some heuristic.

@domasx2 domasx2 marked this pull request as ready for review September 8, 2021 14:48
@domasx2 domasx2 requested a review from a team September 8, 2021 14:48
@domasx2 domasx2 changed the title Draft: Graph: make old graph panel thresholds work even if ngalert is enabled Graph: make old graph panel thresholds work even if ngalert is enabled Sep 8, 2021
@domasx2
Copy link
Contributor Author

domasx2 commented Sep 13, 2021

Let's not delete anything until we have a migration feature that won't deprive the user of the visible threshold. Note that this can also be shipped in a later release when thresholds can be dynamically drawn based on the rule, then we can remove the explicit threshold line based on some heuristic.

Then this PR is good to go I think. "legacy" alert thresholds will essentially become configured on the panels and remain for the time being

peterholmberg
peterholmberg approved these changes Sep 15, 2021
View reviewed changes
Copy link
Contributor

@peterholmberg peterholmberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I agree with @davkal about leaving thresholds in place on the panel even though they can get outdated.

@domasx2 domasx2 merged commit 264946f into main Sep 15, 2021
@domasx2 domasx2 deleted the domas/fix-thresholds-for-old-graph-panel branch September 15, 2021 13:15
@grafanabot grafanabot mentioned this pull request Sep 15, 2021
Merged
grafanabot pushed a commit that referenced this pull request Sep 15, 2021
@domasx2 @grafanabot
Graph: make old graph panel thresholds work even if ngalert is enabled (
b41670c 
#38918)

(cherry picked from commit 264946f)
domasx2 added a commit that referenced this pull request Sep 15, 2021
@grafanabot @domasx2
Graph: make old graph panel thresholds work even if ngalert is enabled (
6e9b0b6 
#38918) (#39233)

(cherry picked from commit 264946f)

Co-authored-by: Domas <domas.lapinskas@grafana.com>
@sunker sunker changed the title Graph: make old graph panel thresholds work even if ngalert is enabled Graph: Make old graph panel thresholds work even if ngalert is enabled Sep 16, 2021
@sunker sunker added this to the 8.1.4 milestone Sep 16, 2021
stevepostill pushed a commit to Reveal-International/grafana that referenced this pull request Nov 3, 2021
@stevepostill
Merge pull request #7 in AVENGE/grafana from feature/synchronize-from…
17a6008 
…-github to revdev

* commit '56aebaac32bd7a82f84a571969b51c62ba54fd6d': (23 commits)
  "Release: Updated versions in package to 8.1.4" (grafana#39273)
  Docs: Update azuread docs to mention about env variables (grafana#39203) (grafana#39265)
  Docs: plugin migration guide 7.0.0 to 8.0.0 (grafana#38911) (grafana#39246)
  Docs: Added "manageAlerts" provisioning option. (grafana#38836) (grafana#39253)
  BarChart: Fix legend and tooltip colors off by 1 after data refresh (grafana#39234) (grafana#39252)
  Update silences.md (grafana#38834) (grafana#39243)
  Explore: Ensure logs volume bar colors match legend colors (grafana#39072) (grafana#39120)
  Graph: make old graph panel thresholds work even if ngalert is enabled  (grafana#38918) (grafana#39233)
  Fix regex to identify / as separator (grafana#39220)
  [v8.1.x] BarChart: fix stale bar values and x axis labels
  Add link to default template (grafana#39106) (grafana#39138)
  Doc: Created a separate topic for AWS authentication  (grafana#39012) (grafana#39160)
  LDAP: Search all DNs for users (grafana#38891) (grafana#39167)
  Docs: Fix broken link to signed GCS URLs docs (grafana#39144) (grafana#39149)
  [v8.1.x] Variables: Fix not updating inside a Panel when the preceding Row uses "Repeat For" (grafana#39141)
  Docs: Improve v8 upgrade notes for SQL data sources (grafana#38792) (grafana#39124)
  Explore: Trace view now shows trace start time in selected timezone (grafana#39054) (grafana#39093)
  Alerting: Fix notification channel migration (grafana#38983) (grafana#39053)
  Annotations: Fixes blank panels for queries with unknown data sources (grafana#39017) (grafana#39034)
  Copy cue.mod and packages/grafana-schema to container workdir (grafana#38998) (grafana#39018)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterholmberg peterholmberg peterholmberg approved these changes

@torkelo torkelo Awaiting requested review from torkelo

@axelavargas axelavargas Awaiting requested review from axelavargas

@hugohaggmark hugohaggmark Awaiting requested review from hugohaggmark

@davkal davkal Awaiting requested review from davkal

@nathanrodman nathanrodman Awaiting requested review from nathanrodman

Assignees
No one assigned
Labels
add to changelog area/frontend area/panel/graph old backport v8.1.x Mark PR for automatic backport to v8.1.x type/bug
Projects
None yet
Milestone
8.1.4
Development

Successfully merging this pull request may close these issues.

Graph(old): It no longer renders thresholds in 8.1+
5 participants
@domasx2 @hugohaggmark @davkal @peterholmberg @sunker