Add error counters to every node type #1175

desa · 2017-02-06T22:58:07Z

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated

nathanielc

The general framework looks good as well as the nodes you have already done.

So far I like what you have done where if an error count already exists you just double count it, once in the generic error counter and once in the specific counter. Let's continue along that path for now.

nathanielc · 2017-02-07T16:53:33Z

alert/HANDLERS.md

@@ -208,6 +208,7 @@ Add the following snippet to the `service.go` file.
 // Handle takes an event and posts its message to the Foo service chat room.
 func (h *handler) Handle(event alert.Event) {
 	if err := h.s.Alert(h.c.Room, event.State.Message); err != nil {
+		h.incrementErrorCount()


This doesn't look right as the handler is not a node and so will not have the incrementErrorCount method.

Lol. May have went into autopilot and added this. Removed now.

nathanielc · 2017-02-07T16:54:44Z

node.go

@@ -59,6 +60,9 @@ type Node interface {

 	emittedCount() int64

+	// TODO: Better name
+	incrementErrorCount()


I actually think the name is fine.

nathanielc · 2017-02-07T16:55:54Z

node.go

@@ -276,6 +282,11 @@ func (n *node) emittedCount() (count int64) {
 	return
 }

+// node increment error count increments a nodes error_count stat
+func (n *node) incrementErrorCount() {
+	n.statMap.Add(statErrorCount, 1)


To avoid the map look up each time you should keep the errCountVar from above on the node struct and then just increment it directly.

nathanielc · 2017-02-07T16:56:42Z

node.go

@@ -20,6 +20,7 @@ import (

 const (
 	statAverageExecTime = "avg_exec_time_ns"
+	statErrorCount      = "error_count"


I think errors is a better name for the metric but I don't have a strong opinion.

I like errors its consistent with the other nodes that have error counts as well.

desa · 2017-02-07T18:05:36Z

I think I've hit every node that has E!.

The command I'm running to check is

git grep -B 3 -n E!

desa · 2017-02-07T18:06:09Z

I did have a specific question on what to do in this case here

419f612#diff-d410d6dce1320d105297cb9419d065e3R603

desa · 2017-02-08T19:53:35Z

@nathanielc anything else that's missing from here? (other than the rebase and changelog addition)

desa · 2017-02-10T20:06:52Z

@nathanielc any thoughts on what I should do here 419f612#diff-d410d6dce1320d105297cb9419d065e3R603

nathanielc · 2017-02-10T21:10:22Z

join.go

@@ -599,6 +600,8 @@ BATCH_POINT:
 			}
 			b, ok := batch.(models.Batch)
 			if !ok {
+				// TODO: determine what to do here. No reference to node is available, but
+				//       a possible error case exists. cant make call to incrementErrorCount


Pass the reference node down so it can be called.

desa · 2017-02-21T16:28:52Z

@nathanielc Should be good to go. Let me know if there's anything else.

nathanielc

Looks good, just a few small changes requested.

nathanielc · 2017-02-21T16:42:07Z

join.go

@@ -570,7 +571,7 @@ func (js *joinset) JoinIntoPoint() (models.Point, bool) {
 }

 // join all batches the set into a single batch
-func (js *joinset) JoinIntoBatch() (models.Batch, bool) {
+func (js *joinset) JoinIntoBatch(j *JoinNode) (models.Batch, bool) {


I'd rather the JoinNode get passed into the newJoinset function and become a field on the joinset type instead of passing it into each function of the joinset type.

Sounds good.

nathanielc · 2017-02-21T16:43:05Z

k8s_autoscale.go

@@ -17,7 +17,7 @@ import (
 const (
 	statsK8sIncreaseEventsCount = "increase_events"
 	statsK8sDecreaseEventsCount = "decrease_events"
-	statsK8sErrorsCount         = "errors"
+	statsK8sErrorsCount         = "k8s_errors"


Should we just remove the k8s_errors stats and use the one counter?

Yeah, should I do that for the other ones as well? In particular the batch and eval nodes?

nathanielc

Code is good to go. Can you update the CHANGELOG entry to make note that the batch node and eval node error stat name changed and mark it as a BREAKING change.

Add error count to alert node Each instance of `E!` now increments the error count for the alert node. Add error count to batch node Each instance of `E!` now increments the error count. Add error count to combine node Each instance of `E!` now increments the error count for a combine node. Add error count to where node Each instance of `E!` now increments the error count Add error count to derivative node Each instance of `E!` now increments a the error count. Add error count to eval node Each instance of `E!` now increments the node error count. It should be noted that the eval node already tracks its own errors. Add error count to flatten node Each instance of `E!` now increments the nodes error count. Add error count to influxdb_out node Each instance of `E!` now increments the node's error count. Add error count to influxql node Each instance of `E!` now increments the nodes error count. Add error count to join node Note the TODO item. Add errors count to k8s autoscale node Note here that k8s already tracks it's own error count. Add error count to log node Each instance of `E!` now increments the error count Add error count to stream node Each insance of `E!` now increments the nodes error count Remove incrementErrorCount from handler Accidentally added it here. Change errors to k8s_errors in K8sAutoscaleNode Rename error_count to errors for node error count Add node errors to node struct Doing this prevents an unnecessary map lookup each time the error count is updated. Pass JoinNode pointer to joinset JoinIntoBatch To increment the nodes error count, a reference to the node must be passed down. Add entry to changelog Remove batch errors in favor of global node error Remove k8 errors in favor of global errors Remove JoinNode reference from JoinIntoBatch fn

desa added the in progress label Feb 6, 2017

nathanielc reviewed Feb 7, 2017

View reviewed changes

nathanielc reviewed Feb 10, 2017

View reviewed changes

desa force-pushed the md-issue#1170 branch from 88ec7f1 to 3c25c48 Compare February 21, 2017 16:28

nathanielc suggested changes Feb 21, 2017

View reviewed changes

desa force-pushed the md-issue#1170 branch 2 times, most recently from 8d94eb5 to adcd75d Compare February 22, 2017 14:51

nathanielc changed the title ~~WIP: Add error counters to every node type~~ Add error counters to every node type Feb 22, 2017

nathanielc reviewed Feb 22, 2017

View reviewed changes

nathanielc mentioned this pull request Feb 22, 2017

add StateDuration node #1216

Merged

4 tasks

desa force-pushed the md-issue#1170 branch from adcd75d to 33f9997 Compare February 23, 2017 16:17

desa merged commit c581b99 into master Feb 23, 2017

desa deleted the md-issue#1170 branch February 23, 2017 16:25

desa removed the in progress label Feb 23, 2017

desa mentioned this pull request Mar 2, 2017

Add error counters for every node type #1170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error counters to every node type #1175

Add error counters to every node type #1175

desa commented Feb 6, 2017 •

edited

Loading

nathanielc left a comment

nathanielc Feb 7, 2017

desa Feb 7, 2017

nathanielc Feb 7, 2017

nathanielc Feb 7, 2017

nathanielc Feb 7, 2017

desa Feb 7, 2017

desa commented Feb 7, 2017

desa commented Feb 7, 2017

desa commented Feb 8, 2017

desa commented Feb 10, 2017 •

edited

Loading

nathanielc Feb 10, 2017

desa commented Feb 21, 2017 •

edited

Loading

nathanielc left a comment

nathanielc Feb 21, 2017

desa Feb 21, 2017

nathanielc Feb 21, 2017

desa Feb 21, 2017 •

edited

Loading

nathanielc left a comment

Add error counters to every node type #1175

Add error counters to every node type #1175

Conversation

desa commented Feb 6, 2017 • edited Loading

Required for all non-trivial PRs

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desa commented Feb 7, 2017

desa commented Feb 7, 2017

desa commented Feb 8, 2017

desa commented Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

desa commented Feb 21, 2017 • edited Loading

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

desa Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

nathanielc left a comment

Choose a reason for hiding this comment

desa commented Feb 6, 2017 •

edited

Loading

desa commented Feb 10, 2017 •

edited

Loading

desa commented Feb 21, 2017 •

edited

Loading

desa Feb 21, 2017 •

edited

Loading