Add an MQTT Alert Handler #1345

timraymond · 2017-04-27T21:20:40Z

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated
Sign CLA (if not already signed)

Connect #1151

Required only if applicable

You can erase any checkboxes below this note if they are not applicable to your Pull Request.

TICKscript Spec updated

nathanielc

@timraymond Thanks! I have made a few comments below, mostly about small things around name convention etc.

There are two larger changes:

The Update method of the MQTT service doesn't appear to be updating the live client connection and it needs to. The basic idea here is that Kapacitor allow configuration to be modified via the API at runtime, so the Update function is in charge of applying those updates and they need to take effect immediately.
The server/server_test.go will need tests added for your new service. How easy is it to mock out an MQTT broker? If its not easy lets discuss ways to test without it.

Thanks again this looks great. 🎉

nathanielc · 2017-04-27T21:31:02Z

pipeline/alert.go

@@ -350,6 +351,10 @@ type AlertNode struct {
 	// tick:ignore
 	TalkHandlers []*TalkHandler `tick:"Talk"`

+	// Send alert to MQTT
+	// tick:ignore
+	MQTTHandlers []*MQTTHandler `tick:"MQTT"`


This will read like this in TICKscript:

|alert() .mQTT()

Which is really odd so I think we should change this and the method name below to Mqtt so that in TICKscript it is all lower case.

nathanielc · 2017-04-27T21:32:19Z

pipeline/alert.go

@@ -942,6 +947,23 @@ func (a *AlertaHandler) Services(service ...string) *AlertaHandler {
 	return a
 }

+// Send alert to an MQTT broker
+// tick:property
+func (a *AlertNode) MQTT() *MQTTHandler {


Can we accept the topic string here in this method? That way it reads like

|alert() .mqtt('topic goes here')

Since there is no other config for the MQTT handler this keeps it simple.

nathanielc · 2017-04-27T21:33:12Z

services/mqtt/config.go

+	// Port of the MQTT Broker
+	Port uint16 `toml:"port" override:"port"`
+
+	ClientID string `toml:"client_id" override:"client_id"`


Kapacitor uses dashes instead of underscores. Can you update this one and the others in this config struct?

nathanielc · 2017-04-27T21:35:26Z

services/mqtt/config.go

+// Broker formats the configured Host and Port as tcp://host:port, suitable for
+// consumption by the Paho MQTT Client
+func (c Config) Broker() string {
+	return fmt.Sprintf("tcp://%s:%d", c.Host, c.Port)


What if the host is an IPv6 address with colon etc? I believe you can use the URL type here to safely create the Broker string without worrying about edge cases.

nathanielc · 2017-04-27T21:36:35Z

services/mqtt/service.go

+	"github.com/influxdata/kapacitor/alert"
+)
+
+type QOSLevel byte


Is QOSLevel a Qualty of Service Level? If so should we name it QoSLevel?

nathanielc · 2017-04-27T21:37:27Z

services/mqtt/service.go

+const DEFAULT_QUIESCE_TIMEOUT time.Duration = 250 * time.Millisecond
+
+type Service struct {
+	Logger *log.Logger


Logger should be private.

nathanielc · 2017-04-27T21:38:48Z

services/mqtt/service.go

+	if c, ok := newConfig[0].(Config); !ok {
+		return fmt.Errorf("expected config object to be of type %T, got %T", c, newConfig[0])
+	} else {
+		s.configValue.Store(c)


How do the changes to the config get propagated to the client?

lol! That would help, wouldn't it?

nathanielc · 2017-04-27T21:39:10Z

services/mqtt/service.go

+
+type HandlerConfig struct {
+	Topic string
+	QOS   QOSLevel


Again QoS is probably a better name here.

nathanielc · 2017-04-27T21:40:05Z

services/mqtt/service.go

+}
+
+func (s *Service) Alert(qos QOSLevel, topic, message string) error {
+	s.client.Publish(topic, byte(qos), false, message) // should retained be configureable?


What does retained do?

Ah good catch. I meant to research this some more. Publishers have no guarantee that messages will be received by a subscriber to a topic, since there might not be any clients listening at the time the message was published. In that case, the message is dropped by the broker. Retained (which is the 3rd param in Publish) sets a flag on the message that causes the broker to hold onto it and deliver it immediately to clients that subsequently subscribe to that topic. The broker holds onto at most one message per topic, so they're useful in describing state ("light switch is on"). Given this, I think the answer to my question is "yes," and it should probably be configureable from within the TICK script.

This entire series of posts has been invaluable: MQTT Essentials Part 8: Retained Messages

nathanielc · 2017-04-27T21:41:07Z

vendor.list

@@ -4,6 +4,7 @@ github.com/cenkalti/backoff
 github.com/davecgh/go-spew
 github.com/dgrijalva/jwt-go v3.0.0
 github.com/dustin/go-humanize
+github.com/eclipse/paho.mqtt.golang


Can you rebase your changes? This file is no longer in use in master. Kapacitor has switched over to using the new golang dep tool. Info on how to use it is in the CONTRIBUTING.md file.

timraymond · 2017-05-02T20:35:16Z

@nathanielc okay, I think this is ready for another look (I see that I need to rebase again though). I moved Topic into the argument for mqtt() at your suggestion. However, I needed to add two more parameters for QoS and Retained, since the more I read, the more it made sense to control those on an alert by alert basis.

Regarding testing, I've come up with three options:

I read through the spec a bit, and I think I can put together a minimal broker that handles connections and published messages for a QoS of 0, which is what we'd need for a minimal test.

Alternatively, we could wrap the client in an interface and substitute a mock. It seems like that should be possible, but I'm not quite sure how to do the substitution with the existing test machinery that's in place. I think this option is the best, since we're not testing that the MQTT client is implemented correctly.

If we don't want to go that route, we could install Mosquitto in the test containers... but that requires everyone that works on Kapacitor to have a broker running if they want to run the tests, which is kind of 💩 .

nathanielc

Just a few more smaller changes.

As for testing if you can mock the client that would be best. Just a package level test at service/mqtt would be enough to start.

nathanielc · 2017-05-03T19:52:11Z

Gopkg.toml

+## what source location any dependent projects specify.
+# source = "https://github.com/myfork/package.git"
+
+


Can you remove these comments?

nathanielc · 2017-05-03T19:54:01Z

services/mqtt/config.go

+
+	ClientID string `toml:"client-id" override:"client-id"`
+	Username string `toml:"username" override:"username"`
+	Password string `toml:"password" override:"password"`


If you update this override:"password" to override:"password,redact" then the config API will redact the password in all relevant API calls.

nathanielc · 2017-05-03T19:56:02Z

services/mqtt/service.go

+
+// QoSLevel indicates the quality of service for messages delivered to a
+// broker.
+type QoSLevel byte


Can you implement the TextUnmarshaler interface on this type so its toml representation is not numeric?

nathanielc · 2017-05-03T19:57:25Z

services/mqtt/service.go

+	// session.
+	opts.SetCleanSession(true)
+
+	s.client = pahomqtt.NewClient(opts)


Since s.client is being modified here you will need to lock access to it or use the atomic.Value type like you do for the config struct.

timraymond · 2017-05-23T15:59:49Z

@nathanielc This should be ready for another look, if you get a chance

nathanielc · 2017-07-21T22:24:22Z

@timraymond I am not quite done (still tracking down one bug), but I have added a commit that makes it so you have multiple mqtt's configured. I am also adding tests which are not quite passing yet but I figured I push this up before I disappear for the weekend. Take a look when you have a sec, no rush.

timraymond

Looks awesome! I just had one question.

timraymond · 2017-07-24T13:55:25Z

services/mqtt/client.go

 )

 // Client describes an immutable MQTT client, designed to accommodate the
 // incongruencies between real clients and mock clients.
 type Client interface {
 	Connect() error
 	Disconnect()
-	Publish(string, byte, bool, string) error
+	Publish(topic string, qos QoSLevel, retained bool, message []byte) error


I didn't realize that you could pass in argument names in an interface declaration. Does that place any requirements on implementors or is it solely a documentation thing?

It just documents the arguments better. The implementation can still use whatever names it wants.

Much of this code was pulled from the guide published here: https://github.com/influxdata/kapacitor/blob/master/alert/HANDLERS.md . This also adds eclipse/paho.mqtt.golang as a dependency to Kapacitor. This client was chosen primarily because it is the same one that is used in Telegraf. It is actively developed, and is widely used by other popular projects in the community (e.g. Gobot).

nathanielc · 2017-07-24T16:37:29Z

@timraymond All tests are passing and I have rebased and squashed commits. If it looks good to you then we can merge it.

nathanielc · 2017-07-24T16:39:57Z

@timraymond Can you add a CHANGELOG entry before merging?

timraymond added the in progress label Apr 27, 2017

nathanielc suggested changes Apr 27, 2017

View reviewed changes

timraymond force-pushed the feature/tr-mqtt-alert branch from d24ecd1 to 943164c Compare May 1, 2017 21:56

nathanielc suggested changes May 3, 2017

View reviewed changes

timraymond force-pushed the feature/tr-mqtt-alert branch from a93cd44 to 4fbd33e Compare May 23, 2017 14:30

nathanielc force-pushed the feature/tr-mqtt-alert branch from 54f8ee0 to 8f46039 Compare July 21, 2017 15:41

timraymond commented Jul 24, 2017

View reviewed changes

nathanielc force-pushed the feature/tr-mqtt-alert branch from d4fa1a5 to 335784f Compare July 24, 2017 15:58

timraymond and others added 2 commits July 24, 2017 10:36

allow multiple mqtt clients, add tests for alert handling

2ef9adf

nathanielc force-pushed the feature/tr-mqtt-alert branch from 335784f to 2ef9adf Compare July 24, 2017 16:36

nathanielc approved these changes Jul 24, 2017

View reviewed changes

Update CHANGELOG for MQTT Alert Handler [ci skip]

dac5399

timraymond changed the title ~~WIP: Add an MQTT Alert Handler~~ Add an MQTT Alert Handler Jul 24, 2017

timraymond merged commit fc8f75d into master Jul 24, 2017

timraymond deleted the feature/tr-mqtt-alert branch July 24, 2017 19:02

DJFliX mentioned this pull request Sep 26, 2017

Add support to publish RAW alert to rabbitmq #557

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an MQTT Alert Handler #1345

Add an MQTT Alert Handler #1345

timraymond commented Apr 27, 2017 •

edited by nathanielc

Loading

nathanielc left a comment •

edited

Loading

nathanielc Apr 27, 2017

nathanielc Apr 27, 2017

nathanielc Apr 27, 2017

nathanielc Apr 27, 2017

nathanielc Apr 27, 2017

nathanielc Apr 27, 2017

nathanielc Apr 27, 2017

timraymond Apr 28, 2017

nathanielc Apr 27, 2017

nathanielc Apr 27, 2017

timraymond Apr 28, 2017

nathanielc Apr 27, 2017

timraymond commented May 2, 2017

nathanielc left a comment

nathanielc May 3, 2017

nathanielc May 3, 2017

nathanielc May 3, 2017

nathanielc May 3, 2017

timraymond commented May 23, 2017

nathanielc commented Jul 21, 2017

timraymond left a comment

timraymond Jul 24, 2017

nathanielc Jul 24, 2017

nathanielc commented Jul 24, 2017

nathanielc commented Jul 24, 2017

		## what source location any dependent projects specify.
		# source = "https://github.com/myfork/package.git"

Add an MQTT Alert Handler #1345

Add an MQTT Alert Handler #1345

Conversation

timraymond commented Apr 27, 2017 • edited by nathanielc Loading

Required for all non-trivial PRs

Required only if applicable

nathanielc left a comment • 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

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

timraymond commented May 2, 2017

nathanielc left a 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

timraymond commented May 23, 2017

nathanielc commented Jul 21, 2017

timraymond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc commented Jul 24, 2017

nathanielc commented Jul 24, 2017

timraymond commented Apr 27, 2017 •

edited by nathanielc

Loading

nathanielc left a comment •

edited

Loading