statsd: support sending metrics with timestamp. #262

remeh · 2022-07-28T09:33:29Z

Adds two methods GaugeWithTimestamp and CountWithTimestamp in the ClientInterface to support sending gauge and count with timestamp, for users aggregating metrics on their side and who can provide a timestamp for a given metric.

Metrics sent using GaugeWithTimestamp and CountWithTimestamp to the DogStatsD server will be directly sent to the intake with no aggregation done server-side.

If the client is created using WithClientSideAggregation(), no aggregation will be performed on metrics submitted with timestamp, they will be written in the serialization buffer as-is.

Adds two methods in the `ClientInterface` to support sending gauge and count with timestamp `GaugeWithTimestamp` and `CountWithTimestamp`, for users aggregating metrics on their side and who can provide a timestamp for a given metric.

…ization buffer. They are not going through the client aggregator.

statsd/aggregator.go

statsd/buffer.go

hush-hush · 2022-07-29T10:01:28Z

statsd/statsd.go

+	// Gauge measures the value of a metric at a given time.
+	// Even with client side aggregation enabled, there is no aggregation done on a metric
+	// sent using GaugeWithTimestamp: it is written as is in the serialization buffer.
+	GaugeWithTimestamp(name string, value float64, tags []string, rate float64, timestamp time.Time) error
+


Why only gauge and count ? rate/set doesn't make send but we might want to send timing, histogram and distribution with timestamp too, no ?

For now, timed metrics ingestion will only be supported for Gauge and Count (DogStatsD server side and backend side).

I think we should explain that somewhere, maybe the README or the Go doc string.

statsd/statsd.go

hush-hush · 2022-07-29T10:13:21Z

statsd/statsd.go

+// Gauge measures the value of a metric at a given time.
+// Even with client side aggregation enabled, there is no aggregation done on a metric
+// sent using GaugeWithTimestamp: it is written as is in the serialization buffer.
+func (c *Client) GaugeWithTimestamp(name string, value float64, tags []string, rate float64, timestamp time.Time) error {


We should return an error if timestamp is 0 (same for count).

Not sure, that's not an invalid timestamp. A weird one, but not an invalid one. For instance, 1 is also a weird one, you see what I mean?

I think it's an error since go has default value. This means an non-initialized timestamp.
Sending a time.Time at 0 will produce a Unix timestamp of -62135596800.

A gotcha... Also linked to your other question about time.Time vs uint64...
You are right I'll add a test if timestamp.Unix() < 0, WDYT?

I'm trying to understand why 0 (jan 1970) is the limit here.

0 is a valid timestamp that can be represented, separate from the go zero-value.
There is a t.IsZero() that differentiates between the two:
https://go.dev/play/p/njBnvlej3Rf

Dogstatsd parsing correctly handles negative timestamps. src

I think the limiting factor here is the intake according to this comment:

// timestamp for this value in seconds since the UNIX epoch

So my suggestion here is to add a comment about why this is the minimum supported epoch.

BTW - I noticed that the public metrics api says:

Note: Metric timestamps cannot be more than ten minutes in the future or more than one hour in the past.

I assume this doesn't apply to us? Or should we be enforcing that condition here by rejecting timestamps older than 1hr?

Yeah.. We have to draw a limit somewhere in the clients and since the intake has a "minimum timestamp" limit and we already have other places limiting to the unix epoch in the Agent, I think having int(0) as the minimal possible value is speaking to everyone in a client. Let me know if you think I'm wrong, really open to discussion with these topics.

I assume this doesn't apply to us?

It doesn't indeed, a future PR will update the public documentation, with things more related about the "features" than the technical chunks in the client.

I'm trying to understand why 0 (jan 1970) is the limit here.

The idea is to provide a hard-coded limit instead of no limit, even if it has no actual feature meaning here.
I added a mention in the comment to redirect to the feature documentation.

Please report to the Datadog documentation for the maximum age of a metric.

statsd/statsd.go

scottopell · 2022-09-06T18:25:54Z

statsd/statsd.go

+// Gauge measures the value of a metric at a given time.
+// Even with client side aggregation enabled, there is no aggregation done on a metric
+// sent using GaugeWithTimestamp: it is written as is in the serialization buffer.
+func (c *Client) GaugeWithTimestamp(name string, value float64, tags []string, rate float64, timestamp time.Time) error {


I'm trying to understand why 0 (jan 1970) is the limit here.

0 is a valid timestamp that can be represented, separate from the go zero-value.
There is a t.IsZero() that differentiates between the two:
https://go.dev/play/p/njBnvlej3Rf

Dogstatsd parsing correctly handles negative timestamps. src

I think the limiting factor here is the intake according to this comment:

// timestamp for this value in seconds since the UNIX epoch

So my suggestion here is to add a comment about why this is the minimum supported epoch.

BTW - I noticed that the public metrics api says:

Note: Metric timestamps cannot be more than ten minutes in the future or more than one hour in the past.

I assume this doesn't apply to us? Or should we be enforcing that condition here by rejecting timestamps older than 1hr?

scottopell · 2022-09-06T18:26:46Z

statsd/statsd.go

+		return ErrNoClient
+	}
+
+	if timestamp.Unix() < 0 {


Suggested change

if timestamp.Unix() < 0 {

if timestamp.Unix() < minSupportedEpoch {

I first introduced a minValidTimestamp const but it was only used here so I've decided to re-use noTimestamp instead, WDYT?

statsd/statsd.go

scottopell · 2022-09-06T18:36:12Z

statsd/statsd.go

@@ -126,6 +126,9 @@ const (
 	writerWindowsPipe string = "pipe"
 )

+// noTimestamp is used as a value for metric without a given timestamp.
+const noTimestamp = int64(0)


Small edge case here:
In GaugeWithTimestamp, we are rejecting timestamps older than epoch 0, which implies that an epoch == 0 is valid.

But here we say that 0 represents a metric without a timestamp, which makes it impossible to submit with a timestamp of 0. GaugeWithTimestamp will happily pass it along, but then appendTimestamp will be a no-op as it matches the noTimestamp value.

Suggested change

const noTimestamp = int64(0)

const noTimestamp = int64(minSupportedEpoch - 1)

Switched to: timestamp must be > 0 to be appended to a message or to be considered valid by GaugeWithTimestamp and CountWithTimestamp.

ogaca-dd

LGTM (Nothing to add compare to existing comments).

statsd: support sending metrics with timestamp.

97e39ef

Adds two methods in the `ClientInterface` to support sending gauge and count with timestamp `GaugeWithTimestamp` and `CountWithTimestamp`, for users aggregating metrics on their side and who can provide a timestamp for a given metric.

remeh force-pushed the remeh/timed-sample branch from 597553e to 97e39ef Compare July 28, 2022 09:42

statsd: the metrics with timestamp are directly written in the serial…

3529331

…ization buffer. They are not going through the client aggregator.

remeh force-pushed the remeh/timed-sample branch from a64f8dc to 3529331 Compare July 28, 2022 12:41

hush-hush requested changes Jul 29, 2022

View reviewed changes

statsd: address review feedback.

87572b0

remeh force-pushed the remeh/timed-sample branch from f895d2f to 87572b0 Compare July 29, 2022 12:24

remeh added 3 commits July 29, 2022 14:43

statsd: add minimum Datadog Agent version for feature support.

68308a5

statsd: return an error if an invalid timestamp (< 0) is provided.

6e8a1ef

statsd: mention a minimum datadog agent version

0325b2a

remeh requested a review from hush-hush August 26, 2022 07:52

hush-hush approved these changes Aug 29, 2022

View reviewed changes

rafaelsierra approved these changes Aug 30, 2022

View reviewed changes

scottopell reviewed Sep 6, 2022

View reviewed changes

ogaca-dd reviewed Sep 7, 2022

View reviewed changes

statsd: address feedback concerning noTimestamp use.

4d27807

scottopell approved these changes Sep 26, 2022

View reviewed changes

remeh force-pushed the remeh/timed-sample branch from 795a20f to 04fc6a2 Compare October 7, 2022 12:16

statsd: comments phrasing

da9d9c6

remeh force-pushed the remeh/timed-sample branch from 04fc6a2 to da9d9c6 Compare October 7, 2022 12:18

remeh merged commit 6391269 into master Oct 31, 2022

remeh deleted the remeh/timed-sample branch October 31, 2022 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statsd: support sending metrics with timestamp. #262

statsd: support sending metrics with timestamp. #262

remeh commented Jul 28, 2022 •

edited

hush-hush Jul 29, 2022

remeh Jul 29, 2022

hush-hush Aug 1, 2022

hush-hush Jul 29, 2022

remeh Jul 29, 2022 •

edited

hush-hush Aug 1, 2022

remeh Aug 2, 2022

scottopell Sep 6, 2022

remeh Sep 15, 2022

remeh Sep 19, 2022

scottopell Sep 6, 2022

scottopell Sep 6, 2022

remeh Sep 19, 2022

scottopell Sep 6, 2022

remeh Sep 19, 2022 •

edited

ogaca-dd left a comment

	if timestamp.Unix() < 0 {
	if timestamp.Unix() < minSupportedEpoch {

	const noTimestamp = int64(0)
	const noTimestamp = int64(minSupportedEpoch - 1)

statsd: support sending metrics with timestamp. #262

statsd: support sending metrics with timestamp. #262

Conversation

remeh commented Jul 28, 2022 • edited

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

remeh Jul 29, 2022 • edited

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

remeh Sep 19, 2022 • edited

Choose a reason for hiding this comment

ogaca-dd left a comment

Choose a reason for hiding this comment

remeh commented Jul 28, 2022 •

edited

remeh Jul 29, 2022 •

edited

remeh Sep 19, 2022 •

edited