Click alert matchers #690

stuartnelson3 · 2017-04-03T15:26:34Z

On alert list view, clicking a matcher sets the filter to that value. Holding ctrl or cmd will append to the current filter, or, if the label already exists, it will remove it. The UX motivation behind this is how users interact with a standard filesystem GUI -- clicking single files highlights that file, cmd+click'ing will select multiples.

Play around with it to make sure it's working correctly, and that this UX is what we want.

addresses #674

stuartnelson3 · 2017-04-04T09:25:19Z

@w0rm

mxinden · 2017-04-04T09:37:03Z

I like the idea of multi-label-selection but I don't know any web application that uses ctrl to do multi-select. This seems rather not intuitive for a web application. Do we need the replace option? Can't we just make the append option default without needing to press the ctrl button?

mxinden · 2017-04-04T09:38:36Z

ui/app/src/Updates.elm

+            in
+                ( { model | mode = mode }, Cmd.none )
+
+        AddLabel msg ( key, value ) ->


This is only relevant for the SilenceList and the AlertList right? Would it be possible to move it down into the respective update functions? Feels weird that this lives in the global update function.

I wasn't sure where to put it. It's definitely something only concerned with the SilenceList and AlertList views, but it is updating the top-level filter.

All the code for labels should be extracted into its own filter method, so it wouldn't be a problem to have the two different update functions implement this.

Thinking about this now, it would simplify the message being sent, and then the update function in question would know what follow-up message to use (fetch alerts/silences). I'll give it a try and see how it looks.

brian-brazil · 2017-04-04T09:54:10Z

I've seen many web-apps use crtl for multi-select, it has been the standard for desktop apps for decades.

stuartnelson3 · 2017-04-04T10:12:44Z

the ctrl+click felt natural to me, but that is just my personal feeling.

If the group decides to have a default Append behavior and clicking the same label twice removes it, that's fine too. I'm not sure what the current expected web behavior is for something like this, but having talked briefly with @beorn7 this seemed to make sense to us both.

brancz · 2017-04-04T10:18:21Z

I don't have the possibility to spin this up myself right now, but from the mock ups we did on the whiteboard I would expect one click to add a label to a query in some kind of a search bar. Then a label in the query would have an "x" button to remove it from the query. Something like this.

brancz · 2017-04-04T10:20:04Z

I don't think

I've seen many web-apps use crtl for multi-select, it has been the standard for desktop apps for decades.

Is an argument for it from a usability perspective. Just because it's been done for a long time, it doesn't mean it's good in terms of usability.

w0rm · 2017-04-05T14:07:53Z

ui/app/src/Updates.elm

+                                   )
+                                |> (\labels ->
+                                        if List.member label labels then
+                                            List.filter (\l -> l /= label) labels


List.filter ((/=) label) labels

w0rm · 2017-04-05T14:16:42Z

ui/app/src/Views/AlertList/Views.elm

+
+        labels =
+            List.filterMap
+                (\( k, v ) ->


filterMap is only needed when you want to filter and map. In this case filter is enough.

labels = List.filter (Tuple.first >> (/=) "alertname") alert.labels

w0rm · 2017-04-05T14:21:14Z

ui/app/src/Updates.elm

+                                        if String.isEmpty cleaned then
+                                            []
+                                        else
+                                            Regex.split Regex.All (Regex.regex "\\s*,\\s*") cleaned


What will happen if you call Regex.split on the empty string? Wouldn't this result with an empty list?

I check in the other if branch if it is an empty string and return an empty list

@stuartnelson3 why do you need this check? Won't Regex.split work on the empty string?

- Normal click replaces filter with selected label - Modifier (ctrl, cmd) + click appends to filter

w0rm · 2017-04-07T16:32:27Z

ui/app/main.go

@@ -37,15 +37,14 @@ func main() {
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()

-		const elmMake = "elm-make"
-		elmMakeArgs := []string{"src/Main.elm", "--output", "script.js"}
+		elmMakeArgs := []string{"make", "src/Main.elm", "--yes", "--output", "script.js"}


In the future there will only be a single elm binary.

That's cool. In general we try to bundle relevant code changes together in a PR, so try to do that in the future. That having been said, this main.go file is totally unimportant, so no worries on changing it here.

Sorry, I should've opened a separate PR for this change. If you want, I can still do this.

Not a problem, this file will get deleted soon (I hope)

For now we are still going to require the user to explicitly click "filter" before we send an API request to update the list.

stuartnelson3 · 2017-04-08T10:59:23Z

ui/app/src/Views/AlertList/Views.elm

            else
                buttonLink "fa-exclamation-triangle" "#/silences/new?keep=1" "dark-red" (CreateSilenceFromAlert alert)
+
+        labels =
+            List.filter (Tuple.first >> (/=) "alertname") alert.labels


I can read and understand what it's doing, but how is blowing my mind.

This PR closes prometheus#690

stuartnelson3 requested a review from mxinden April 3, 2017 15:26

mxinden reviewed Apr 4, 2017

View reviewed changes

stuartnelson3 mentioned this pull request Apr 4, 2017

Click label matcher -> update filter #674

Closed

mxinden added the component/ui label Apr 5, 2017

w0rm reviewed Apr 5, 2017

View reviewed changes

stuartnelson3 added 4 commits April 7, 2017 11:51

Clicks update alerts filter text

314df71

- Normal click replaces filter with selected label - Modifier (ctrl, cmd) + click appends to filter

Auto-filter when clicking labels

5255cbd

ctrl+click removes already present matcher

6891871

Match arbitrary whitespace before/after comma

08371cc

w0rm force-pushed the click-alert-matchers branch from 8c53eb9 to 08371cc Compare April 7, 2017 10:02

Andrey Kuzmin added 2 commits April 7, 2017 18:20

Run elm make with --yes

693eb52

Always add labels to the filter

b5238f5

w0rm reviewed Apr 7, 2017

View reviewed changes

Updating filter currently sends Noop msg

23a9e04

For now we are still going to require the user to explicitly click "filter" before we send an API request to update the list.

stuartnelson3 commented Apr 8, 2017

View reviewed changes

stuartnelson3 merged commit 8e4c29f into ui-rewrite Apr 8, 2017

stuartnelson3 deleted the click-alert-matchers branch April 8, 2017 11:00

hh pushed a commit to ii/alertmanager that referenced this pull request Nov 6, 2017

Remove travis ci (prometheus#702)

defe2f3

This PR closes prometheus#690

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Click alert matchers #690

Click alert matchers #690

stuartnelson3 commented Apr 3, 2017

stuartnelson3 commented Apr 4, 2017

mxinden commented Apr 4, 2017

mxinden Apr 4, 2017

stuartnelson3 Apr 4, 2017

brian-brazil commented Apr 4, 2017

stuartnelson3 commented Apr 4, 2017

brancz commented Apr 4, 2017

brancz commented Apr 4, 2017

w0rm Apr 5, 2017

w0rm Apr 5, 2017

w0rm Apr 5, 2017

stuartnelson3 Apr 7, 2017

w0rm Apr 7, 2017

w0rm Apr 7, 2017

stuartnelson3 Apr 8, 2017

w0rm Apr 8, 2017

stuartnelson3 Apr 8, 2017

stuartnelson3 Apr 8, 2017

Click alert matchers #690

Click alert matchers #690

Conversation

stuartnelson3 commented Apr 3, 2017

stuartnelson3 commented Apr 4, 2017

mxinden commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil commented Apr 4, 2017

stuartnelson3 commented Apr 4, 2017

brancz commented Apr 4, 2017

brancz commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment