Write only as many chunk bytes as needed

[bboreham]

Fixes #631

Note we had not previously edited the chunk code copy-pasted from Prometheus (v1).

Only fully implemented for varbit chunks at present; I don't think we're intending to use other kinds.

Varbit chunks hold metadata in a header and a footer at the end of the chunk, however the footer is only used while appending more samples, so can be discarded when the chunk is "closed" and written to store.

A flag is added to make the new behaviour optional on writing, since shorter chunks cannot be read by older versions of Cortex so if you roll back after turning it on, you lose the ability to query any data written with short chunks.

The shorter encoding is also used when transferring chunks from one ingester to another on rolling updates.

[tomwilkie]

Would you mind adding some unit tests?

[bboreham]

I should have given some thought to making this optional. Once you write some smaller chunks, you need the code change to read them back, so you'd need to be sure you weren't rolling back past this change.

[bboreham]

Would you mind adding some unit tests?

The existing unit tests exercise this code. What extra were you thinking of?

[tomwilkie]

The existing unit tests exercise this code.

Theres nothing in this package which exercises this code. The tests in pkg/chunk/ cover it, but aren't exactly looking for bugs in the chunk encoding - for instance, TestChunkCodec only puts a single sample in the chunk.

I think it would be good to have a test which appends an increasing number of samples to a chunk, encodes it via a Marshal, decodes it and then checks all the samples are there.

[tomwilkie]

Also needs a DCO.

[tomwilkie]

Note we had not previously edited the chunk code copy-pasted from Prometheus (v1).

#1029 edited the code here too.

[tomwilkie]

I think it would be good to have a test which append..

I added such a test in #1048

[tomwilkie]

@bboreham now that #1048 is merged, want me to update this?

[bboreham]

Sure, if you like. I left it alone because I saw only a 5% overall reduction in storage size, but I still think it's worth doing if you have time.

[tomwilkie]

There you go @bboreham; I ended up remove the Unmarshal func on chunks, as it was only used in tests.

[tomwilkie]

(Its an LGTM from me, but I'll let you merge it @bboreham)

[bboreham]

Current status is I believe I have tried this and I believe it isn't working, in that it still outputs a bunch of zeros at the end of the chunk.
Further research required.

[bboreham]

I take it back: the code where I thought this had been used was turned off.

[bboreham]

I did a more effective test. Everything worked when I had ingesters rolled forward and queriers to match, but when I tried to roll back the ingesters the chunk hand-over failed:

level=warn ts=2018-11-29T14:31:32.59474578Z caller=gokit.go:46 method=/cortex.Ingester/TransferChunks duration=25.357151ms err="insufficient bytes copied from buffer during unmarshaling, want 1024, got 155" msg="gRPC\n"

this is something I hadn't thought about: it's actually a nice efficiency gain for the old-style chunks to do the hand-over without padding, but you can't fail back to older ingesters.

So, what to do? Make it optional on the write path so we can update everything and then turn it on?

[tomwilkie]

Could this be done more like PreallocConfig in the client pkg? https://github.com/cortexproject/cortex/blob/master/pkg/ingester/client/timeseries.go#L13

[tomwilkie]

One nit, otherwise LGTM

[tomwilkie]

Nice, thanks! LGTM

[bboreham]

This is a bit broken, because you need the flag on the reading side but I only plumbed it in to ingester.

What I wanted was that all components could read chunks of either style, and the flag changes what ingester writes.

[bboreham]

Still broken - the chunks passed from one ingester to another in hand-over cannot be appended to.
Don't turn the feature on until this is fixed!