Refactor Alert Service #1209

nathanielc · 2017-02-17T22:19:34Z

This is a refactor of the Alert Service there are a handful of changes:

Add the concept of External vs Internal Handlers
Create explicit types for each of the different responsibilities of the alert service
Remove the concept of Actions, now a handler only ever has a single action.
General reorg of the code to make it easy to consume/extend.

TODO

desa

From what I can tell it seems good. Going to Read over it another time.

This may just be due to my own misunderstanding, but it seems like its not possible to create a topic without using it in a TICKscript. Is this correct?

desa · 2017-03-10T20:27:29Z

services/alert/api.go

+		httpd.HttpError(w, fmt.Sprint("invalid pattern: ", err.Error()), true, http.StatusBadRequest)
+		return
+	}
+	minLevelStr := r.URL.Query().Get("min-level")


Not a really an issue since this isn't a hot code path, but URL.Query() generates a new map each time its called.

https://golang.org/src/net/url/url.go?s=27419:27447#L967

desa · 2017-03-10T20:47:26Z

services/alert/handlers.go

-	h.next = n
-}
-func (h *stateChangesOnlyHandler) Close() {}
+// TODO implement state changes only as a match condition


What does this mean?

Ah, yes so with the change to not have handler actions anymore there is no way to do stateChangesOnly via a handler.

The proposed solution is to use match criteria on handlers. So something like this

id: my_handler topic: system kind: slack options: channel: '#alerts' match: "changed" == TRUE AND "sometag" == 'bob'

Where the match property is a lambda expression then when it evals to true then the handler handles the alert event. This way we get the same functionality back but in a more consistent manner.

I was thinking we should add the match expression logic in another PR so that its can remain as an independent change. Or should we lump it in here so that we maintain feature parity from the get go? Thoughts?

nathanielc · 2017-03-13T18:47:26Z

it seems like its not possible to create a topic without using it in a TICKscript. Is this correct?

No quite, Topics are created whenever they are referenced so there is no need to explicitly create them. So if they are referenced in a TICKscript they are created as well as if a handler references a topic it will be created.

desa · 2017-03-14T20:01:44Z

No quite, Topics are created whenever they are referenced so there is no need to explicitly create them. So if they are referenced in a TICKscript they are created as well as if a handler references a topic it will be created.

Ah I see. I spent some time confused about this. Here's the process I just went through.

First I created a task with this tickscript

var warn_threshold = 4
var crit_threshold = 10

var period = 1h
var every = 1m

var data = stream
  |from()
    .measurement('system')
  |alert()
   .crit(lambda: TRUE)
   .topic('learning')

Then

$ kapacitor

I noticed the define-handler subcommand and then ran

$ kapacitor define-handler -h

From here I noticed

For example:

	Define a handler using the slack.yaml file:

		$ kapacitor define-handler system slack.yaml

So I make a slack.yaml file

id: example
kind: slack
topic: learning
options:
  username: What

and then I run

$ kapacitor define-handler slack slack.yaml

No errors, so moving on.

$ kapacitor show-topic learning
ID: learning
Level: CRITICAL
Collected: 1340
Handlers: [ ]
Events:
Event      Level    Message                Date
system:nil CRITICAL system:nil is CRITICAL 14 Mar 17 15:27 EDT

Hmm, then I run

$ kapacitor list handlers
ID         Topic    Kind
example   slack slack

Hmm, so I rerun

$ kapacitor define-handler -h

This time I notice

Must provide both a topic ID and a path to a handler file.
Usage: kapacitor define-handler <topic> <path to handler file>

and I notice this time that you specify the topic id as the first positional argument to the command. Then everything was fine.

This confusion was definitely due to me not reading, but the positional arguments vs flags split is different than what I'm used to with kapacitor. The two other define subcommands have

$ kapacitor define <task ID> [options]
$ kapacitor define-template <template ID> [options]

where positional arguments are used for naming things and flags are used to specify details.

nathanielc · 2017-03-14T20:38:04Z

@desa Thanks for the walk through. Agreed lets change it to be more consistent.

Would this be more clear?

kapacitor define-handler slack.yaml -topic system

The difference here is that the ID is in the yaml file. Should we move the ID out of the yaml file so that the ID is specified on the command line?

desa · 2017-03-14T20:57:33Z

@nathanielc
My only issue with pulling it out of the config file is that the define-handler is kind of different than the others.

With define or define-template

kapacitor define example -type stream -dbrp telegraf.autogen -tick example.tick

defines a single task and reissuing that command with different flags redefines the task example. The same thing isn't the case for define-handler though. e.g.

kapacitor define-handler my_hanlder -topic system -file slack.yaml

and

kapacitor define-handler my_hanlder -topic other -file slack.yaml

define two different handlers.

I keep mulling over different ways of going about it and I cant think of one I like.

Syntacticly, I like

kapacitor define-handler my_hanlder -topic system -file slack.yaml

the most. Just because its very similar to how the other define commands work. However, it philosophically bothers me, that changing a flag defines a new thing.

nathanielc · 2017-03-14T21:01:24Z

However, it philosophically bothers me, that changing a flag defines a new thing.

This is why I made it an positional argument. In other words the ID for a handler is topicID and handlerID together.

desa · 2017-03-14T21:05:18Z

maybe changing the name of the command?

kapacitor define-topic-handler <topic> <file>

nathanielc · 2017-03-14T21:08:46Z

I like that, its a bit verbose but that is what tab completion is for :)

I did a similar thing in the Go client.

desa · 2017-03-14T21:09:44Z

or what about moving the topic into the handler config

id: example
kind: slack
topic: learning
options:
  username: What

and just having

kapacitor define-handler <file>

nathanielc · 2017-03-14T21:14:17Z

I like topic-handler as it also helps with the other commands like list and delete as they become

kapacitor list topic-handlers <topic> <handler>
kapacitor delete topic-handlers <topic> <handler>

And show-handler becomes show-topic-handler <topic> <handler>

It creates a more consistent treatment of handlers, such that they are always referenced in the scope of a topic.

desa · 2017-03-14T21:25:04Z

👍

desa · 2017-03-14T21:35:24Z

Final thought. Maybe this is what you were already thinking, but what about something like

kapacitor define-topic-handler <topic id> <handler id> -config <file>

with

kind: slack
options:
  username: What

nathanielc · 2017-03-14T22:58:04Z

@desa I have pushed an updated CLI, could you look it over and see how UX is?

desa · 2017-03-15T06:57:31Z

For some reason github wont let me directly comment on where some of the issues are.

kapacitor/cmd/kapacitor/main.go

Line 1833 in ec9dc79

    
           return fmt.Errorf("cannot list '%s' did you mean 'tasks', 'recordings', 'replays', 'topics', 'handlers' or 'service-tests'?", kind)

Should say topic-handlers

kapacitor/cmd/kapacitor/main.go

Line 1849 in ec9dc79

$ kapacitor delete handlers [topic] [ID or pattern]

kapacitor/cmd/kapacitor/main.go

Line 1868 in ec9dc79

$ kapacitor delete handlers system slack

Should say topic-handlers

kapacitor/cmd/kapacitor/main.go

Line 2002 in ec9dc79

    
           return fmt.Errorf("cannot delete '%s' did you mean 'tasks', 'templates', 'recordings', 'replays', 'topics' or 'handlers'?", kind)

Should say topic-handlers

Just a couple copy issues.

Personally, I think I'd prefer having the handler configuration be specified as a flag, but I don't particularly mind it the way that it currently is.

…a handler

nathanielc · 2017-03-16T15:51:04Z

Personally, I think I'd prefer having the handler configuration be specified as a flag, but I don't particularly mind it the way that it currently is.

I started that way and then reversed it because Go is picky about where flags need to be, so it would always have to be the last or first argument which basically turned it into a positional argument. And it is not an optional argument which a flag implies.

I'll fix the other issues.

…m/google/uuid subrepo: subdir: "vendor/github.com/google/uuid" merged: "6a5e28554" upstream: origin: "https://github.com/google/uuid.git" branch: "master" commit: "6a5e28554" git-subrepo: version: "0.3.0" origin: "???" commit: "???"

nathanielc · 2017-03-17T20:40:01Z

@desa The migration path is ready for testing.

You should be able to

upgrade with any v1.2 handler and have all functionality continue to work.
downgrade back to v1.2 and have everything back to how it was

desa · 2017-03-21T00:02:43Z

@nathanielc Just did a manual test and verified that this process works.

Refactor Alert Service

nathanielc added the in progress label Feb 17, 2017

nathanielc force-pushed the nc-alert-nohandle branch from f6721cf to c324fcb Compare February 23, 2017 17:31

nathanielc changed the title ~~Add no handle collect option to alert topics~~ Add external vs internal handlers Feb 23, 2017

nathanielc force-pushed the nc-alert-nohandle branch 3 times, most recently from 073d874 to 0375e6d Compare March 8, 2017 16:18

nathanielc force-pushed the nc-alert-nohandle branch 3 times, most recently from 0d60091 to f0efb9b Compare March 9, 2017 20:14

nathanielc changed the title ~~Add external vs internal handlers~~ Refactor Alert Service Mar 9, 2017

nathanielc requested a review from desa March 9, 2017 20:17

nathanielc force-pushed the nc-alert-nohandle branch 2 times, most recently from 64cb0c3 to 0085fd9 Compare March 9, 2017 20:50

desa reviewed Mar 10, 2017

View reviewed changes

nathanielc mentioned this pull request Mar 14, 2017

Single Alert message for the batch/stream #892

Open

nathanielc mentioned this pull request Mar 14, 2017

Add match conditions to alert handlers #1259

Closed

4 tasks

add concept of external vs internal handlers

e8e4e50

nathanielc added 5 commits March 15, 2017 14:23

organize alert service into specific interfaces

9bf66a0

separate out HTTP api server from the rest of the service

6a43f80

simplify Topics interface

7943055

remove concept of alert actions, now everything is either a topic or …

8a6b8a2

…a handler

add tests for aggregate and publish handlers

77978cc

nathanielc force-pushed the nc-alert-nohandle branch 2 times, most recently from 6e0ce1e to 0553eab Compare March 15, 2017 20:26

update CLI usage to be consistent about handlers

c19dad6

nathanielc force-pushed the nc-alert-nohandle branch from 0553eab to 64bd6b2 Compare March 16, 2017 15:55

nathanielc added 2 commits March 16, 2017 10:10

switch to using UUIDs directly

bfb7cc8

nathanielc force-pushed the nc-alert-nohandle branch from 64bd6b2 to adb37b7 Compare March 16, 2017 16:15

nathanielc mentioned this pull request Mar 16, 2017

Chain topics #1260

Closed

add match conditions to alert handlers

3850c33

nathanielc force-pushed the nc-alert-nohandle branch from 8b0148d to 9b4b259 Compare March 17, 2017 20:38

add migration logic

1521a8c

nathanielc force-pushed the nc-alert-nohandle branch from 9b4b259 to 1521a8c Compare March 21, 2017 20:52

nathanielc merged commit 1521a8c into master Mar 27, 2017

nathanielc added a commit that referenced this pull request Mar 27, 2017

Merged pull request #1209 from influxdata/nc-alert-nohandle

86457a5

Refactor Alert Service

nathanielc removed the in progress label Mar 27, 2017

nathanielc deleted the nc-alert-nohandle branch April 26, 2017 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Alert Service #1209

Refactor Alert Service #1209

nathanielc commented Feb 17, 2017 •

edited

Loading

desa left a comment •

edited

Loading

desa Mar 10, 2017

desa Mar 10, 2017

nathanielc Mar 13, 2017

nathanielc commented Mar 13, 2017

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017 •

edited

Loading

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017 •

edited

Loading

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017

desa commented Mar 14, 2017

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017

desa commented Mar 15, 2017

nathanielc commented Mar 16, 2017

nathanielc commented Mar 17, 2017

desa commented Mar 21, 2017

Refactor Alert Service #1209

Refactor Alert Service #1209

Conversation

nathanielc commented Feb 17, 2017 • edited Loading

desa left a comment • edited Loading

Choose a reason for hiding this comment

desa Mar 10, 2017

Choose a reason for hiding this comment

desa Mar 10, 2017

Choose a reason for hiding this comment

nathanielc Mar 13, 2017

Choose a reason for hiding this comment

nathanielc commented Mar 13, 2017

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017 • edited Loading

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017 • edited Loading

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017

desa commented Mar 14, 2017

desa commented Mar 14, 2017

nathanielc commented Mar 14, 2017

desa commented Mar 15, 2017

nathanielc commented Mar 16, 2017

nathanielc commented Mar 17, 2017

desa commented Mar 21, 2017

nathanielc commented Feb 17, 2017 •

edited

Loading

desa left a comment •

edited

Loading

nathanielc commented Mar 14, 2017 •

edited

Loading

nathanielc commented Mar 14, 2017 •

edited

Loading