Introduce new writers to API v0.2 #343

LotharSee · 2017-12-18T14:53:55Z

Split the Writer into 3 different writers, writing to 3 new endpoints:

/traces, TraceWriter: new protobuf endpoint. Only contains traces (and soon transactions).
/stats, StatsWriter: similar to the previous AgentPayload but without the traces.
/services, ServiceWriter: similar to before. But now a writer on its own.

They are split as they differ in terms of encoding, compression, replay logic, ...

To make that work properly, there is some heavy rewiring.

The trigger of a flush no longer comes from agent/agent.go. Traces and services are sent to their writer as soon as processed. The concentrator generates and flush StatsBuckets itself to the writer. Then writer are free to send it over the wire when/how they want (allowing future different flush conditions / replay features).
Revamp Endpoint API to be much simpler, simply writing bytes with HTTP headers. Also extract the client/proxy configuration.
New TracePayload is our first protobuf payload. For now it isn't compressed, we should benchmarks options before picking a compression.
Many lines changed around info to make it work with the new metrics / writers.

TODOs before being able to merge it:

figure out encoding for the trace endpoint
heavy real-world testing with compatible server trace api

AlexJF

A little less than halfway but leaving some nitpick comments already.

AlexJF · 2017-12-18T16:30:31Z

agent/agent.go

@@ -60,41 +60,62 @@ type Agent struct {
 // NewAgent returns a new Agent object, ready to be started
 func NewAgent(conf *config.AgentConfig, exit chan struct{}) *Agent {
 	dynConf := config.NewDynamicConfig()
+
+	// inter-component channels
+	rawTraceChan := make(chan model.Trace, 5000) // about 1000 traces/sec for 5 sec


Should we take this opportunity to remove some copies here too (chan *model.Trace)?

model.Trace being a slice it isn't such a big deal.

https://github.com/golang/go/wiki/CodeReviewComments#receiver-type
But it falls under the:

If the receiver is a struct, array or slice and any of its elements is a pointer to something that might be mutating, prefer a pointer receiver, as it will make the intention more clear to the reader.

So it is mostly to make the code more explicit ; I'll leave it as a comment.

True about it just being a slice. Hadn't noticed. Comment was mostly because sampledTraceChan indeed uses *model.Trace and it felt a bit weird to have both styles.

Totally ; I'm not sure yet about what is the best there / I'd rather take enough time to make the right decision. Left it as a comment, it can be a later task to review these questions across the entire Agent.

AlexJF · 2017-12-18T16:37:49Z

agent/concentrator.go

 	}
 	sort.Strings(c.aggregators)
 	return &c
 }

+// Start starts the writer.


starts the concentrator

AlexJF · 2017-12-18T16:40:48Z

agent/main.go

@@ -148,7 +148,7 @@ func runAgent(exit chan struct{}) {

 	if opts.info {
 		if err := info.Info(os.Stdout, agentConf); err != nil {
-			// need not display the error, Info should do it already
+			os.Stdout.WriteString(fmt.Sprintf("failed to print info: %s\n", err))


Less "should" more action 👍 😄

AlexJF · 2017-12-18T16:42:42Z

agent/receiver.go

@@ -71,8 +71,6 @@ type HTTPReceiver struct {
 func NewHTTPReceiver(conf *config.AgentConfig, dynConf *config.DynamicConfig) *HTTPReceiver {
 	// use buffered channels so that handlers are not waiting on downstream processing
 	return &HTTPReceiver{
-		traces:     make(chan model.Trace, 5000), // about 1000 traces/sec for 5 sec


We should be passing these by argument. Technically this is a constructor function so whatever comes out should be working without needing to modify unexported fields (IMO)

Very good point! It both makes the code more explicit and avoid sneaky bugs. Updated.

AlexJF · 2017-12-18T16:45:34Z

agent/sampler.go

-// NewScoreEngine creates a new empty sampler ready to be started
-func NewScoreEngine(conf *config.AgentConfig) *Sampler {
+// NewScoreSampler creates a new empty sampler ready to be started
+func NewScoreSampler(conf *config.AgentConfig) *Sampler {


Again, it kinda feels cleaner to me if we would pass sampled as an argument to the constructors.

AlexJF · 2017-12-18T16:45:45Z

agent/sampler.go

-// NewPriorityEngine creates a new empty distributed sampler ready to be started
-func NewPriorityEngine(conf *config.AgentConfig, dynConf *config.DynamicConfig) *Sampler {
+// NewPrioritySampler creates a new empty distributed sampler ready to be started
+func NewPrioritySampler(conf *config.AgentConfig, dynConf *config.DynamicConfig) *Sampler {


AlexJF · 2017-12-18T16:49:14Z

agent/sampler.go

+			switch s.engine.(type) {
+			case *sampler.ScoreEngine:
+				info.UpdateSamplerInfo(info.SamplerInfo{EngineType: engineType, Stats: stats, State: state})
+			case *sampler.PriorityEngine:


Would be cleaner as methods of engine. Let method resolution do its work.

talwai · 2017-12-18T22:58:54Z

model/stats_payload.go

+
+// StatsPayload represents the payload to be flushed to the stats endpoint
+type StatsPayload struct {
+	HostName string        `json:"hostnamed"`


typo hostname ?

talwai · 2017-12-18T23:09:59Z

writer/datadog_endpoint.go

+		return err
+	}
+
+	// Set API key in the header and issue the request


i realize this is copy-paste, but the comment disagrees with the code. We still send the api key as a query param rather than through headers.

Since the code is being touched maybe leave a TODO to really move it to HTTP headers. The new backend endpoints should support this as the main way to auth.

Right, I moved it to HTTP headers (the same as https://github.com/DataDog/datadog-agent/blob/master/pkg/forwarder/forwarder.go#L66 )

Since the code is now more readable, we can also remove examples from doc strings and only rely on tests or the more readable template.

AlexJF · 2017-12-20T11:14:43Z

agent/receiver_test.go


+	receiver.traces = rawTraceChan
+	receiver.services = serviceChan


Redundant with arguments being passed to constructor.

AlexJF

Went through it all. Besides the nitpick, only other observation is that we seem to have a lot of duplication around the writers (the whole start/stop/run, switch waiting for data, flushing, etc.). Perhaps having a BaseWriter composed into all of the writers and have the BaseWriter encode that common logic could work? Anyway, I can also explore this as part of the logic for the async retry stuff :)

LGTM

AlexJF · 2017-12-20T11:20:11Z

model/trace.proto

+	uint64 traceID = 1;
+	repeated Span spans = 2;
+	int64 startTime = 6;
+    int64 endTime = 7;


Nit: extra space

LotharSee · 2017-12-20T12:39:03Z

Yes that's the goal: I tried to keep writers as independent/uncoupled as possible ; that way it won't constraint us when we will have more writer-specific code (different buffering, retrying, logging, metrics, ...). After that we will be able to see what's in common and share it! (even if, sometimes, we can't avoid some dupe in Go).

LotharSee · 2018-01-04T13:37:21Z

writer/service_writer.go

+	}
+
+	headers := map[string]string{
+		languageHeaderKey: strings.Join(info.Languages(), "|"),


Currently updating our backend, as this header will be enough on the stats payload, not needed for service and traces.

LotharSee requested review from AlexJF and talwai December 18, 2017 14:53

AlexJF reviewed Dec 18, 2017

View reviewed changes

talwai reviewed Dec 18, 2017

View reviewed changes

LotharSee added 2 commits December 19, 2017 09:56

Redirect traces to new API endpoint

7e355ee

Improve info code and support new trace writer

1cad4ac

LotharSee force-pushed the benjamin/new-writers branch 2 times, most recently from e5bbbf1 to 2575975 Compare December 19, 2017 09:53

LotharSee added 6 commits December 19, 2017 11:33

Add stats and service writer

5a1a93b

Update lint configuration

5414a48

Remove legacy writer code

bc417e0

Make channels struct constructor parameters

a830c92

Extract text from info tests

8a62fbf

Since the code is now more readable, we can also remove examples from doc strings and only rely on tests or the more readable template.

Set apiKey as a header

1e367ac

LotharSee force-pushed the benjamin/new-writers branch from 5ae1eb1 to 1e367ac Compare December 19, 2017 10:33

AlexJF reviewed Dec 20, 2017

View reviewed changes

model/trace.proto Outdated

uint64 traceID = 1;

repeated Span spans = 2;

int64 startTime = 6;

int64 endTime = 7;

Copy link

AlexJF Dec 20, 2017

Choose a reason for hiding this comment

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

Nit: extra space

Clean minor nitpicks

33b1bab

LotharSee force-pushed the benjamin/new-writers branch from 57570be to 33b1bab Compare December 20, 2017 16:07

AlexJF mentioned this pull request Dec 22, 2017

Async flushing of writer payloads with jittery exponential retrying #344

Merged

talwai force-pushed the benjamin/new-writers branch from 45ed17a to 33b1bab Compare January 3, 2018 16:59

talwai approved these changes Jan 3, 2018

View reviewed changes

LotharSee commented Jan 4, 2018

View reviewed changes

LotharSee merged commit 33b1bab into master Jan 11, 2018

dtilghman deleted the benjamin/new-writers branch January 12, 2018 05:18

palazzem added this to the 5.21.1 milestone Jan 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new writers to API v0.2 #343

Introduce new writers to API v0.2 #343

LotharSee commented Dec 18, 2017 •

edited

Loading

AlexJF left a comment

AlexJF Dec 18, 2017

LotharSee Dec 19, 2017

AlexJF Dec 19, 2017

LotharSee Dec 19, 2017

AlexJF Dec 18, 2017

AlexJF Dec 18, 2017

AlexJF Dec 18, 2017

LotharSee Dec 19, 2017

AlexJF Dec 18, 2017

AlexJF Dec 18, 2017

AlexJF Dec 18, 2017

talwai Dec 18, 2017

talwai Dec 18, 2017

LotharSee Dec 19, 2017

AlexJF Dec 20, 2017

AlexJF left a comment

AlexJF Dec 20, 2017

LotharSee commented Dec 20, 2017

LotharSee Jan 4, 2018


		receiver.traces = rawTraceChan
		receiver.services = serviceChan

Introduce new writers to API v0.2 #343

Introduce new writers to API v0.2 #343

Conversation

LotharSee commented Dec 18, 2017 • edited Loading

AlexJF 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexJF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LotharSee commented Dec 20, 2017

Choose a reason for hiding this comment

LotharSee commented Dec 18, 2017 •

edited

Loading