writer: split stats writer payloads based on number of entries. by vlad-mh · Pull Request #432 · DataDog/datadog-trace-agent

vlad-mh · 2018-06-05T12:43:20Z

Adds a splitting logic in the stats writer in order never to exceed a pre-configured maximum number of "entries" in a stats payload.

Doing so ensures we don't hit the maximum payload size of the Datadog trace intake API.

The first commit is just a cleanup of the stats writer code, and some added comments.

The second commit adds the configuration value max_entries_per_payload. This value should be tweaked very cautiously. Setting it too low will have the agent send much more payloads than it should when flushing, which will impact performances and will hit the backend significantly more. Setting the value too high, and the payloads might end up being rejected due to their size.

The third commit is the actual splitting logic + unit tests.

gbbr · 2018-06-06T12:47:51Z

@vlad-mh can I review or is it still WIP?

vlad-mh · 2018-06-06T12:48:38Z

@gbbr ready for review

LotharSee · 2018-06-06T12:55:26Z

+
+	// 3. Iterate over each entries of each stats. Add the entry
+	//    to one of the payload container mappings, in a round
+	//    robin fashion. In some edge cases, we can end up having


In which case can we have that? I don't see how that could happen as the concentrator is already aggregating?

Indeed, it seemed like this couldn't happen with out current implementation. But it did show up right away in unit tests due to how the fixtures are done, and I don't think we should rely too much on the concentrator's logic. I figured a bit of pain now would go a long way in terms of reusability and potential sneaky bug.

gbbr

This is looking really nice @vlad-mh. I really like the clean-up and all the nice explanations. It makes the code very easy to follow.

I've left a lot of comments but besides the ones in the test file, they are very small nitpicks with regards to mostly variable naming and documentation.

The stats/bucketing code is a bit too cryptic. Maybe we can work together to make it a bit easier to reason about. The comments you've added there definitely already provide a lot of value. Thank you for doing that.

gbbr · 2018-06-06T13:09:44Z


-// DefaultStatsWriterConfig creates a new instance of a StatsWriterConfig using default values.
+// DefaultStatsWriterConfig creates a new instance of a
+// StatsWriterConfig using default values.


It should be fine to have this comment one one line... Do you have a set limit you want us to enforce? I was thinking 120 should be ok.

Yeah my editor is set to 80.. I'll change to 120

gbbr · 2018-06-06T13:12:45Z

 	return StatsWriterConfig{
-		UpdateInfoPeriod: 1 * time.Minute,
-		SenderConfig:     DefaultQueuablePayloadSenderConf(),
+		MaxEntriesPerPayload: 12000,


How would you feel about moving this default value into a constant, and the comment above as documentation for that constant? I think that would go great together and make everything clearer. As the comment stands now, it's a bit unclear at first what it's referring too because it's not referring to the direct next line.

makes sense, will do

gbbr · 2018-06-06T13:15:08Z

+	BaseWriter
+
+	// InStats is the stream of stat buckets to send out.
+	InStats <-chan []model.StatsBucket


Since this is the only input channel, let's call it the usual in. It will pair nicely with the same naming of the trace writer's "in" channel, like we have now (link here). It needs not be an exported field as it is not used anywhere. A lot of the identifiers in the agent are exported but aren't used in foreign packages.

Had the same train of thought but didn't want to extend the scope of the PR to other files, as this is a publicly exported field and the PR is already big. If you insist, I can?

Ah, my bad. I thought you added this field. You can leave it as is. If you were to change it, it would be super awesome and nice IMO but up to you. You don't have to!

gbbr · 2018-06-06T13:16:42Z

-	env      string
-	conf     writerconfig.StatsWriterConfig
-	InStats  <-chan []model.StatsBucket
+	// env is environment this agent is configured with, to be


// env specifies the environment that this agent is configured to. It will be // sent as part of the stats payload.

Please add a blank line above this comment, to keep it consistent.

gbbr · 2018-06-06T13:17:11Z

+	// eye on things.
+	info info.StatsWriterInfo
+
+	// hostName is the resolved host name on which the agent is


s/is/specifies

gbbr · 2018-06-06T13:41:28Z

+	nbStats := 0
+	nbEntries = 0
+	payloads := make([]*model.StatsPayload, 0, nbPayloads)
+	for _, pm := range pMaps {


How about:

for _, b := range buckets {

gbbr · 2018-06-06T13:42:21Z

+	assert := assert.New(t)
+
+	// Common case, no duplicate entries
+	{


Instead of creating these artificial closures, let's remove the comments and use sub-tests.

was looking for them and didn't find in the doc, as I was looking for "case" and not sub-test :D perfect, thanks!

gbbr · 2018-06-06T13:43:00Z

+			assert.Equal(expectedNbEntriesByPayload[i], len(payloads[i].Stats[0].Counts))
+		}
+
+		// Assert the counts match


Let's not leave commented out code.

gbbr · 2018-06-06T13:43:19Z

+		actualCounts := make(map[string]float64, expectedNbEntries)
+		for _, p := range payloads {
+			for _, s := range p.Stats {
+				for ekey, e := range s.Counts {


Please use k, v, k, c or key, count, etc.

gbbr · 2018-06-06T13:45:02Z

+
+		// Remove duplicates so that we have a predictable
+		// state. In another case we'll test with duplicates.
+		expectedNbEntries := 0


This is duplicated in two tests. IMHO we should try and remove duplication. This test is too long and complex. It should be short and easy to reason about. If possible, let's create some helper methods.

If we are using a fixture method and then iterating over it to change it, that means that we're not using the right method. Let's instead create helper methods for this. Maybe you can add something new to fixtures or just keep it in here.

ufoot

So I was starting to review this and if we split payloads it's the right thing to do I'd say. But how about not splitting at all, but when we detect that we have enough stuff (we could figure that out here maybe:

datadog-trace-agent/cmd/trace-agent/concentrator.go

Line 113 in 416acd3

}

) putting an event in a bufferFull channel which we'd read here:

datadog-trace-agent/cmd/trace-agent/concentrator.go

Line 72 in 416acd3

case <-flushTicker.C:

so we could just case bufferFull: c.Flush(). This would save all the splitting code, and also avoid cluttering the memory with temporary things. In theory this could be broken if one trace generates enough entries that we hit the limit. Wonder if this happens in practice. Anyway both approaches (flushing when we hit the limit and splitting) are valid are orthogonal so we might implement both.

ufoot · 2018-06-06T13:31:03Z

 	close(w.exit)
 	w.exitWG.Wait()
+
+	// Closing the base writer, among other things, will close the


nitpick s/Closing/Stopping/g ? Or else we rename the method .Close() ;)

ufoot · 2018-06-06T13:34:26Z

-	// If no stats, we can't construct anything
-	if numStats == 0 {
+	payloads, nbStatBuckets, nbEntries := w.buildPayloads(stats, w.conf.MaxEntriesPerPayload)
+	if len(payloads) == 0 {


Well, I'd keep that test as the first test in the func and return if len(stats) == 0. Pretty much the same, but unless we have a serious bug, if len(stats) > 0 then len(payloads) > 0 so let's quit as soon as possible, and we can know it by just testing len(stats) first.

we test for len of stats in buildPayloads right away, so same result.

ufoot · 2018-06-06T15:36:25Z

+		}}, len(stats), nbEntries
+	}
+
+	nbPayloads := nbEntries / maxEntriesPerPayloads


I tend to prefer nbPayloads := ((nbEntries - 1) / maxEntriesPerPayloads) + 1 which avoid a test, but your version might be more readable. Just a side note.

gbbr

.

vlad-mh · 2018-06-08T10:19:07Z

Hopefully this version of the PR fits a bit more.

To be clear on the goal: this PR doesn't aim to fix the overall architecture of how we create and flush stats bucket. There might be better ways to architecture it as a whole, and this requires more thought. This PR does, however, try to ensure we don't go above a certain payload size in worse case situations (very high volume services, or high resource cardinality), as the buckets end up being refused by the API, and as a result we have wrong generated metrics.

@ufoot and I chatted IRL about this. My understanding is that the collector relies on time window and having a full view of data during that time window, so we can't trigger a flush earlier than the ticker, based on number of entries. I think overall this logic still holds?

@gbbr I refactored the common code in the tests and used the sub test feature. I went through most of the nits as well.

vlad-mh · 2018-06-08T12:30:01Z

Hopefully the last round of logic changes.

the default value for the max number of entries is set to 0, thus disabling the feature. This will allow us to opt-in when payloads are being rejected by the API and see if it works well.
the last commit adds the "splits" field to the writer's info, and report the number of split the agent is doing as a DD metric.

…ies per payload.

This entry counts the number of split the agent is doing.

This allows us to stay under 1.5MB for our compressed stats payloads.

LotharSee · 2018-06-09T12:09:04Z

+	// 1. Get an estimate of how many payloads we need, based on the total
+	//    number of map entries (i.e.: sum of number of items in the stats
+	//    bucket's count map).
+	//    NOTE: we use the number of items in the count map as the


As curiosity, how is that possible to have a different count of Counts and Distribution?

we will always have more counts than distributions: https://github.com/DataDog/datadog-trace-agent/blob/master/model/statsraw.go#L89

vlad-mh force-pushed the vlad/stats-writer-split-payload branch 4 times, most recently from 027f8b8 to 464698d Compare June 6, 2018 12:25

vlad-mh changed the title ~~[WIP] stats writer: split stats payloads based on number of entries.~~ stats writer: split stats payloads based on number of entries. Jun 6, 2018

LotharSee requested review from LotharSee and gbbr June 6, 2018 12:37

vlad-mh force-pushed the vlad/stats-writer-split-payload branch from 464698d to df80a8c Compare June 6, 2018 12:42

gbbr added this to the 6.3.0 milestone Jun 6, 2018

LotharSee reviewed Jun 6, 2018

View reviewed changes

gbbr changed the title ~~stats writer: split stats payloads based on number of entries.~~ writer: split stats writer payloads based on number of entries. Jun 6, 2018

gbbr reviewed Jun 6, 2018

View reviewed changes

ufoot reviewed Jun 6, 2018

View reviewed changes

vlad-mh force-pushed the vlad/stats-writer-split-payload branch from df80a8c to a52702a Compare June 7, 2018 13:34

gbbr reviewed Jun 7, 2018

View reviewed changes

vlad-mh force-pushed the vlad/stats-writer-split-payload branch from a52702a to 4462fea Compare June 8, 2018 10:06

vlad-mh force-pushed the vlad/stats-writer-split-payload branch from 4462fea to be1a9c7 Compare June 8, 2018 12:28

vlad-mh force-pushed the vlad/stats-writer-split-payload branch from be1a9c7 to 7e0c469 Compare June 8, 2018 12:35

gbbr approved these changes Jun 9, 2018

View reviewed changes

vlad-mh added 5 commits June 9, 2018 14:03

writer: cleanup stats writer code.

6ae8902

writer/config: add max_entries_per_payload config value.

ed4c086

writer: split stats to multiple payloads, based on max number of entr…

389f136

…ies per payload.

writer: add the "splits" entry to the writer info.

eb150cb

This entry counts the number of split the agent is doing.

config/writer: set maxEntriesPerPayload to 12000.

95fe630

This allows us to stay under 1.5MB for our compressed stats payloads.

vlad-mh force-pushed the vlad/stats-writer-split-payload branch from f955fa5 to 95fe630 Compare June 9, 2018 12:03

gbbr approved these changes Jun 9, 2018

View reviewed changes

LotharSee reviewed Jun 9, 2018

View reviewed changes

LotharSee approved these changes Jun 9, 2018

View reviewed changes

vlad-mh merged commit 821cc80 into master Jun 9, 2018

LotharSee deleted the vlad/stats-writer-split-payload branch June 18, 2018 08:26

Uh oh!

Conversation

vlad-mh commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gbbr commented Jun 6, 2018

Uh oh!

vlad-mh commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ufoot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbbr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlad-mh commented Jun 8, 2018

Uh oh!

vlad-mh commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

vlad-mh commented Jun 5, 2018 •

edited

Loading

gbbr left a comment •

edited

Loading