WIP: Automatic detection of metric interval

[woodsaj]

closes #741

If a metric is received with an Interval field set to 0 or less, then we
need to automatically detect the interval. This is achieved by buffering
the last 3 points in memory. We then sort the points by timestamp
and get the time difference between the first 2, and last 2. The lower
difference is used as the interval. Metrics are delayed from being processed
until the Interval is known, so when a new series is sent it will take 3x interval
before any data is queriable. The interval is re-calculated for the series every 24hours

Future effort needs to be done to adjust the interval to known valid intervals. eg 1s, 5s, 10s, 30s, 1m, 5m, etc....

[Dieterbe]

the values going into the map should be as small & pointerless as possible, for two reasons: memory usage and GC workload.
we don't need the full metricdata of each datapoint, we don't need a lock for each individual record (the logic is so concise that a lock on just the map is enough), pos is trivially implied (check whether the values in the array are 0), and time.Time is a fairly bloated structure (in particular, holds a pointer to a timezone)

thus, instead of a sync.Map holding these items, I suggest something like one of these two:

map[string][4]uint32

type record struct {
interval uint32
points [3]uint32
}

map[string]record

[Dieterbe]

to be more precise, the out-of-orderness must be constrained to the window of 3, for the smallest delta to be correct.
consider this sequence of timestamps:
40, 60, 80, 30, 50, 70

any window of 3 will result in an outcome of 20s interval, which is not correct.
note that it's common to have out-of-orderness beyond 3 points. (default reorderbuffer is 20 which I think is not always enough)

note also that currently the reorderbuffer requires the interval to be known for it to properly bucketize values.

i want to think a bit more about how we can make the reorderbuffer work better together with interval detection.
especially because interval detection can work better if it's done after data is reordered.
(maybe a separate ROB implementation for unknown-interval data?)

[Dieterbe]

this is an interesting problem.
Ideally, we allow for:

• very frequent resolution changes (eg somebody restarts their agent a couple times during a 10 min window with different settings as they figure things out and change their mind)
• allow for many missing points

correct me if i'm wrong, but your heuristic allows for a resolution change once every 24h and will assume out of 3 points, there's 2 of them that are subsequent and exhibit the "real" interval (barring some timing artifacts causing some error as you point out)

it seems the level of correctness is a tunable simply based on how many points we can track and inspect.
perhaps we can integrate this better with the reorderbuffer, or with the data in the tsz chunks, to be able to consider more data and achieve higher correctness.
also, maybe there's a way to not have to look at invididual points, but rather use a streaming approach.
anyway, just some unfinished thoughts, you said you're working on a new approach, so i'll leave it at that for now :)

[woodsaj]

it seems the level of correctness is a tunable simply based on how many points we can track and inspect.

to a degree. Metrics dont become queryable until we determine the interval as the interval is used when generating the MetricId. So we can add the metricDef to the index until we know the correct interval.

However once we have received the first 3 points, we can then use the ROB to periodically determine the inteval. And with a larger ROB the accuracy of the interval detection improves.

[woodsaj]

@Dieterbe this is ready for first review.

it still has local modifications in vendor/gopkg.in/raintank/schema.v1, But that will be addressed soon

You can use the docker/docker-cloud stack to test. You will need to build a docker image for tsdb-gw using the optimizeKafkaMsg branch of tsdb-gw first.

[Dieterbe]

what's with the Interval: -1 ? why not 0? I would always try to use unsigned numbers when we can, that leaves room for future encoding optimizations, like in #260 , it also fits with the go mantra "make the zero value useful" (to mean: unset / default)

[woodsaj]

what's with the Interval: -1 ? why not 0

MetricData.Validate() rejects metrics that have an Interval == 0. but not if it is less then 0.

Though we can obviously change that.

[Dieterbe]

I added some commits, cleaning up the old benchmarks to be a better reference, adding the schema as discussed in comment #199 (comment) and onwards (minus the byte payload header which is a message-specific implementation detail, and the effect on it is the same irrespective of what new serialization format we'll use anyway), and added an experimental simplistic manual serializing/deserializing step.

the format without orgid is codenamed MetricPointId1, the one with orgid is MetricPointId2
In this experiment i focused on the former, but the results should be similar for the latter.
I took the experiment a step further and also added a Marshaling function that doesn't need to extend the input slice, since the caller (metrictank kafka plugin) can just always provide it a buffer of the right size

here's how it compares:

BenchmarkSerializeMetricPointId1Msgp-8                 	20000000	        77.1 ns/op	     110 B/op	       0 allocs/op
	metric_serialization_msgp_test.go:90: with   20000000 metrics -> final size: 47.0 bytes per metric
BenchmarkDeSerializeMetricPointId1Msgp-8               	20000000	        62.8 ns/op	       0 B/op	       0 allocs/op

BenchmarkSerializeMetricPointId1Manual-8               	50000000	        23.2 ns/op	      75 B/op	       0 allocs/op
	metric_serialization_msgp_test.go:158: with   50000000 metrics -> final size: 28.0 bytes per metric
BenchmarkDeSerializeMetricPointId1Manual-8             	200000000	         7.76 ns/op	       0 B/op	       0 allocs/op

BenchmarkSerializeMetricPointId1ManualStaticBuffer-8   	100000000	        15.9 ns/op	       0 B/op	       0 allocs/op
	metric_serialization_msgp_test.go:146: with      10000 metrics -> final size: 28.0 bytes per metric

conclusion:

=> data payload is much smaller, because messagepack adds metadata and attribute names (it writes strings like "Id", "Time", etc preceding the actual fields). mine doesn't do that and is exactly the 28 bytes we were aiming for.
=> serializing and deserializing is faster. especially deserializing. haven't looked in depth why, but probably because messagepagk is more generic (supports arbitrary listings of fields, in any order, so lots of clauses and switch statements)
=> when we know we can pass it a properly sized buffer, the encoding speed is even faster.

it's almost too good to be true. maybe there's a bug somewhere but I added unit tests that validate the encoding works, and they succeed. so i'm pretty happy with this experiment.

let's avoid the overhead of interfaces and function calls, we don't need them.
we just need the format byte and decode accordingly, and leave it up to the client (e.g. tsdbgw) to assure that full MetricData payloads are sent instead of optimized payload (currently codenamed MetricPointId1) unless we've sent a full MetricData within the last hour. metrictank can then simply drop MetricPointId1 messages if it doesn't recognize the id. we can simply extend kafka retention by an hour to make this safe.

do you plan to clean up this PR? otherwise I can create a new one with my stuff.

[woodsaj]

all of these changes to raintank/schema need to be moved to raintank/schema#15

[woodsaj]

let's avoid the overhead of interfaces and function calls, we don't need them.

Without using interfaces, how do you plan on passing datapoints throughout metrictank ingestion path? Are you just going to create two variants of every function that the datapoints pass through. One that takes a the full metricData and one that takes the optimized data structure?

[Dieterbe]

yes, that's more or less what I was thinking of. will have to prototype it to see if it can be done in a way that's not too ugly. my main concern is really just allocations in the ingestion path, we've seen those get expensive fast and have effects on various other characteristics (like GC induced latency spikes).
my goal is for ingestion to be zero-allocation, except when we receive new metrics we haven't seen before, or when we need to create new chunks. perhaps this can be achieved with the interface approach as well, i'm not sure. i'm currently reviewing the v2 schema pr.

[Dieterbe]

closing this as we decided not to pursue this for now.