WIP: Add cardinality stat to each node type. #1221

desa · 2017-02-23T16:09:23Z

This PR introduces a new stat cardinality that tracks the number of groups that exist for a given node for each relevant node type.

Add new tests for cardinality tracking
Fill out k8s cardinality test

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated

nathanielc

Should we set the stat in the node.go base type so that all nodes have the counter but it remains zero by default. Then each node can replace the value with their correct IntFuncGague?

Not sure that is a great idea but I want to make sure nodes are always consistent when it comes to having these standard stats.

nathanielc · 2017-02-23T17:03:31Z

expvar/expvar.go

@@ -46,6 +46,26 @@ func (v *Int) IntValue() int64 {
 	return atomic.LoadInt64(&v.i)
 }

+// IntGauge is a 64-bit integer variable that satisfies the expvar.Var interface.
+type IntFuncGauge struct {
+	ValueF func() int


Should this be func() int64 so that the function itself can handle the casting?

nathanielc · 2017-02-23T17:04:09Z

expvar/expvar.go

@@ -46,6 +46,26 @@ func (v *Int) IntValue() int64 {
 	return atomic.LoadInt64(&v.i)
 }

+// IntGauge is a 64-bit integer variable that satisfies the expvar.Var interface.
+type IntFuncGauge struct {


Can you update the comment for the name IntFuncGague?

desa · 2017-02-23T17:45:55Z

My only hesitation for putting it on the node is that its not relevant for all nodes. I suppose having a cardinality=0 for those nodes isn't that bad though.

I'll do that

desa · 2017-02-27T22:19:49Z

@nathanielc assuming that latest commit goes green, it should be in a reviewable state.

There's a bit of ugliness around setting the n.nodeCardinality.ValueF because of a race.

nathanielc

How did you solve the race in setting ValueF? I have an idea of how it could work. I commented below.

nathanielc · 2017-02-28T17:06:46Z

node.go


-	nodeErrors *kexpvar.Int
+	nodeErrors      *kexpvar.Int
+	nodeCardinality *kexpvar.IntFuncGauge


I was thinking that this would not be a struct level field. Rather in the init method you would set a *kexpvar.Int var that is always zero. Then in each node they would call n.statsMap.Set(statCardinalityGague, <create their own IntFuncGague>) this way you avoid the race but still have a default 0 value for nodes that are always zero.

There is a tiny bit of waste creating the initial kexpvar.Int value since in most cases it is replaced but the GC will take care of that.

That works and avoids the locking nonsense. I'll do that.

@nathanielc fixed.

nathanielc · 2017-03-02T18:04:18Z

alert.go

@@ -558,7 +571,10 @@ func (a *AlertNode) runAlert([]byte) error {
 			var highestPoint *models.BatchPoint

 			var currentLevel alert.Level
-			if state, ok := a.states[b.Group]; ok {
+			a.cardinalityMu.RLock()


Since this is common and references a field on the alert node I think it would be best to wrap this into a small function.

I think this pattern is pretty common throughout, should I add it to the other nodes that have this pattern as well?

nathanielc · 2017-03-02T18:06:47Z

combine.go

@@ -60,6 +65,14 @@ func (t timeList) Less(i, j int) bool { return t[i].Before(t[j]) }
 func (t timeList) Swap(i, j int)      { t[i], t[j] = t[j], t[i] }

 func (n *CombineNode) runCombine([]byte) error {
+	valueF := func() int64 {
+		n.cardinalityMu.RLock()


Should we name this expressionsByGroupMu? I have typically named mutexes using the convention Mu so that it is obvious, or just mu if it is global to the entire struct.

Thoughts? Maybe we leave the name alone but above in the struct place the cardinalityMu next to the things being locked so its clear.

I like the idea of naming it after the thing its locking. The reason I named it cardinalityMu was so that it would be universal across each of the nodes.

nathanielc · 2017-03-02T18:17:04Z

sample.go

@@ -73,10 +85,14 @@ func (s *SampleNode) shouldKeep(group models.GroupID, t time.Time) bool {
 		keepTime := t.Truncate(s.duration)
 		return t.Equal(keepTime)
 	} else {
+		s.cardinalityMu.RLock()


Seems like we should just get the write lock for the whole section here.

This commit only tracks JoinNode.groups, it might be worthwhile to add stats for the following: // Buffer for caching points that need to be matched with specific points. matchGroupsBuffer map[models.GroupID][]srcPoint // Buffer for caching specific points until their match arrivces. specificGroupsBuffer map[models.GroupID][]srcPoint

We could consider finer grained locking inside of evalExpr, but I figured grabbing a write lock around the whole thing would be fine. + k.cardinalityMu.Lock() newReplicas, err := k.evalExpr(state.current, group, k.k.Replicas, k.replicasExprs, k.replicasScopePool, t, fields, tags) + k.cardinalityMu.Unlock()

nathanielc · 2017-03-03T18:15:24Z

alert.go

@@ -580,7 +591,8 @@ func (a *AlertNode) runAlert([]byte) error {
 			var highestPoint *models.BatchPoint

 			var currentLevel alert.Level
-			if state, ok := a.states[b.Group]; ok {
+			state, ok := a.getAlertState(b.Group)
+			if ok {


This can go back inside the if statement now.

nathanielc · 2017-03-03T18:22:29Z

alert.go

@@ -488,7 +498,8 @@ func (a *AlertNode) runAlert([]byte) error {
 				return err
 			}
 			var currentLevel alert.Level
-			if state, ok := a.states[p.Group]; ok {
+			state, ok := a.getAlertState(p.Group)


And this one too.

desa added the in progress label Feb 23, 2017

nathanielc reviewed Feb 23, 2017

View reviewed changes

desa force-pushed the md-issue#1173 branch from c2976e4 to 6bf7670 Compare February 23, 2017 18:53

desa added review and removed in progress labels Feb 28, 2017

nathanielc suggested changes Feb 28, 2017

View reviewed changes

nathanielc suggested changes Mar 2, 2017

View reviewed changes

desa added 21 commits March 2, 2017 15:45

Add IntFuncGauge to expvar package

97b969a

Add cardinality stat to AlertNode

f6c3639

Add cardinality stat to WindowNode

fe62376

Add cardinality stat to CombineNode

0e6a4b5

Add cardinality stat to DerivativeNode

3d99a26

Add cardinality count to EvalNode

09bf19d

Add cardinality stat to FlattenNode

f3fb481

Add cardinality count to HTTPOutNode

bc447fc

Add cardinality stat for streaming InfluxQL node

d677cbf

Add cardinality stat to SampleNode

90ef5f4

Add cardinality stat for WhereNode

a8d27c9

Change IntFuncGauge type signature to use int64

06a96da

Define nodeCardinality stat on all nodes

bb3c2e5

Fix bug that reassigned the nodeCardinality

a4bbdb9

Change cardinality stat to working_cardinality

bf4ee90

Add testStreamerCardinality function

005f88b

Add derivative cardinality test

a61965e

Add cardinality stat to group by node

b7a5bf5

Add cardinality tests for various node types

359fe04

desa added 15 commits March 2, 2017 15:45

Fix clock issue with running task in test

d169a1c

Add cardinality tests

abbf09a

Add working_cardinality to CHANGELOG

0c1d7a4

Fix data race in setting nodeCardinality.ValueF

a50e721

Fix k8sAutoscale cardinality test

3b815bc

Remove nodeCardinality and mutex from node struct

ed190fa

Remove nodeCardinality from node struct

55840df

Add a getAlertState method that looks up the state

8526f28

Change cardinalityMu to statesMu on alert node

381955d

Change cardinalityMu to expressionsByGroupMu

b6a7a5e

Change cardinalityMu to expressionsByGroupMu in ev

b2edeb1

Change cardinalityMu to groupsMu in join node

7b60c8c

Change cardinalityMu to replicasExprsMu in k8s node

69ee032

Change cardinalityMu to countsMu in sample node

f97f8e4

Change RLock to a Lock in shouldKeep in sample node

32d1502

desa force-pushed the md-issue#1173 branch from 190000e to 32d1502 Compare March 2, 2017 20:46

nathanielc suggested changes Mar 3, 2017

View reviewed changes

Fold getAlertState into conditional

e9c0658

nathanielc approved these changes Mar 6, 2017

View reviewed changes

desa merged commit 20627b3 into master Mar 6, 2017

desa deleted the md-issue#1173 branch March 6, 2017 16:26

desa removed the review label Mar 6, 2017

desa mentioned this pull request Mar 6, 2017

Revert "WIP: Add cardinality stat to each node type." #1240

Merged

desa restored the md-issue#1173 branch March 6, 2017 16:27

desa mentioned this pull request Aug 3, 2017

Track cardinality for each node in a task #1173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add cardinality stat to each node type. #1221

WIP: Add cardinality stat to each node type. #1221

desa commented Feb 23, 2017 •

edited

Loading

nathanielc left a comment

nathanielc Feb 23, 2017

desa Feb 23, 2017

nathanielc Feb 23, 2017

desa commented Feb 23, 2017 •

edited

Loading

desa commented Feb 27, 2017

nathanielc left a comment

nathanielc Feb 28, 2017

desa Feb 28, 2017

desa Feb 28, 2017

nathanielc Mar 2, 2017

desa Mar 2, 2017

nathanielc Mar 2, 2017

desa Mar 2, 2017

nathanielc Mar 2, 2017

desa Mar 2, 2017

nathanielc Mar 3, 2017

nathanielc Mar 3, 2017

desa Mar 3, 2017

WIP: Add cardinality stat to each node type. #1221

WIP: Add cardinality stat to each node type. #1221

Conversation

desa commented Feb 23, 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

desa commented Feb 23, 2017 • edited Loading

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

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

desa commented Feb 23, 2017 •

edited

Loading

desa commented Feb 23, 2017 •

edited

Loading