Filter alerts #633

stuartnelson3 · 2017-02-24T15:43:21Z

@fabxc something I wanted to add in the new UI was filtering alerts using the same promql label matching language.

~~promQL and the metric.LabelMatchers aren't a separable library, so it makes for a super terrible vendoring situation.~~

~~I talked with @beorn7 and he reminded me that you had mentioned that you wanted promQL to be usable as a standalone package.~~

~~Do you have any thoughts on this? Is this something we could do to contribute to the project?~~

package labels are imported from prometheus branch dev-2.0.

fabxc · 2017-02-27T15:44:08Z

PromQL is usable as a standalone package largely.
The matchers are troubling though as it ties into the storage/metric package. Ties to common/model are also an anti-pattern at best.

I did significant refactoring of Prometheus's internal type structure in the dev-2.0 branch. It is largely self-contained now and does not depend on anything related to the storage.

https://github.com/prometheus/prometheus/tree/dev-2.0/promql

You can give it a look and let me know if and how it helps.

In general one has to consider at which point one wants to depend at all as opposed to just copying the relevant bits of code. Label matcher queries are somewhat involved of course. So it can make sense.

fabxc · 2017-02-27T15:42:35Z

dispatch/dispatch.go

 	var overview AlertOverview

 	d.mtx.RLock()
 	defer d.mtx.RUnlock()

+	matchers, err := promql.ParseMetricSelector(filter)
+	if err != nil {
+		// TODO: Do we bail on a bad filter? Or just ignore it and continue on?


Yes, return an error in this function.

stuartnelson3 · 2017-02-28T13:56:36Z

I'm getting a failure on circle-ci because of the build constraint in vendor/github.com/dgryski/go-bits -- the files only compile for arch=amd64, so it fails on the 386 build. @fabxc thoughts?

fabxc · 2017-02-28T18:45:33Z

@stuartnelson3 that shouldn't be the case I think. There's a fallback for the amd64 assembly implementations: https://github.com/dgryski/go-bits/blob/master/clz.go

Looks like vendoring tools might only vendor for the arch they are run on. You'll probably have to add it manually.

fabxc · 2017-02-28T18:47:27Z

This now imports a ton of other stuff storage related. Mh, I thought I eliminated everything.

fabxc · 2017-02-28T18:52:47Z

Urgh, so it still depends on storage/ which is just a set of interface, so that's largely okay.
The promql test setup cannot be a _test.go file for other reasons but of course relies on a storage to plug into test... which happens to be the actual storage. So it pulls in all the code again =/

brancz · 2017-03-01T07:59:30Z

Sounds like integration tests that may better live in /test rather than in the promql package itself. Tests in the promql package should simply test against a mocked storage. That said, that's probably a ton of work.

stuartnelson3 · 2017-03-01T09:30:27Z

Not sure if this is a valid vendoring strategy by you folks, but I went ahead and deleted the test.go and benchmark.go files and removed their dependencies.

beorn7 · 2017-03-01T12:55:58Z

The circleci build fails because github.com/cespare/xxhash/xxhash_other.go is not vendored. govendor sometimes gets confused with the build tags. In principle, it should be able to correctly include all those architecture-dependent files. We can have a look together, I dealt with a similar thing in the prometheus repo recently.

fabxc · 2017-03-01T15:47:25Z

@stuartnelson3 thinking a bit more. Parsing a metric descriptor or selector are so useful across a bunch of potential tools, I don't thinking binding us to the promql package is generally useful here. The parsing logic is well-defined and not bound to change anytime soon. So having duplicate code here twice seems totally fine.

I'd propose adding parsing code for both to the dev-2.0 branch for pkg/labels: https://github.com/prometheus/prometheus/tree/dev-2.0/pkg/labels

ParseMetric(string) (Labels, error)
ParseLabels(string) (Labels, error)
ParseMatcher(string) (*Matcher, error)

That can either be copying logic from promql (probably a bit complex with the full lex/parse cycle), a regexp, or a small new parse loop tokenizing strings and checking them for validity after (code: https://github.com/prometheus/prometheus/tree/dev-2.0/pkg/labels).

How does that sound?

stuartnelson3 · 2017-03-01T16:05:17Z

@fabxc that makes sense -- parsing metrics/selectors is going to be needed in most tools within the prometheus ecosystem, but all of the additional promql stuff is not necessary.

Whichever of your three suggestions you prefer sounds good to me. If a regexp is easiest that sounds fine to me since these won't be performance critical, but a smaller parse loop would also be fine by me.

Any idea when you'll have a chance to add this to your dev-2.0 branch?

fabxc · 2017-03-01T16:14:27Z

Somewhat busy right now. Also we have to add a Selector type actually composed of multiple matchers and it should be ParseSelector then.

Blocker here is that I've already been at that point and realized that we might want it to be an interface that can also implement OR. metric{a=~"b"} is basically an AND between two matchers – but for something like federation, one also wants to check whether two different of those match.
I recall that this hit a wall somewhere.

FWIW, in the meantime I'd just hack into AM whatever you need and we can worry about making it nice and moving it into the right package later. Just stab it with a regex ;)

stuartnelson3 · 2017-03-06T13:09:39Z

Sweet lord finally.

This ... got a bit out of hand. There was a lot of crazy vendoring updates that have resulted in a ridiculous diff.

The bottom line is that alertmanager will now filter the silences and alert groups as designated by the URL encoded matcher string in the filter query value.

http://alertmanager.example.com/api/v1/silences?filter=%7Bseverity%3D%22warning%22%2C%20owner%3D%22backend%22%7D

Corresponds to the label matchers:

{severity="warning", owner="backend"}

fabxc · 2017-03-06T13:13:29Z

Can you please make sure to keep the commits for a PR in a reviewable way. This is 20 commits not and I've no clear set of things to look at other than "everything".

stuartnelson3 · 2017-03-06T13:15:34Z

Yeah, I apologize for that. I had a lot of issues going back and forth with vendoring. I'll squash these down into "code" and a "vendoring" commit.

stuartnelson3 · 2017-03-06T14:08:11Z

api/api.go

 		sils = append(sils, s)
 	}

 	respond(w, sils)
 }

+func matchesFilterLabels(s *types.Silence, matchers []*labels.Matcher) bool {
+	sms := map[string]string{}
+	for _, m := range s.Matchers {


This whole loop is less than ideal.

I'm not sure how Matchers is used elsewhere in the code or if it is a slice for a reason, but changing the initial parsing of the protobuf silence so that silence.Matchers is a map[string]*Matcher would simplify this.

Or, if you don't care about this being in the API, that's fine too.

We probably don't want to couple our protobuf representation, which is also our internal model to that type.
I'd have to give our whole architecture a proper look and see how it all fits with the modeling changes we discussed as part of the new UI.

stuartnelson3 · 2017-03-06T14:09:21Z

@fabxc Cleaned up the commits, should be easy to read now.

This updates several old dependencies, removes some that are no longer needed, and adds `pkg/labels` from prometheus `dev-2.0` branch.

This is a temporary simplified re-implementation of promQL's metric selector parsing.

Filter alerts through `?filter=` query string.

Filter silences through `?filter=` query string.

stuartnelson3 · 2017-03-15T15:51:03Z

Assuming tests pass, I would love to get this merged -- querying silences and alerts is something we definitely wanted to add to the API, and when we figure out how we want to do the inverse searching (I think that was one of the implementation optimizations) it can just be changed without modifying the API.

Adding this also helps remove a decent amount of filtering code from #636

fabxc · 2017-03-16T07:40:24Z

Sorry about dropping the ball. Generally looks fine to me 👍

fabxc · 2017-03-16T07:37:14Z

api/api.go

 		sils = append(sils, s)
 	}

 	respond(w, sils)
 }

+func matchesFilterLabels(s *types.Silence, matchers []*labels.Matcher) bool {
+	sms := map[string]string{}
+	for _, m := range s.Matchers {


We probably don't want to couple our protobuf representation, which is also our internal model to that type.
I'd have to give our whole architecture a proper look and see how it all fits with the modeling changes we discussed as part of the new UI.

fabxc · 2017-03-16T07:38:48Z

parse/parse.go

@@ -0,0 +1,59 @@
+package parse


Can we move this to pkg/parse?

* 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 mentioned this pull request Feb 27, 2017

Amtool implementation #636

Merged

fabxc reviewed Feb 27, 2017

View reviewed changes

stuartnelson3 force-pushed the filter-alerts branch from ab15877 to 7b68724 Compare March 1, 2017 09:16

stuartnelson3 force-pushed the filter-alerts branch from 7810ad0 to 0954959 Compare March 6, 2017 13:58

stuartnelson3 commented Mar 6, 2017

View reviewed changes

stuartnelson3 added 4 commits March 15, 2017 16:34

Vendor dependencies.

ab275cd

This updates several old dependencies, removes some that are no longer needed, and adds `pkg/labels` from prometheus `dev-2.0` branch.

Add metrics selector parsing code

0a0f654

This is a temporary simplified re-implementation of promQL's metric selector parsing.

Add alerts filtering

714d106

Filter alerts through `?filter=` query string.

Add silences filtering

049fbbd

Filter silences through `?filter=` query string.

stuartnelson3 force-pushed the filter-alerts branch from 0954959 to 049fbbd Compare March 15, 2017 15:47

fabxc reviewed Mar 16, 2017

View reviewed changes

Move parse to pkg/parse

73b25ec

stuartnelson3 merged commit 1e34f29 into master Mar 16, 2017

stuartnelson3 deleted the filter-alerts branch March 16, 2017 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter alerts #633

Filter alerts #633

stuartnelson3 commented Feb 24, 2017 •

edited

Loading

fabxc commented Feb 27, 2017

fabxc Feb 27, 2017

stuartnelson3 commented Feb 28, 2017

fabxc commented Feb 28, 2017 •

edited

Loading

fabxc commented Feb 28, 2017

fabxc commented Feb 28, 2017

brancz commented Mar 1, 2017

stuartnelson3 commented Mar 1, 2017

beorn7 commented Mar 1, 2017

fabxc commented Mar 1, 2017

stuartnelson3 commented Mar 1, 2017

fabxc commented Mar 1, 2017 •

edited

Loading

stuartnelson3 commented Mar 6, 2017

fabxc commented Mar 6, 2017

stuartnelson3 commented Mar 6, 2017

stuartnelson3 Mar 6, 2017 •

edited

Loading

fabxc Mar 16, 2017

stuartnelson3 commented Mar 6, 2017

stuartnelson3 commented Mar 15, 2017

fabxc commented Mar 16, 2017

fabxc Mar 16, 2017

fabxc Mar 16, 2017

Filter alerts #633

Filter alerts #633

Conversation

stuartnelson3 commented Feb 24, 2017 • edited Loading

fabxc commented Feb 27, 2017

fabxc Feb 27, 2017

Choose a reason for hiding this comment

stuartnelson3 commented Feb 28, 2017

fabxc commented Feb 28, 2017 • edited Loading

fabxc commented Feb 28, 2017

fabxc commented Feb 28, 2017

brancz commented Mar 1, 2017

stuartnelson3 commented Mar 1, 2017

beorn7 commented Mar 1, 2017

fabxc commented Mar 1, 2017

stuartnelson3 commented Mar 1, 2017

fabxc commented Mar 1, 2017 • edited Loading

stuartnelson3 commented Mar 6, 2017

fabxc commented Mar 6, 2017

stuartnelson3 commented Mar 6, 2017

stuartnelson3 Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

fabxc Mar 16, 2017

Choose a reason for hiding this comment

stuartnelson3 commented Mar 6, 2017

stuartnelson3 commented Mar 15, 2017

fabxc commented Mar 16, 2017

fabxc Mar 16, 2017

Choose a reason for hiding this comment

fabxc Mar 16, 2017

Choose a reason for hiding this comment

stuartnelson3 commented Feb 24, 2017 •

edited

Loading

fabxc commented Feb 28, 2017 •

edited

Loading

fabxc commented Mar 1, 2017 •

edited

Loading

stuartnelson3 Mar 6, 2017 •

edited

Loading