Add call to estimate number of samples in a chunk to the API #2168

tomwilkie · 2016-11-06T15:36:37Z

Want this for metrics in Cortex (nee Frankenstein)

For (double) delta this is exact, but for varbit its an under-estimate IIUC.

tomwilkie · 2016-11-16T12:13:02Z

@beorn7 mind taking a look at this please?

beorn7 · 2016-11-16T12:16:05Z

Sorry, I thought this was meant for @juliusv . It's on my review queue now.

tomwilkie · 2016-11-16T12:17:30Z

Thanks!

beorn7 · 2016-11-16T17:41:55Z

storage/local/chunk/varbit.go

+		return 1
+	}
+
+	return float64(int(offset)-varbitFirstValueDeltaOffset) / float64(varbitWorstCaseBitsPerSample[c.valueEncoding()])


That more a lower limit of chunks than an estimate. I expect this to be wildly off most of the time as the timestamps compress most reliably, and they are always counted with 27 bits here.

beorn7 · 2016-11-16T17:45:16Z

I'm wondering about the "must run in constant time" thing.

A chunk being written to could just keep counting the number of samples. That would be no noticeable overhead compared to the usual encoding efforts. A chunk that has just been loaded needed to iterate through all samples on the first call of Len but could then remember and update the number for as long as it is loaded.

Would that fulfill the requirements of your use case?

I don't think the Len reported by varbit chunks is very useful with the current implementation, see comment there.

beorn7 · 2016-11-16T18:01:40Z

Problem would be all the still open chunks coming from loading the checkpoint. They would all need to iterate through to find out about their length.

tomwilkie · 2016-11-16T18:44:16Z

It would. We could lazily initialise the length field to get round the
problem? WDYT?

For cortex (nee Frankenstein) we monitor samples/chunk as it's the key
metric in optioning and managing costs. So the estimate as is would not be
good enough, no.
On Wed, 16 Nov 2016 at 18:01, Björn Rabenstein notifications@github.com
wrote:

Problem would be all the still open chunks coming from loading the
checkpoint. They would all need to iterate through to find out about their
length.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2168 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAbGhWYsaZ46qJ0VZ8Xv1V_H9etXJJ8Wks5q-0UGgaJpZM4Kqleg
.

juliusv · 2016-11-17T06:36:30Z

It's annoying that we're having to think about code interactions that will not happen in practice, but should still be taken care of to keep the code "correct": Prometheus itself is not using Len() (thus doesn't care about those methods reporting correct values in any case). Cortex on the other hand is not loading existing chunks from disk (thus wouldn't care about repopulating chunk lengths when loading from disk). Further, Cortex is not even using varbit chunks... so now we're going to be adding significant code that neither system will use.

What are other options?

panic("not implemented") in the varbit implementation?
track the chunk lengths ourselves somehow in Cortex?

beorn7 · 2016-11-17T09:09:56Z

In general, I like the feature (as long as it doesn't inflict any noticeable cost during normal operation). I was playing with some analysis options in storagetool to find out more about a cold storage…
But such a feature didn't need constant runtime, so it could just iterate through a chunk and count.

If Cortex really doesn't use varbit chunks, I'd tend towards implementing Len without guaranteed constant runtime, and then document the runtime for each implementation. (I'd read a panic("not implemented") as a TODO. Better have a working but slow implementation.)

tomwilkie · 2016-11-17T11:34:19Z

Cortex is not even using varbit chunks...

Well, thats about to change :-) A motivation for this PR is doing tuning on cortex to manage cost, and we're hoping switching to varbit will help in this regard. So its kinda related.

The constant runtime thing is actually not important - we only sample this value when flushing chunks. So perhaps lets drop that?

beorn7 · 2016-11-17T12:32:33Z

Yeah, let's have a naive implementation for now. If it doesn't serve your purpose, we can reconsider.

tomwilkie · 2016-11-17T13:20:16Z

@beorn7 simplest thing, with a test. Let me know what you think.

beorn7 · 2016-11-17T17:54:50Z

storage/local/chunk/chunk_test.go

+	for _, c := range chunks {
+		for i := 0; i <= 10; i++ {
+			if c.Len() != i {
+				t.Errorf("empty chunk type %s should have %d samples, had %d", c.Encoding(), i, c.Len())


Empty? I guess that words needs to go.

beorn7 · 2016-11-17T17:56:05Z

storage/local/chunk/chunk_test.go

+				t.Errorf("empty chunk type %s should have %d samples, had %d", c.Encoding(), i, c.Len())
+			}
+
+			cs, _ := c.Add(model.SamplePair{model.Time(i), model.SampleValue(i)})


For clarity in case it ever happens, I'd error out if an err!=nil is returned, and likewise if more than one chunk is returned.

beorn7 · 2016-11-17T17:58:44Z

storage/local/chunk/chunk.go

@@ -276,6 +276,7 @@ type Chunk interface {
 	UnmarshalFromBuf([]byte) error
 	Encoding() Encoding
 	Utilization() float64
+	Len() int


I guess it deserves documentation that it returns the number of chunks (and not the length in byte or something), and that the implementation might be expensize.

(And while you are on it, the "add" above in the doc comment needs to be capitalized.)

beorn7 · 2016-11-17T18:00:20Z

storage/local/chunk/delta.go

@@ -296,7 +296,7 @@ func (c deltaEncodedChunk) sampleSize() int {
 	return int(c.timeBytes() + c.valueBytes())
 }

-func (c deltaEncodedChunk) len() int {
+func (c deltaEncodedChunk) Len() int {


Doc comment that it implements chunk and that it is constant runtime.

beorn7 · 2016-11-17T18:00:58Z

storage/local/chunk/doubledelta.go

@@ -336,7 +336,7 @@ func (c doubleDeltaEncodedChunk) sampleSize() int {
 	return int(c.timeBytes() + c.valueBytes())
 }

-func (c doubleDeltaEncodedChunk) len() int {
+func (c doubleDeltaEncodedChunk) Len() int {


Doc comment that it implements chunk and that it is constant runtime.

beorn7 · 2016-11-17T18:01:24Z

storage/local/chunk/varbit.go

@@ -328,6 +328,15 @@ func (c varbitChunk) Utilization() float64 {
 	return math.Min(float64(c.nextSampleOffset()/8+15)/float64(cap(c)), 1)
 }

+// Len implements chunk.


Perhaps note that this has O(n) runtime.

beorn7 · 2016-11-17T18:02:03Z

storage/local/chunk/varbit.go

+// Len implements chunk.
+func (c varbitChunk) Len() int {
+	it := c.NewIterator()
+	var i = 0


We usually write this as
i := 0

beorn7 · 2016-11-17T18:03:13Z

Looks good. Just nits left.

tomwilkie · 2016-11-17T19:10:35Z

Done & squashed. Thanks for feedback @beorn7!

juliusv · 2016-11-18T07:13:49Z

Alright alright, then we do it this way!

beorn7 reviewed Nov 16, 2016

View reviewed changes

beorn7 reviewed Nov 17, 2016

View reviewed changes

Add call to estimate number of samples in a chunk to the API

585878c

tomwilkie force-pushed the chunk-len branch from 5d881db to 585878c Compare November 17, 2016 19:10

juliusv merged commit 127332c into prometheus:master Nov 18, 2016

juliusv deleted the chunk-len branch November 18, 2016 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add call to estimate number of samples in a chunk to the API #2168

Add call to estimate number of samples in a chunk to the API #2168

tomwilkie commented Nov 6, 2016 •

edited

Loading

tomwilkie commented Nov 16, 2016

beorn7 commented Nov 16, 2016

tomwilkie commented Nov 16, 2016

beorn7 Nov 16, 2016

beorn7 commented Nov 16, 2016

beorn7 commented Nov 16, 2016

tomwilkie commented Nov 16, 2016

juliusv commented Nov 17, 2016

beorn7 commented Nov 17, 2016

tomwilkie commented Nov 17, 2016

beorn7 commented Nov 17, 2016

tomwilkie commented Nov 17, 2016

beorn7 Nov 17, 2016

beorn7 Nov 17, 2016

beorn7 Nov 17, 2016

beorn7 Nov 17, 2016

beorn7 Nov 17, 2016

beorn7 Nov 17, 2016

beorn7 Nov 17, 2016

beorn7 commented Nov 17, 2016

tomwilkie commented Nov 17, 2016

juliusv commented Nov 18, 2016

Add call to estimate number of samples in a chunk to the API #2168

Add call to estimate number of samples in a chunk to the API #2168

Conversation

tomwilkie commented Nov 6, 2016 • edited Loading

tomwilkie commented Nov 16, 2016

beorn7 commented Nov 16, 2016

tomwilkie commented Nov 16, 2016

Choose a reason for hiding this comment

beorn7 commented Nov 16, 2016

beorn7 commented Nov 16, 2016

tomwilkie commented Nov 16, 2016

juliusv commented Nov 17, 2016

beorn7 commented Nov 17, 2016

tomwilkie commented Nov 17, 2016

beorn7 commented Nov 17, 2016

tomwilkie commented Nov 17, 2016

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

beorn7 commented Nov 17, 2016

tomwilkie commented Nov 17, 2016

juliusv commented Nov 18, 2016

tomwilkie commented Nov 6, 2016 •

edited

Loading