Feat: Alerts per check #1011
Feat: Alerts per check #1011
Conversation
|
Thanks for making time to work on this @VikaCep ! 🙌 The first one is that thresholds show no units. I think it could be helpful to specify the units for each threshold, for example whether the threshold refers to a The second one we have already discussed it briefly, personally I think the alerts that support multiple percentiles look better grouped in a single "panel" as the previous iteration? In that case, though, we would need to figure a way to allow users to set different threshold values for the different percentiles, in case that more than one is set, as the same value would probably not make sense for all of them. If we finally opt for this solution and there is something that we have to change in the way the API behaves, let me know and we can work it out also. |
There was a problem hiding this comment.
I need to do a much more thorough review of the UI/UX but I'd like to explore how we can do that by mocking the data in someway first.
I've focussed mainly on things I can see in the code first that will be agnostic to any of the other changes we'll discuss 😃
| const threshold: number = getValues(`alerts.${predefinedAlert.type}.threshold`) || 0; | ||
|
|
||
| return ( | ||
| <Card |
There was a problem hiding this comment.
I'll do a much more thorough review of the UI/UX separately but just a heads up I don't think we should use cards but probably a table of some sort.
|
I agree with @ka3de that setting units will clarify what is being measured. When it comes to the UI, my initial impression was that the cards functioned as radio buttons, allowing only one selection, due to the icon in the top right corner. However, after reviewing Daniel's Slack video, I understand users can set multiple alerts. A more elegant solution would be to replace the icons with a clear on/off indicator on the left side of each card. This would improve clarity. Also, I would suggest a more condensed card layout to reduce the prominence of the description. Let's discuss this further at our next meeting. |
|
Setting the PR to draft until we settle on the UI/UX changes and address them. |
- adjust schema validation to make it optional - add test handlers for new alerts requests
- Also, invalidate cache for fetching fresh check alerts
- Instead of grouping by alert type, I'm displaying all alerts as separate ones - Grouping by type didn't allow to specify threshold for different percentiles
- since the alerts API is no longer in dev, I'm adding the mocks back to be able to test it - This should be improved by #850
- Also, adding loading and error states
- Add specific predefined alerts according to the check type in a single constants file - Display threshold units
VikaCep
force-pushed
the
alerts-per-check
branch
from
a75b502 to
0b04626
Compare
|
After discussing with @majavojnoska and @ckbedwell, we agreed to implement the feature using a structure similar to the Probes selection screen, leveraging a list with checkboxes for selection. After applying the changes, the result now looks as follows: HTTP Check: Screen.Recording.2025-01-07.at.16.14.50.movPing Check: All other checks: |
Closes https://github.com/grafana/synthetic-monitoring/issues/186
First iteration of the UI implementation for creating alerts per check.
Some considerations:
Screenshots:
Edit check screen
New check screen
Validation errors:
POST request