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.

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

Graph(old): It no longer renders thresholds in 8.1+ #38596

Closed
torkelo opened this issue Aug 26, 2021 · 13 comments · Fixed by #38918
Closed

Graph(old): It no longer renders thresholds in 8.1+ #38596

torkelo opened this issue Aug 26, 2021 · 13 comments · Fixed by #38918
Assignees
Labels
area/panel/graph prio/high Must be staffed and worked on either currently, or very soon, ideally in time for the next release. type/bug

Comments

@torkelo
Copy link
Member

torkelo commented Aug 26, 2021

Noticed on play site that the old graph is no longer showing any thresholds.

@torkelo torkelo added this to the 8.1.3 milestone Aug 26, 2021
@torkelo torkelo added the prio/high Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Aug 26, 2021
@hugohaggmark
Copy link
Contributor

Is this related to what you're working on @dprokop, I mean with thresholds?

@hugohaggmark hugohaggmark added this to Backlog (max bugs: TBD) in User essentials squad (deprecated) Sep 2, 2021
@hugohaggmark hugohaggmark moved this from Backlog (max bugs: TBD) to To be prioritised in User essentials squad (deprecated) Sep 2, 2021
@hugohaggmark hugohaggmark moved this from To be prioritised to Backlog (max bugs: TBD) in User essentials squad (deprecated) Sep 2, 2021
@dprokop
Copy link
Member

dprokop commented Sep 2, 2021

Is this related to what you're working on @dprokop, I mean with thresholds?

Nope - my work is related to enabling threshold controls in the graph ng based visualizations, and in the alerting views to start with. We have not decided yet whether or not we want to bring this functionality to the time series panel.

@axelavargas
Copy link
Member

@torkelo I noticed there is a message that says: Visual thresholds options disabled, it seems intentional

AlertingThresholds

@axelavargas
Copy link
Member

Nevermind, I tested in another dashboard and indeed there are no thresholds, I will take a look :)

@axelavargas axelavargas self-assigned this Sep 6, 2021
@axelavargas axelavargas moved this from Backlog (max bugs: TBD) to In progress (max. 4) in User essentials squad (deprecated) Sep 6, 2021
@axelavargas
Copy link
Member

Update:

I noticed in the graph plugin we have this line that is checking if ng-alert feature toggle is enabled or not, based on that it will draw the thresholds. On play.grafana.org we have that feature toggle enabled, and this is the reason we can't see thresholds.

// unified alerting does not support threshold for graphs, at least for now
if (!config.featureToggles.ngalert) {
      this.thresholdManager = new ThresholdManager(this.ctrl);
 }

Locally I tested removing the feature toggle and the thresholds were shown correctly, I asked the alerting team about this topic, to gather more context.

@torkelo
Copy link
Member Author

torkelo commented Sep 6, 2021

@axelavargas ah! please remove that if statement, we should always add the threshold manager

@axelavargas
Copy link
Member

@torkelo I got some feedback from @domasx2, this was added to avoid showing dashboard alerts thresholds when ngalert is enabled, but the implementation is missing other threshold use cases like this, he will take over this issue tomorrow.

@axelavargas axelavargas assigned domasx2 and axelavargas and unassigned axelavargas Sep 6, 2021
@torkelo
Copy link
Member Author

torkelo commented Sep 6, 2021

@domasx2 not sure I understand the logic for now showing graph thresholds, this display option should be available to users always.

@domasx2
Copy link
Contributor

domasx2 commented Sep 6, 2021

The intention was to only disable dashboard alert thresholds if ngalert is enabled. Disabling thresholds entirely was a mistake. Not enough research / PR reviews I guess :)

@torkelo
Copy link
Member Author

torkelo commented Sep 6, 2021

@domasx2 but even with ngalert enabled, do not understand the reasons to disable a graph visualization feature

@domasx2
Copy link
Contributor

domasx2 commented Sep 6, 2021

Did not intend to disable graph visualizatiom feature entirely, this was a mistake. Just to not render old existing dashboard alert threshold if ngalert is emabled

@domasx2
Copy link
Contributor

domasx2 commented Sep 6, 2021

I think didnt realize at the time that threshold manager was not for alerts only, my bad

@domasx2
Copy link
Contributor

domasx2 commented Sep 7, 2021

@hugohaggmark @torkelo need some input on how to best proceed with fixing this, problem descirbed in draft PR: #38918 🙇

@sunker sunker modified the milestones: 8.1.3, 8.1.4 Sep 8, 2021
@axelavargas axelavargas moved this from In progress (max. 4) to In Review (max internal 8, max external 3) in User essentials squad (deprecated) Sep 13, 2021
User essentials squad (deprecated) automation moved this from In Review (max internal 8, max external 3) to Done Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/panel/graph prio/high Must be staffed and worked on either currently, or very soon, ideally in time for the next release. type/bug
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants