[wip] start maintaining an internal count of time series produced per metric #246

kiran-stripe · 2017-09-07T20:30:42Z

Summary

Maintain an internal count of time series produced by metric, which we then flush out to sinks.

Still to do:

Ensure that the memory consumption is within bounds (and tune the precision)
Determine the cardinality width of each type of metric and ensure that we account for this correctly
Determine whether we'd only want to flush the top k highest metrics to Datadog

Motivation

This lays down the groundwork for a few features:

automatic cardinality management! (We could catch high-card metrics earlier!)
this is now feasible because of the hyperloglogplus upgrade (Added a more performant Hyperloglog. #190)

Test plan

Automated tests

Rollout/monitoring/revert plan

deploy veneur

ChimeraCoder · 2017-09-07T20:31:31Z

samplers/samplers.go

@@ -72,6 +74,101 @@ type JSONMetric struct {
 	Value []byte `json:"value"`
 }

+const CardinalityCountName = "cardinality.count"


I'd recommend adding comments explaining the difference here

ChimeraCoder · 2017-09-07T20:34:47Z

samplers/samplers.go

+// is unique across all types)
+func (cc *CardinalityCount) Sample(name string, joinedTags string) {
+	if _, present := cc.MetricCardinality[name]; !present {
+		hll, _ := hyperloglog.NewPlus(18)


can we extract 18 into a const? we can keep it unexported for now

ChimeraCoder · 2017-09-07T20:36:29Z

samplers/samplers.go

+// Export groups the CardinalityCount struct into sub structs, grouped by what
+// their metric consistently hashes to
+func (cc *CardinalityCount) ExportSets() ([]JSONMetric, error) {
+	jsonMetrics := make([]JSONMetric, 0, len(cc.MetricCardinality))


Yes! Thank you for pre-make-ing! :D

ChimeraCoder · 2017-09-07T20:37:36Z

samplers/samplers.go

+	jsonMetrics := make([]JSONMetric, 0, len(cc.MetricCardinality))
+	for metricName, hll := range cc.MetricCardinality {
+
+		buf := new(bytes.Buffer)


nit: I'm not much of a fan of new, and it's generally avoided style-wise (for no particularly good reason) in Go projects.

In this case, the zero value would work if we then pass &buf everywhere; we could also do buf := &bytes.Buffer{}.

ChimeraCoder · 2017-09-07T20:41:17Z

samplers/samplers.go

+		err := encoder.Encode(map[string]*hyperloglog.HyperLogLogPlus{metricName: hll})
+		if err != nil {
+			// TODO (kiran): do we return an array of metrics, instead?
+			log.Printf("failed to export cardinality count for metric name:%s, error:%v", metricName, err)


At the moment, we don't use logrus (and therefore our Sentry hooks) here. Ideally, this should go to Sentry, though that'll require a little more work to configure, since this is in a subpackage.

from IRL: we're going to make a custom error type to return to the caller here, and have the caller emit to Sentry via logrus, instead

ChimeraCoder · 2017-09-07T20:46:39Z

samplers/samplers.go

+	cc.MetricCardinality[name].Add(hasher)
+}
+
+// Export groups the CardinalityCount struct into sub structs, grouped by what


comment about 'sub strict'

ChimeraCoder · 2017-09-07T20:47:50Z

samplers/samplers.go

+
+	err := decoder.Decode(&decodedMap)
+	if err != nil {
+		panic(err)


return instead of panic?

ChimeraCoder · 2017-09-07T20:55:08Z

samplers/samplers_test.go

+	}
+
+	// // HLLs are approximate, and we've seen error of +-1 here in the past, so
+	// // we're giving the test some room for error to reduce flakes


Can we extract the margin of error into a constant?

You can use InDelta here!

ChimeraCoder · 2017-09-07T20:58:30Z

worker_test.go

@@ -78,3 +80,25 @@ func TestWorkerImportHistogram(t *testing.T) {
 	wm := w.Flush()
 	assert.Len(t, wm.histograms, 1, "number of flushed histograms")
 }
+
+func TestWorkerImportCardinality(t *testing.T) {
+	w := NewWorker(1, nil, logrus.New())


I think we can actually do log here instead of logrus.New(), because it will pick up the global one initialized in server.go

cory-stripe · 2017-09-08T14:57:42Z

samplers/samplers.go

+}
+
+func (e *CardinalityExportError) Error() string {
+	return "oh no"


cory-stripe · 2017-09-08T15:01:00Z

samplers/samplers.go

+	for metricName, hll := range cc.MetricCardinality {
+
+		buf := &bytes.Buffer{}
+		encoder := gob.NewEncoder(buf)


We recently had #220 show up which got us into a bit of trouble because the transmission of this sort of data isn't versioned or anything. I get a bit worried when I see new gob-encoded things. This is probably not actionable now but something we should consider for the future. Should we version the import endpoint or something?

cory-stripe · 2017-09-08T15:02:05Z

samplers/samplers.go

+
+// cardinalityPrecision is the precision of the HLL that stores cardinality for
+// each metric name.
+const cardinalityPrecision = 18


Should this be configurable? #189 has pointed out that we seem to use an overly "big" HLL @ precision of 18.

cory-stripe · 2017-09-08T15:02:42Z

samplers/samplers.go

+
+// CardinalityCount is a sampler that records an approximate cardinality count
+// by metric name.
+type CardinalityCount struct {


I think it's really cool that you implemented this as a sampler!

cory-stripe · 2017-09-08T15:10:03Z

samplers/samplers.go

+		hll, _ := hyperloglog.NewPlus(cardinalityPrecision)
+		cc.MetricCardinality[name] = hll
+	}
+	hasher := fnv.New64a()


The UDPMetric has a Digest wherein we've already computed the hash of tags + type + name. Rather than rehashing you can just use that. Then your function can be Sample(name string, digest uint32), your map keyed by the digest and you save a hash operation.

cory-stripe · 2017-09-08T15:11:52Z

samplers/samplers.go

+}
+
+// Flush emits the names + cardinality of all the metrics in the map
+// TODO (kiran): investigate whether we'd want to emit only the top k metrics


Could we also emit a counter here based on len(cc.MetricCardinality) to get the total # of unique time series seen?

cory-stripe · 2017-09-08T15:17:12Z

worker.go

@@ -54,6 +57,7 @@ func NewWorkerMetrics() WorkerMetrics {
 		histograms:      make(map[samplers.MetricKey]*samplers.Histo),
 		sets:            make(map[samplers.MetricKey]*samplers.Set),
 		timers:          make(map[samplers.MetricKey]*samplers.Histo),
+		cardinality:     samplers.NewCardinalityCount(),


For posterity maybe note that we can't use a map[MetricKey]HLL here because we need a per-name map not a per-name, per-unique-tag map.

kiran-stripe · 2017-09-18T23:01:26Z

closing this for now -- I think I have a better solution down the road.

kiran added 6 commits August 31, 2017 16:28

start sketching out the cardinality counting metric

d81d132

hook up the sampling

d7affac

start sketching out the combine

7a4bb30

finish fleshing out Combine

a1d32d8

export each of the cardinality counts individually

019e69c

work out the flush for cardinality counts

8585da7

kiran-stripe requested a review from ChimeraCoder September 7, 2017 20:30

ChimeraCoder suggested changes Sep 7, 2017

View reviewed changes

stripe deleted a comment from ChimeraCoder Sep 7, 2017

stripe-ci assigned kiran-stripe Sep 7, 2017

adress some pull request comments

47aa894

cory-stripe reviewed Sep 8, 2017

View reviewed changes

samplers/samplers.go

}

func (e *CardinalityExportError) Error() string {

return "oh no"

Copy link

Contributor

cory-stripe Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cory-stripe reviewed Sep 8, 2017

View reviewed changes

kiran-stripe closed this Sep 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] start maintaining an internal count of time series produced per metric #246

[wip] start maintaining an internal count of time series produced per metric #246

kiran-stripe commented Sep 7, 2017 •

edited

Loading

ChimeraCoder Sep 7, 2017

ChimeraCoder Sep 7, 2017

ChimeraCoder Sep 7, 2017

ChimeraCoder Sep 7, 2017

ChimeraCoder Sep 7, 2017

kiran-stripe Sep 7, 2017

ChimeraCoder Sep 7, 2017

ChimeraCoder Sep 7, 2017

ChimeraCoder Sep 7, 2017

cory-stripe Sep 8, 2017

ChimeraCoder Sep 7, 2017

kiran-stripe Sep 7, 2017

cory-stripe Sep 8, 2017

cory-stripe Sep 8, 2017

cory-stripe Sep 8, 2017

cory-stripe Sep 8, 2017

cory-stripe Sep 8, 2017

cory-stripe Sep 8, 2017

cory-stripe Sep 8, 2017

kiran-stripe commented Sep 18, 2017

[wip] start maintaining an internal count of time series produced per metric #246

[wip] start maintaining an internal count of time series produced per metric #246

Conversation

kiran-stripe commented Sep 7, 2017 • edited Loading

Summary

Motivation

Test plan

Rollout/monitoring/revert plan

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

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

kiran-stripe commented Sep 18, 2017

kiran-stripe commented Sep 7, 2017 •

edited

Loading