Amtool implementation #636

Kellel · 2017-02-25T00:08:26Z

As discussed in #567

The primary goal of an alertmanager tool is to provide a cli interface for the prometheus alertmanager. My vision for this tool has two parts: - Silence management (query, add, delete) - Alert management (query, maybe more in future?) Resolves: prometheus#567

…tderr

Error messages are maybe better?

* also allow for man page generation * update vendors to include cobra/doc * split silence commands into multiple files * move some ommon gits into utils.go

Amtool initial implementation

Kellel · 2017-02-25T00:10:35Z

Oops. Looks like I have some vendoring issues

beorn7 · 2017-02-27T11:15:03Z

Looks quite cool. But also very involved.

Before we dive into code-level review, it would be great if @fabxc and/or @brancz could comment if this is fundamentally sane and meeting their expectations. @stuartnelson3 might also have an opinion as he is getting into the AM API quite a bit recently.

stuartnelson3 · 2017-02-27T14:42:55Z

cli/utils.go

+}
+
+// Parse a list of labels (cli arguments)
+func parseMatchers(labels []string, resolve int) (types.Matchers, error) {


This is re-implementing functionality that already exists in PromQL, which seems like another vote for moving PromQL to a standalone package as mentioned in this PR #633

Hmm. I looked at this, but it didn't immediately strike me as a fitting what I was trying to do. I'll have to take another look.

It's my hope to #633 merged soon, so then the tool could accept a promql label matcher string ({foo="bar", baz=~"qu.*"}) and pass that on to the api.

this would be changing your api, however, so it's worth a discussion, and looping in @fabxc to see what he thinks. It looks like this might be expanding into generating a swagger api. Let's drop the discussion down to the main flow of the PR.

The other issue I had was that I needed to filter silences. Will that be covered in #633 ?

Filtering silences is available in that pr, in 0954959

I'm going to try to get that PR pushed through, then it should just be a few easy changes here and we can get this merged.

Cool. Looking at that.

Quick question.
I implemented a feature where if a user doing a query (silence or alert) does something like the following:
amtool alert query Host

With no matcher name specified then I attempt to match the alertname using a prefix match. Is this something we want to support? Moving to promql should do everything I want, but i'm curious if I should spend the time to re-implement this functionality?

hm, @fabxc ?

stuartnelson3 · 2017-03-23T15:31:57Z

cli/silence_add.go

+
+ buf := bytes.NewBuffer([]byte{})
+ enc := json.NewEncoder(buf)
+ err = enc.Encode(silence)


json.NewEncoder(buf).Encode(silence)

The encode and decode stuff doesn't get re-used, so no need to assign it to a var.

stuartnelson3 · 2017-03-23T15:32:05Z

cli/silence_add.go

+ decoder := json.NewDecoder(res.Body)
+
+ response := addResponse{}
+ err = decoder.Decode(&response)


Same as above

stuartnelson3 · 2017-03-23T15:33:02Z

cli/silence_add.go

+ response := addResponse{}
+ err = decoder.Decode(&response)
+ if err != nil {
+ return errors.New(fmt.Sprintf("Unable to parse silence json response from %s", u.String()))


errors.New(fmt.Sprintf("..", thing1, thing2)) == fmt.Errorf("..", thing1, thing2)

Kellel · 2017-04-03T20:40:10Z

Sorry for not having an update in quite a few days. Been busy with other things at work.

* refactor alert fetch to pull filters * discard all multiple query logic in favor of simpler regex * discard all matching logic * cleanup a number of small things that made the code odd * relax the regex in pkg/parse/parse.go to allow no quote characters

stuartnelson3 · 2017-04-07T13:50:28Z

Running the top level amtool alert is returning all alerts, but if I try a regex matcher I get an error:

# ./amtool alert query 'alertname=~MySQL.*'
Error: [bad_data] bad matcher format
Usage:
  amtool alert [flags]

Flags:
      --expired    Show expired alerts as well as active
  -s, --silenced   Show silenced alerts

Global Flags:
      --alertmanager.url string   Alertmanager to talk to
      --config string             config file (default is $HOME/.amtool.yml)
  -o, --output string             Output formatter (simple, extended, json) (default "simple")
  -v, --verbose                   Verbose running information

[bad_data] bad matcher format

The documentation seems to say this is valid though:

amtool alert query 'alertname=~foo.*'

Actually, it seems like every query I'm running is resulting in a bad matcher error?

# ./amtool alert query alertname=NodeHasMemoryErrors
Error: [bad_data] bad matcher format

Same deal with silences: Showing all silences works, but when I try to query for them it says bad matcher.

Kellel · 2017-04-07T14:26:50Z

Looks like I forgot to register the query alias for the alert command. 🤣

Silence seems to be working in my testing environment. Did you rebuild your alertmanager and docker container?

stuartnelson3 · 2017-04-10T09:33:21Z

Ahhhh yeah I didn't rebuild the actual alertmanager binary :)

This is super cool!

Querying seems to be working, except for the bare alertname case:

# ./amtool --alertmanager.url $url alert query alertname=K8sCPURequestTooLow
Alertname            Starts At                Summary                 
K8sCPURequestTooLow  2017-04-10 06:27:06 UTC  Pod CPU use is too low 

# ./amtool --alertmanager.url $url alert query K8sCPURequestTooLow
Error: [bad_data] bad matcher format
Usage:
  amtool alert query [flags]

Flags:
      --expired    Show expired alerts as well as active
  -s, --silenced   Show silenced alerts

Global Flags:
      --alertmanager.url string   Alertmanager to talk to
      --config string             config file (default is $HOME/.amtool.yml)
  -o, --output string             Output formatter (simple, extended, json) (default "simple")
  -v, --verbose                   Verbose running information

[bad_data] bad matcher format

Other than that this looks pretty cool to me.

One thing that would be nice is also filtering by receiver as a sort of special key-value pair, but I think we can wait on that because it's also a desired feature in the UI, so doing that via a query param in the API is probably what will happen.

Kellel · 2017-04-10T18:17:33Z

Oh I don't think I re-implemented the bare alertname functionality now that I'm not parsing any of the arguments.

Should be reasonably simple to re-add

stuartnelson3 · 2017-04-11T09:29:57Z

seems to also be an error when using the config subcommand:

# ./amtool config -v
Using config file: /Users/stuartnelson/.amtool.yml
Error: json: cannot unmarshal string into Go struct field Route.match_re of type config.Regexp

expiring an already expired silence:

# ./amtool silence expire a6c18c6b-9cb9-4826-a657-f054bef0a121
Error: end time must not be before start time
Usage:
  amtool silence expire [flags]

Global Flags:
      --alertmanager.url string   Alertmanager to talk to
      --config string             config file (default is $HOME/.amtool.yml)
  -o, --output string             Output formatter (simple, extended, json) (default "simple")
  -v, --verbose                   Verbose running information

end time must not be before start time

it also seems that any error output is duplicated? I noticed this before but forgot to mention it.

One idea that I had while interacting with this:

It's awesome that I can e.g.:

# ./amtool silence expire $uuid1 $uuid2 $uuid3

But this requires me to do some bashery to grab the IDs:

# ./amtool silence expire (./amtool silence query 'alertname=~TestAmTool.*' | awk 'NR>1{print $1}')

Taking a cue from the docker cli, what do you think about adding a -q flag to show just IDs? So this would then collapse into

# ./amtool silence query foo=bar -q
c0c6cb44-cac5-4c26-a797-a7f688009890
c776bc72-7d88-438b-bed7-0485d1ab975b
8fa7a0e6-5723-4c9f-892e-8b8d99a93eb3
1795274c-555e-46dd-b861-da5da459bc22
76441da9-f583-4188-a7f7-e674f24092e0
6c4f4425-62f3-487a-bd83-92462662895b
d31b41fe-c1da-47d7-8dba-2b33438b87ef
e91409e4-e356-43c7-bd2f-422234d5b15a
82af6174-f098-47a9-b4e3-c6700b1ff5e7
0ce9c4c3-5001-4cf2-8d7d-67702a02a0d9
a368b6ce-5e27-4191-a315-18c649e3cce5

Seems to be -q and --quiet. What do you think about this?

stuartnelson3 · 2017-04-11T09:45:03Z

There's also an inconsistency between the UI and the cli tool when making a silence with a regex:

# ./amtool silence add 'alertname=~TestAmTool.*' --comment='test out creation'
dfbacf0a-a4ed-4bf2-80d5-26dad57f8b65

The created regex ends up being set as the name:

But that's only supposed to happen internally when creating the regex. Creating the same thing through the ui results in alertname=~"TestAmTool.*" without the starting and ending anchors.

Kellel · 2017-04-11T17:47:08Z

I think the -q --quiet would be a great addition.

After a little bit of debugging it seems that the "(?:TestAmTool.*)$" bug seems to be coming directly from the way that pkg/parse.go works I was hoping to use that code to do the parsing of matchers.

Also remove uneeded function

Kellel · 2017-04-11T17:57:38Z

vendor/github.com/prometheus/prometheus/pkg/labels/matcher.go

@@ -46,8 +46,7 @@ func NewMatcher(t MatchType, n, v string) (*Matcher, error) {
 Value: v,
 }
 if t == MatchRegexp || t == MatchNotRegexp {
- m.Value = "^(?:" + v + ")$"
- re, err := regexp.Compile(m.Value)
+ re, err := regexp.Compile("^(?:" + m.Value + ")$")


This seems to be the cause of the regex information showing up in the ui since I'm using the internal parser to parse silence matchers.

Is this change acceptable? All tests are still passing

I would prefer not to diverge from vendored packages.

I went through the code and made a couple changes, but I'll leave it to you to apply them to your branch.

First, reverting the change to the vendored package:

git revert 5d0bba4b3b3e682b4eae10381ce96660d5c3979a

Then here's a patch file that you can apply. Read through it to see what it does, but basically it separates out the actual parsing code in pkg/parse so that there's an exported function for parsing the raw input string, which the CLI then uses to send to the API.

parse_regex.txt

Note: Apparently github won't let me upload a .patch file, so it says text, but you should be able to apply it all the same.

Edit:
In case you have to google it (which I always do 🤷‍♂️ ), I'll save you the 30sec:

git apply parse_regex.patch

… silences" This reverts commit 5d0bba4.

Kellel · 2017-04-17T16:28:32Z

What are our next steps to get this merged?

stuartnelson3 · 2017-04-17T17:56:49Z

There are a few things that need to be fixed:

Config output is failing to marshal my config:

# ./amtool config -v
Using config file: /Users/stuartnelson/.amtool.yml
Error: json: cannot unmarshal string into Go struct field Route.match_re of type config.Regexp
Usage:
  amtool config [flags]

Global Flags:
      --alertmanager.url string   Alertmanager to talk to
      --config string             config file (default is $HOME/.amtool.yml)
  -o, --output string             Output formatter (simple, extended, json) (default "simple")
  -v, --verbose                   Verbose running information

json: cannot unmarshal string into Go struct field Route.match_re of type config.Regexp
# cat ~/.amtool.yml 
alertmanager.url: http://redacted.net:9093
author: [email protected]
comment_required: true
output: simple

Output errors are printed twice (see above)
Can you move the --quiet flag to the top-level silence subcommand? So regardless of if I'm querying or looking at all silences, this will output the ids (currently it's just working for query)

After that this should be ready to merge. Thanks for you patience!

implementation

stuartnelson3 · 2017-04-20T09:03:51Z

Awesome. I'm going to go ahead and merge this because it's been so long in a branch. There's an issue I would like to address still, but I'll put that in issue #721

Kellen Fox and others added 25 commits December 31, 2016 15:58

Update gitignore

cd0d337

Implement config subcommand

7578ffc

Move output flag to root command. Also print config notification to s…

f33bac9

…tderr

Implement alert command

b2ab06b

Revamp silence command

be80a7e

Flesh out format stuff

02c4ac2

Fix config format for extended output

ba742c8

Allow alername default when no = specified

e61d360

Vendor in cobra, viper, and pflag

605c403

Fix argument ordering

fbd99d9

added more comments:

0d534c2

Remove unneeded copyright block

89869e7

Fix capital A in Alertmanager

a0bab45

Exit 1 instead of -1

1d8dbf0

Remove un-used function

513312f

Fixup tests to simplified types

8915d2f

Remove unneeded comment from code generation

a22ac3d

Make alertmanager url flag consistent with alertmanager cli syntax

d9413d0

Made the url validation better

d1dec7e

Error messages are maybe better?

Fixups from 1:1 session

360a631

Vastly improve help commands

2199e68

* also allow for man page generation * update vendors to include cobra/doc * split silence commands into multiple files * move some ommon gits into utils.go

Add docs about config file

8f614b0

Add docs about how to generate man pages and bash completions

8e8f364

Merge pull request #1 from Kellel/amtool

597a362

Amtool initial implementation

Kellen Fox added 2 commits February 24, 2017 16:34

Fix my vendors

c7e603a

Re-order things in tests so that they pass (things are sorted now)

f5478c6

stuartnelson3 reviewed Feb 27, 2017

View reviewed changes

stuartnelson3 reviewed Mar 23, 2017

View reviewed changes

Kellen Fox added 2 commits April 6, 2017 11:30

Add missing dependancy

aa47c41

Kellel mentioned this pull request Apr 6, 2017

Don't return alerts in the API (alerts/groups) until they are cross-c… #692

Closed

Add alert query alias

e3caaa8

Re-implement alertname= prefix when the first argument isn't a matcher

7d6a860

Fix bug with extra regex information leaking from cli created silences

5d0bba4

Also remove uneeded function

Kellel commented Apr 11, 2017

View reviewed changes

Kellen Fox added 3 commits April 11, 2017 13:23

Implement -q flag for only silence id output

ac50b07

Revert "Fix bug with extra regex information leaking from cli created…

507b32b

… silences" This reverts commit 5d0bba4.

Revert change to vendored repo and apply parsing change patch

dabbd06

Kellen Fox and others added 4 commits April 19, 2017 11:20

Make -q --quiet flags a silence global command

7f3ce6d

Fixup config handling since things changed in alertmanager since initail

0da7098

implementation

Remove double error notifications

ffb6747

Merge branch 'master' into master

e56573b

stuartnelson3 merged commit 3aab66e into prometheus:master Apr 20, 2017

mjtrangoni mentioned this pull request Apr 26, 2017

Amtool: Documentation #741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amtool implementation #636

Amtool implementation #636

Kellel commented Feb 25, 2017

Kellel commented Feb 25, 2017

beorn7 commented Feb 27, 2017

stuartnelson3 Feb 27, 2017

Kellel Feb 27, 2017

stuartnelson3 Feb 28, 2017

Kellel Mar 3, 2017

stuartnelson3 Mar 15, 2017

Kellel Mar 16, 2017

Kellel Mar 16, 2017 •

edited

Loading

stuartnelson3 Mar 17, 2017

stuartnelson3 Mar 23, 2017

stuartnelson3 Mar 23, 2017

stuartnelson3 Mar 23, 2017

Kellel Mar 23, 2017

Kellel commented Apr 3, 2017

stuartnelson3 commented Apr 7, 2017 •

edited

Loading

Kellel commented Apr 7, 2017

stuartnelson3 commented Apr 10, 2017

Kellel commented Apr 10, 2017

stuartnelson3 commented Apr 11, 2017

stuartnelson3 commented Apr 11, 2017

Kellel commented Apr 11, 2017

Kellel Apr 11, 2017

stuartnelson3 Apr 12, 2017 •

edited

Loading

Kellel commented Apr 17, 2017

stuartnelson3 commented Apr 17, 2017

stuartnelson3 commented Apr 20, 2017

Amtool implementation #636

Amtool implementation #636

Conversation

Kellel commented Feb 25, 2017

Kellel commented Feb 25, 2017

beorn7 commented Feb 27, 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

Kellel Mar 16, 2017 • edited Loading

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

Kellel commented Apr 3, 2017

stuartnelson3 commented Apr 7, 2017 • edited Loading

Kellel commented Apr 7, 2017

stuartnelson3 commented Apr 10, 2017

Kellel commented Apr 10, 2017

stuartnelson3 commented Apr 11, 2017

stuartnelson3 commented Apr 11, 2017

Kellel commented Apr 11, 2017

Choose a reason for hiding this comment

stuartnelson3 Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Kellel commented Apr 17, 2017

stuartnelson3 commented Apr 17, 2017

stuartnelson3 commented Apr 20, 2017

Kellel Mar 16, 2017 •

edited

Loading

stuartnelson3 commented Apr 7, 2017 •

edited

Loading

stuartnelson3 Apr 12, 2017 •

edited

Loading