Default grouping in ui #1864
Default grouping in ui #1864
Conversation
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
w0rm
force-pushed
the
default-grouping-in-ui
branch
from
77973c3 to
9ddadb8
Compare
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
w0rm
force-pushed
the
default-grouping-in-ui
branch
from
372e38c to
eb8345e
Compare
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
|
@stuartnelson3 I'm not sure why the circle ci tests are failing. |
|
Not sure, will take a look at it soon (probably Monday) thanks for doing this! |
|
@beorn7 does it make you happy? :D |
stuartnelson3
left a comment
stuartnelson3
left a comment
There was a problem hiding this comment.
couple questions, but otherwise looks good
| required: | ||
| - labels | ||
| - receiver | ||
| - alerts |
There was a problem hiding this comment.
thanks for fixing the openapi definition!
| type alias Filter = | ||
| { text : Maybe String | ||
| , group : Maybe String | ||
| , customGrouping : Bool |
There was a problem hiding this comment.
Whenever I look at boolean flags/query strings, I wonder if it would be better to have a concrete setting, like grouping=default or grouping=custom. It helps keep things from ballooning to ever larger sizes. For example, I'm not super thrilled with how I introduced silenced=true and inhibited=true, but I wasn't sure if it would be better or worse to have alert_state=silenced|inhibited.
Any thoughts on this? Stay with the boolean flag, or move use a groups query string? @mxinden @simonpasquier @w0rm
There was a problem hiding this comment.
@stuartnelson3 I think the missing parameter should be the default. Because this is how you open it anyways. So grouping=default is not really needed.
|
UI-wise, this looks good to me. Thank you very much. I haven't looked at the code. One feature request (which might or might not already work) is to be able to deep-link one group (with the idea to link back to that group from a notification rather than to all alerts of the receiver, as we do it now). If that's not already possible, it could also be added in a later PR (perhaps backend changes are necessary). Just saying it so that we keep it in mind. |
Yeah, we should be able to add the alert groups fingerprint to the response and "link" to it that way. There would be a few backend changes, nothing major, but enough to warrant a new pr. thanks for the reminder! |
stuartnelson3
left a comment
stuartnelson3
left a comment
There was a problem hiding this comment.
Looks good to me! Will wait until thursday to see if @mxinden or @simonpasquier have something to say.
Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
Show alert groups from the alertmanager config by default and hide the custom grouping behind the checkbox.
customGroupingis present in the url, then fetch alerts from the/alertsendpoint and group on the frontend/alerts/groupsendpointcloses #868