Adding Partitioning to Datadog Sink #88

Areson · 2023-01-03T20:56:59Z

Added options for compression and partitioning to the Datadog sink when the API method is used. When receiving an error related to the payload size being too large the sink will attempt to calculate a possible size that will satisfy the size requirements and re-send the metrics. In the event that it exhausts options and cannot send the metrics the attempt will fail.

Added the option to turn on compression for the API, which allows much larger payloads by default.

daniel-nichter · 2023-01-04T14:03:35Z

docs/v1.0/sinks/datadog.md

+
+## API Considerations
+
+When using the API directly the sink may run into issues when sending large payloads, as the Datadog API [restricts the payload to a maximum of 500Kib](https://docs.datadoghq.com/api/latest/metrics/). The DogStatsD option avoids this as it always sends metrics individually. By default the API option will attempt to automatically adjust the size of the payloads sent when it encounters an error related to a payload being too large. You can also explicitly set the number of metrics to send at once using `api-partition-size` and setting `api-auto-partition` to `false`. You can optionally turn on compression for the API, in which case Datadog will accept any payloads whose uncomressed size is less than 5Mib.


500 KiB -- bytes, not bits, I presume?

daniel-nichter · 2023-01-05T21:06:22Z

docs/v1.0/sinks/datadog.md

@@ -32,4 +32,5 @@ sinks:
    metric-translator: ""
    metric-prefix: ""
    dogstatsd-host: ""
-```
+    api-use-compression: "true"


api-compression (or api-compress if you want to mimic the DD client). Blip doesn't use words like "enable", "use", etc. The only such on/off type word is the consistent prefix disable-....

daniel-nichter · 2023-01-05T21:08:01Z

sink/datadog.go

@@ -104,6 +119,9 @@ func NewDatadog(monitorId string, opts, tags map[string]string, httpClient *http
 			}
 			d.prefix = v

+		case "api-use-compression":
+			d.compress = strings.ToLower(v) == "true"


d.compress = blip.Bool(v)
https://pkg.go.dev/github.com/cashapp/blip#Bool

daniel-nichter · 2023-01-05T21:14:31Z

sink/datadog.go

+		return pErr
+	}
+
+	if err != nil {


This needs to be handled before checking r.StatusCode because err will be a low-level error (e.g. network issue), so there's no *http.Response. Afaik, if err is not nil, then there must be an *http.Response.

daniel-nichter · 2023-01-05T21:23:53Z

sink/datadog.go

+		monitorId:     monitorId,
+		tags:          tagList,
+		resources:     resources,
+		partitionSize: math.MaxInt32,


Let's clarify "partition size" for future engineers who'll have to understand this code. The first confusing thing is partition (size of) what? We've got MAX_PAYLOAD_SIZE which is clear (max payload sizes are a common thing). So what's not clear is that partitionSize is actually something like "metrics per Send request", if I'm not mistaken? Then that plus starting with MaxInt32 makes it seems like we'll have to partition a lot (when needed) because 2^32= ~4 billion, yet we only deal with metrics in the 10s or 1000s.

I can update the name. My thought with starting with MaxInt32 was that we begin with no partitioning of the metrics. We only start to do that if we happen to run into the case where we get a error response back from Datadog indicating the payload is too large. With compression on that is much less likely to happen in general. MaxInt32 is just a nice default to set this to instead of just arbitrarily picking a value I thought was big enough.

daniel-nichter · 2023-01-05T21:26:27Z

sink/datadog.go

 	}

-	return err
+	// Figure out how many metrics can fit inside the max payload, but pad it slightly to control for headers, etc.
+	estPartitionSize := (MAX_PAYLOAD_SIZE - 1000) / size


Here's another example of where partition size is confusing: comment says "how many metrics", but it's calculating a value in bytes (based off MAX_PAYLOAD_SIZE and current payload size). I don't see where code does something like "N metrics = B bytes, so new partition size is something fraction of N".

daniel-nichter · 2023-01-05T21:28:36Z

sink/datadog.go

+
+	if estPartitionSize >= s.partitionSize {
+		// The new partition size is greater than what we have now, so let's reduce it by a small factor to make sure we can send metrics properly
+		estPartitionSize = int(float32(s.partitionSize) * .9)


This comment/code will need some clarification for future engineers. What does "properly" mean? And why 0.9? Maybe something like:

// Reduce partition size by 10% as a heuristic/guess for finding the maximum partition size // (metrics per send) that'll fit within the DD payload size limit

daniel-nichter · 2023-01-06T16:11:14Z

sink/datadog.go

+		}
+
+		blip.Debug("error sending data points to Datadog: %s", err)
+		blip.Debug("error sending data points to Datadog: Http Response - %s", r)


Combine these.

Oh actually, you need a return nil here to handle other error so you can return nil below. I.e. as the code is written now, it's mixing boilerplates, which is confusing. I.e. the bloilerplat,

err := f() if err = nil { return err }

isn't happening.

daniel-nichter · 2023-01-06T16:11:54Z

sink/datadog.go

+		blip.Debug("error sending data points to Datadog: Http Response - %s", r)
+	}
+
+	return err


return nil because the if above already checked that it's not nil, so an explicit return nil is idiomatic to show that func has reached a successful end/return.

By contrast, this kind of return err is used when a func returns the last return val of its last call without checking that return value.

daniel-nichter · 2023-01-06T16:47:35Z

sink/datadog.go

+			oldMaxMetricsPerRequest := s.maxMetricsPerRequest
+
+			// Sample one of the metrics and try caculating a new maxMetricsPerRequest
+			if pErr := s.updateMaxMetricsPerRequest(dp[0], len(dp)); pErr != nil {


Don't use pErr here in a block scope but then again later in another (outer) block scope. They have same special name but are different, which contradicts the normal expectation that any specifically-named var is the same var wherever the specific name occurs. (This is not true for nonspecifically named vars like the ubiquitous err in Go, and other classics like i in loops, etc.)

daniel-nichter · 2023-01-09T16:18:14Z

test/mock/tr.go

+package mock
+
+type Tr struct {
+	TranslateF func(domain, metric string) string


Strike the trailing F so mock field name = func name. (Other mocks in Blip should follow this convention.)

It looks like the other mocks favor using Func rather than just F. Match that then?

Correction (as you noted on Slack): "Func" suffix.

daniel-nichter · 2023-01-09T16:19:37Z

Squash commits please. GitHub does this thing where it generates release notes from git commits, which is super handy when doing releases. So we'll try to squash commits in the project to make cutting releases a little easier.

Added request partitioning to the Datadog sink when the API method is used. When receiving an error related to the payload size being too large the sink will attempt to calculate a possible size that will satisfy the size requirements and re-send the metrics. In the event that it exhausts options and cannot send the metrics the attempt will fail. Added am option manage compression for the API (on by default).

Areson requested a review from daniel-nichter January 3, 2023 20:57

Areson force-pushed the ioberst-dd-sink-message-too-large branch from e69b2bd to 711ad7f Compare January 3, 2023 20:57

daniel-nichter reviewed Jan 4, 2023

View reviewed changes

Areson force-pushed the ioberst-dd-sink-message-too-large branch 2 times, most recently from e1aba3b to 80d352e Compare January 4, 2023 21:12

daniel-nichter reviewed Jan 5, 2023

View reviewed changes

daniel-nichter requested changes Jan 5, 2023

View reviewed changes

daniel-nichter reviewed Jan 5, 2023

View reviewed changes

daniel-nichter reviewed Jan 6, 2023

View reviewed changes

daniel-nichter reviewed Jan 9, 2023

View reviewed changes

daniel-nichter approved these changes Jan 9, 2023

View reviewed changes

Areson force-pushed the ioberst-dd-sink-message-too-large branch from 5bf3a93 to dfbc000 Compare January 9, 2023 16:25

Areson force-pushed the ioberst-dd-sink-message-too-large branch from dfbc000 to a99ec0d Compare January 9, 2023 16:28

Areson merged commit 4f58ee1 into main Jan 9, 2023

Areson deleted the ioberst-dd-sink-message-too-large branch January 9, 2023 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Partitioning to Datadog Sink #88

Adding Partitioning to Datadog Sink #88

Areson commented Jan 3, 2023

daniel-nichter Jan 4, 2023

daniel-nichter Jan 5, 2023

daniel-nichter Jan 5, 2023 •

edited

Loading

daniel-nichter Jan 5, 2023

daniel-nichter Jan 5, 2023

Areson Jan 5, 2023

daniel-nichter Jan 5, 2023

daniel-nichter Jan 5, 2023

daniel-nichter Jan 6, 2023

daniel-nichter Jan 6, 2023

daniel-nichter Jan 6, 2023

daniel-nichter Jan 6, 2023

daniel-nichter Jan 6, 2023

daniel-nichter Jan 9, 2023

Areson Jan 9, 2023

daniel-nichter Jan 9, 2023

daniel-nichter commented Jan 9, 2023


		## API Considerations

		When using the API directly the sink may run into issues when sending large payloads, as the Datadog API [restricts the payload to a maximum of 500Kib](https://docs.datadoghq.com/api/latest/metrics/). The DogStatsD option avoids this as it always sends metrics individually. By default the API option will attempt to automatically adjust the size of the payloads sent when it encounters an error related to a payload being too large. You can also explicitly set the number of metrics to send at once using `api-partition-size` and setting `api-auto-partition` to `false`. You can optionally turn on compression for the API, in which case Datadog will accept any payloads whose uncomressed size is less than 5Mib.

Adding Partitioning to Datadog Sink #88

Adding Partitioning to Datadog Sink #88

Conversation

Areson commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-nichter Jan 5, 2023 • edited Loading

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

daniel-nichter commented Jan 9, 2023

daniel-nichter Jan 5, 2023 •

edited

Loading