Combine global/local logic into one unified flush and parallelize sinks. #292

cory-stripe · 2017-10-18T20:35:17Z

Summary

Improve the Server's Flush work from calling FlushLocal and FlushGlobal, which was a poorly compartmentalized implementation that led to code duplication and confusion.

Motivation

At the end of the Datadog sink PR we are in a world with a single, hardcoded sink. This isn't the intent! We want to convert plugins to sinks and execute all of them.

This patch aims to do just that, but also takes an opportunity to refactor away the FlushLocal and FlushGlobal functions. This was done by first inlining them, removing duplicate code, and then refactoring the special cases away. Notably, the tallyMetrics size calculation.

Notes

This is intended for post 1.7 so >= 1.8
At present DD is still the only metric sink, as such I didn't add any tests of consequence because the existing tests do the thing. We'll add "did all sinks run" as a test when we introduce the next patches.
I added some comments to improve what flush does. Right now it's a rat's nest. This improves the rat's nest incrementally and avoids some tremendously big PR.
I left a half-ass set of context.TODO() sprinkled around because we want to implement a deadline for sinks. I will fix this.

Test plan

Existing tests.

r? @aditya-stripe

ChimeraCoder · 2017-10-19T20:10:04Z

flusher.go

 	span := tracer.StartSpan("flush").(*trace.Span)
 	defer span.Finish()

+	// TODO Move this to an independent ticker routine or something?


So, I don't think we can do this in a separate goroutine, because we want this to be reporting the memory at the time of maximum usage (right before we clear and flush everything).

The alternate way of looking at it: we need to do it in a separate ticker, and choose our interval for that ticker such that we are sampling the underlying, sinusoidal data accurately.

Either way, this would also be a great place to report softnet data, once this is merged: #291

This is separately addressed in #294

ChimeraCoder · 2017-10-19T20:11:48Z

flusher.go


-	go s.metricSinks[0].FlushEventsChecks(span.Attach(ctx), events, checks) // we can do all of this separately
-	go s.flushTraces(span.Attach(ctx))
+	go s.flushTraces(span.Attach(ctx)) // this too!


I think this comment no longer makes sense?

ChimeraCoder · 2017-10-19T20:12:46Z

flusher_test.go

@@ -174,7 +175,7 @@ func testFlushTraceDatadog(t *testing.T, protobuf, jsn io.Reader) {
 	assert.NoError(t, err)

 	server.HandleTracePacket(packet)
-	server.Flush()
+	server.Flush(context.TODO())


I think we actually want context.Background() for all of these, no?

Ha! Yes, I was doing TODO out of habit for a temporary fix when in fact Background was the permanent answer here. :) Thanks!

ChimeraCoder · 2017-10-19T20:13:19Z

flusher.go

+		go func(ms metricSink) {
+			err := ms.Flush(span.Attach(ctx), finalMetrics)
+			if err != nil {
+				log.WithError(err).WithField("sink", ms.Name()).Warn("Error flushin sink")


flushing :)

ChimeraCoder · 2017-10-19T20:16:15Z

flusher.go

+	for _, sink := range s.metricSinks {
+		wg.Add(1)
+		go func(ms metricSink) {
+			err := ms.Flush(span.Attach(ctx), finalMetrics)


Now that this is happening asynchronously, we'll want to make doubly-sure that our interface definition specifies that the Flush method is not allowed to mutate the slice it receives.

Definitely! How do we do that? :)

I'd add it to the interface definition in metric_sink.go.

stripe-ci · 2017-10-19T21:12:56Z

Gerald Rule: Copy Observability on Veneur and Unilog pull requests

cc @stripe/observability
cc @stripe/observability-stripe

cory-stripe · 2017-10-20T22:13:37Z

re-r? @aditya-stripe

ChimeraCoder · 2017-10-23T20:17:22Z

plugin_test.go

@@ -82,7 +82,7 @@ func TestGlobalServerPluginFlush(t *testing.T) {
 		})
 	}

-	f.server.Flush()
+	f.server.Flush(context.TODO())


nit: we also want Background() here too. context.TODO is intended for functions that are not yet fully traced, and for static analysis tools that can track whether a context originates with a TODO or a BACKGROUND.

ChimeraCoder · 2017-10-23T20:17:46Z

One comment. lgtm after that, so feel free to self-approve :)

cory-stripe · 2017-10-25T19:28:52Z

Self-approval post rebase

stripe-ci assigned aditya-stripe Oct 18, 2017

ChimeraCoder suggested changes Oct 19, 2017

View reviewed changes

stripe-ci assigned cory-stripe and unassigned aditya-stripe Oct 19, 2017

cory-stripe force-pushed the cory-metric-sink-cleanup branch from b70830c to 979f3bb Compare October 20, 2017 22:13

ChimeraCoder reviewed Oct 23, 2017

View reviewed changes

cory-stripe added the approved label Oct 23, 2017

cory-stripe added 3 commits October 25, 2017 12:14

Combine global/local logic into one unified flush and parallelize sinks.

d9db44e

Feedback fixes, comments and typos

c688eb5

Document sink's flush

7555ec2

cory-stripe force-pushed the cory-metric-sink-cleanup branch from 4262f3c to 142613d Compare October 25, 2017 19:15

stripe-ci removed the approved label Oct 25, 2017

Update changelog and readme, fix TODO

a137e5c

cory-stripe force-pushed the cory-metric-sink-cleanup branch from 142613d to a137e5c Compare October 25, 2017 19:16

cory-stripe added the approved label Oct 25, 2017

cory-stripe merged commit 3163ea5 into master Oct 25, 2017

cory-stripe mentioned this pull request Nov 3, 2017

Add go-metrics listeners for metric syndication #127

Open

cory-stripe deleted the cory-metric-sink-cleanup branch November 22, 2017 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine global/local logic into one unified flush and parallelize sinks. #292

Combine global/local logic into one unified flush and parallelize sinks. #292

cory-stripe commented Oct 18, 2017

ChimeraCoder Oct 19, 2017

cory-stripe Oct 19, 2017

ChimeraCoder Oct 19, 2017

ChimeraCoder Oct 19, 2017

cory-stripe Oct 19, 2017

ChimeraCoder Oct 19, 2017

ChimeraCoder Oct 19, 2017

cory-stripe Oct 19, 2017

ChimeraCoder Oct 20, 2017

stripe-ci commented Oct 19, 2017

cory-stripe commented Oct 20, 2017

ChimeraCoder Oct 23, 2017

ChimeraCoder commented Oct 23, 2017

cory-stripe commented Oct 25, 2017

Combine global/local logic into one unified flush and parallelize sinks. #292

Combine global/local logic into one unified flush and parallelize sinks. #292

Conversation

cory-stripe commented Oct 18, 2017

Summary

Motivation

Notes

Test 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

stripe-ci commented Oct 19, 2017

cory-stripe commented Oct 20, 2017

Choose a reason for hiding this comment

ChimeraCoder commented Oct 23, 2017

cory-stripe commented Oct 25, 2017