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

Feat: Alerts per check #1011

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Feat: Alerts per check #1011

wants to merge 50 commits into from

Conversation

VikaCep
Copy link
Contributor

@VikaCep VikaCep commented Dec 6, 2024

Closes https://github.com/grafana/synthetic-monitoring/issues/186

UI implementation for the new alerts per check.

Some considerations:

Screenshots:
image

  • POST request
image
  • GET request
image

@VikaCep VikaCep self-assigned this Dec 6, 2024
@VikaCep VikaCep requested a review from a team as a code owner December 6, 2024 20:13
@VikaCep VikaCep requested a review from w1kman December 6, 2024 20:13
@VikaCep VikaCep marked this pull request as draft December 6, 2024 20:20
@VikaCep VikaCep requested a review from ckbedwell December 6, 2024 20:38
@VikaCep VikaCep marked this pull request as ready for review December 10, 2024 21:23
@ka3de
Copy link
Contributor

ka3de commented Dec 11, 2024

Thanks for making time to work on this @VikaCep ! 🙌
I will not make comments on the code because that's beyond my knowledge, but I have a couple of small comments on the UX, I would not consider these as blockers for this PR, but something that we can consider including in the long run.

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 %, or if it is supposed to be defined in seconds (s), milliseconds (ms) or days, etc.

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.

Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

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 😃

src/types.ts Outdated Show resolved Hide resolved
src/components/CheckForm/AlertsPerCheck.tsx Outdated Show resolved Hide resolved
src/data/useCheckAlerts.ts Outdated Show resolved Hide resolved
src/datasource/DataSource.ts Outdated Show resolved Hide resolved
src/page/EditCheck/EditCheck.tsx Outdated Show resolved Hide resolved
const threshold: number = getValues(`alerts.${predefinedAlert.type}.threshold`) || 0;

return (
<Card
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@majavojnoska
Copy link

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.

@VikaCep VikaCep requested a review from ckbedwell December 12, 2024 19:51
@VikaCep VikaCep marked this pull request as draft December 16, 2024 12:38
@VikaCep
Copy link
Contributor Author

VikaCep commented Dec 16, 2024

Setting the PR to draft until we settle on the UI/UX changes and address them.

VikaCep added 17 commits January 7, 2025 11:54
- 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
@VikaCep
Copy link
Contributor Author

VikaCep commented Jan 14, 2025

Thanks @ka3de! Added the default values:

2025-01-14 17 24 35

@VikaCep VikaCep requested a review from w1kman January 14, 2025 20:25
The API is removing the id property for check alerts as they can be identified just by the name
Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

Works mint now.

Nit: The label thingy is a bit off center though.
image

image

@VikaCep VikaCep requested a review from w1kman January 22, 2025 20:16
@VikaCep
Copy link
Contributor Author

VikaCep commented Jan 22, 2025

Update with the header text centered

image

@VikaCep
Copy link
Contributor Author

VikaCep commented Jan 23, 2025

I've removed the mocked data as now the API is available in dev 🚀

@VikaCep
Copy link
Contributor Author

VikaCep commented Jan 27, 2025

I have made some updates based on @ckbedwell comment on Slack:

Shared (validate both sides?)

  1. I can submit values that should be invalid. I submitted 1000% here for Probe Failed Executions Too High and it got converted to 999.99 which is still too high by about 900 😅

UI/UX

  1. If I delete everything in the input field it resets to 0 and I can't delete the 0. If I want to type 10 it appears as 010 (if I select all then type I can get rid of the 0)
  2. I found just visiting the alerts section changes the form state so even if I make no changes to my check I get the Unsaved changes modal
  3. The double alert notification is confusing. I know they are separate APIs but we shouldn't render the alert update even if successful. Which brings us onto:
  4. I like that if the alerts api fails we stay on the page but we have to make this experience nicer even if it should be very rare as it is very confusing what just happened. We need to be more explicit that the check did save but its associated alerts did not. Let's handle this problem in a separate PR but we need to address it.

Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

When using scientific notation, it's possible to save the form with a 0 as a value.

E.g. Entering 1e-100 can be saved, which results in a 0. If that scenario is triggered, the form step is still validated as an error (if you go to the next step) but without indication on what is the actual error.

image

If there is a step error (follow instructions above to trigger), it's not possible to empty the field. If the field says 0, the 0 cannot be removed, only overwritten.

When in a broken state, there is no error message on entering an invalid value. The value may also be "randomly" removed when switching step/section.

src/components/CheckForm/AlertsPerCheck/AlertItem.tsx Outdated Show resolved Hide resolved
src/components/CheckForm/AlertsPerCheck/AlertItem.tsx Outdated Show resolved Hide resolved
src/components/CheckForm/AlertsPerCheck/AlertsPerCheck.tsx Outdated Show resolved Hide resolved
src/components/CheckForm/AlertsPerCheck/AlertsPerCheck.tsx Outdated Show resolved Hide resolved
@VikaCep
Copy link
Contributor Author

VikaCep commented Jan 28, 2025

When using scientific notation, it's possible to save the form with a 0 as a value.

I added a validation to prevent users from entering numbers in scientific notation in the threshold inputs. I also added a min value of 0.01 as after discussing with @ka3de, he told me values with more than two decimals are disregarded in the database (so sending 0.001 would be converted to 0.00)

image
image

@VikaCep VikaCep requested a review from w1kman January 28, 2025 14:48
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

Small one to avoid the unnecessary type assertion. This is pretty much ready to go so will approve after that ⭐

src/types.ts Show resolved Hide resolved
@VikaCep VikaCep requested a review from ckbedwell January 28, 2025 16:13
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

LGTM 🌟

@ckbedwell
Copy link
Contributor

policy bot: approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants