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

Alerting: Change how we display annotations in the rule form #69338

Merged
merged 21 commits into from
Jun 21, 2023

Conversation

VikaCep
Copy link
Contributor

@VikaCep VikaCep commented May 31, 2023

What is this feature?

Changes how we display the section to add annotations when creating a rule. From now on Summary, Description and Runbook URL are always present as optional fields. Besides that, now we display de Dashboard name and Panel name when linked instead of just the id.

Why do we need this feature?

To make it simpler to use.

Who is this feature for?

All users.

Which issue(s) does this PR fix?:

Fixes #68200

image

image

@VikaCep VikaCep added this to the 10.0.x milestone May 31, 2023
@VikaCep VikaCep self-assigned this May 31, 2023
@VikaCep VikaCep force-pushed the alerting/68200-annotations branch from e7c3cbe to e43245d Compare June 12, 2023 20:34
@VikaCep VikaCep marked this pull request as ready for review June 12, 2023 21:53
@VikaCep VikaCep requested a review from a team June 12, 2023 21:53
Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

I tested locally and I don't see the summary, description and runbook url fields when editing an existing alert rule, is this correct? 👀

Screenshot 2023-06-13 at 11 23 57

Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

Also in the prototype I see some changes in this button 👀

Screenshot 2023-06-13 at 11 29 45

@VikaCep
Copy link
Contributor Author

VikaCep commented Jun 13, 2023

I tested locally and I don't see the summary, description and runbook url fields when editing an existing alert rule, is this correct? 👀

Screenshot 2023-06-13 at 11 23 57

I asked Deyan and he confirmed we should always show the default fields. I fixed that in my last PR. It's pending to show them always in the same order though but I'll fix that next.

@konrad147
Copy link
Contributor

konrad147 commented Jun 14, 2023

I think there might be an edge case when it comes to the dashboard and panel link.

It might happen that an alert rule and a dashboard are in different folders, and the user editing the rule has no access to the dashboard and panel. Another example might be when the user has access to alert rules but has no access to dashboards (I think it's doable using RBAC).
In such a case, we need to handle permission errors and display something. The question is if we should allow to override the dashboard and panel in such cases or not.

Until we've displayed just an Id of a dashboard it wasn't an issue, but now we need to fetch a dashboard to display the name but in some cases we might receive 403 error, or the dashboard might be missing in the response (which doesn't indicate it actually doesn't exist, maybe the user just doesn't have the permission to see it)

It might require digging a bit deeper into how permissions work
image

@soniaAguilarPeiron
Copy link
Member

soniaAguilarPeiron commented Jun 14, 2023

Until we've displayed just an Id of a dashboard it wasn't an issue, but now we need to fetch a dashboard to display the name but in some cases we might receive 403 error, or the dashboard might be missing in the response (which doesn't indicate it actually doesn't exist, maybe the user just doesn't have the permission to see it)

What about showing a read only id and a warning in case user doesn't have permission? @konrad147 @VikaCep

@VikaCep
Copy link
Contributor Author

VikaCep commented Jun 14, 2023

I think there might be an edge case when it comes to the dashboard and panel link.

It might happen that an alert rule and a dashboard are in different folders, and the user editing the rule has no access to the dashboard and panel. Another example might be when the user has access to alert rules but has no access to dashboards (I think it's doable using RBAC). In such a case, we need to handle permission errors and display something. The question is if we should allow to override the dashboard and panel in such cases or not.

Until we've displayed just an Id of a dashboard it wasn't an issue, but now we need to fetch a dashboard to display the name but in some cases we might receive 403 error, or the dashboard might be missing in the response (which doesn't indicate it actually doesn't exist, maybe the user just doesn't have the permission to see it)

It might require digging a bit deeper into how permissions work image

Good point. If the dashboard response doesn't return what we expect (a dashboard object with a title and a panel object) I added a fallback where we don't show links but a text indicating dashboard/panel id. Let me know what you think of this solution.

image

Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

LGTM!🚀

@VikaCep VikaCep force-pushed the alerting/68200-annotations branch from 051a701 to aafab41 Compare June 21, 2023 13:42
@VikaCep VikaCep merged commit 929d9ea into main Jun 21, 2023
13 checks passed
@VikaCep VikaCep deleted the alerting/68200-annotations branch June 21, 2023 14:15
LudoVio pushed a commit that referenced this pull request Jun 26, 2023
* Change how we display annotations in the rule form

* Allow to add custom annotations using a free text input

* Get dashboard and panel titles to display in the annotations section

* Add component to display dashboard and panel annotations as links

* Add styling to help tooltip

* Fix styling on annotations controls

* Fix tests

* Fix tests

* Remove unused imports

* Add component for custom annotations

* Display default annotations even if editing and they're empty

* Adjust tests

* Make conditional rendering more clear

* Fix tests

* Move annotation header to separate component

* Fix lint

* Show annotation fields in the right order

* Prevent showing custom annotation fields by default

* Don't display links to dashboard/panel if response fails

* Rename custom annotation header component

* Fix after rebase
@zerok zerok modified the milestones: 10.0.x, 10.0.2, 10.1.x Jun 30, 2023
harisrozajac pushed a commit that referenced this pull request Jun 30, 2023
* Change how we display annotations in the rule form

* Allow to add custom annotations using a free text input

* Get dashboard and panel titles to display in the annotations section

* Add component to display dashboard and panel annotations as links

* Add styling to help tooltip

* Fix styling on annotations controls

* Fix tests

* Fix tests

* Remove unused imports

* Add component for custom annotations

* Display default annotations even if editing and they're empty

* Adjust tests

* Make conditional rendering more clear

* Fix tests

* Move annotation header to separate component

* Fix lint

* Show annotation fields in the right order

* Prevent showing custom annotation fields by default

* Don't display links to dashboard/panel if response fails

* Rename custom annotation header component

* Fix after rebase
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Summary and annotations improvements
5 participants