nhcb: store custom buckets in WAL, WBL

[krajorama]

Proposal

Related to prometheus/proposals#31

WAL, WBL records need to be able to encode the native histogram custom values.

Alternative 1:

Encode custom values into every histogram record.

Alternative 2:

This is the preferred method for implementation. #14730 (comment)

Introduce new record type that has a series reference and the custom values. Only record once every (segment?).

[bwplotka]

Can't we use native histogram chunk format as a first step? I guess that's the alternative 1, right?

[krajorama]

Can't we use native histogram chunk format as a first step? I guess that's the alternative 1, right?

Yes. I don't know if it's a good idea, haven't thought it through. I'm worried that storing the custom values for every histogram individually is a huge waste of space. Similar to how we don't store the labels in every sample, but once per series:

prometheus/tsdb/record/record.go

Line 147 in 8c7bf39

Labels labels.Labels

On the other hand adding a new RefCustomValues record type like

type RefCustomValues struct {
	Ref chunks.HeadSeriesRef
	V  []float64
}

is probably more complicated?

[krajorama]

@bwplotka We've discussed this question in the histograms sync meeting. It was pointed out that we already keep the bucket definitions separate from the values, since the bucket definition is a label le in the classic case and labels have their own record, separate from the value. I'm convinced now that we should have a separate record for the custom values. Updating the issue.

[bwplotka]

That's separate discussion than chunk format, or is it similar decision?

[krajorama]

That's separate discussion than chunk format, or is it similar decision?

Separate. Reusing the same formatting as in the chunks should be fine. We can always add new kind of record if we need to.

[bwplotka]

@bwplotka We've discussed this question in the histograms sync meeting. It was pointed out that we already keep the bucket definitions separate from the values, since the bucket definition is a label le in the classic case and labels have their own record, separate from the value. I'm convinced now that we should have a separate record for the custom values. Updating the issue.

Hm... I would challenge this. It's true we move bucketing from label + sample to complex sample. But:

1. We do the same for exponential native histograms, yet we didn't decide to have a separate histogram sample record and separate histogram spans/buckets/schema record.
2. So far looking on the @carrieedwards first attempt adding another record adds a lot of complexity to code and format vs existing histogram types referenced by refs literally have custom buckets.
3. AFAIK, there is currently no logic for NOT adding recording if it does not change or something, so I don't see how separation is cheaper here no both storage and encoding/decoding. I would argue it's much more expensive actually to process separate buckets.

To sum up, I would not try to prematurely optimize with a new pattern, especially as it's not easy to add/maintain - but instead do same as native histograms are doing for now, especially as NH literally have CustomValues slice in it. Then if profile and benchmarks suggest, we can try to be more smart and split/only add new record when it's needed or something.

Started some attempt on top of @carrieedwards code here: main...bwplotka/nhcb-wal-wbl

The main "counter argument" is backward compatibility. I don't see any versioning for WAL records (?), so this will require a new Histogram and float histogram record (with versioning added ideally for future use). This however can and should be FULLY abstracted by inside Re(Float)HistogramSample so little complexity for appenders and head.

[bwplotka]

Related #15200

[bwplotka]

To progress here:

A) We could start by adding a single different record type for RefHistogram to replace float and normal histogram, but also supports custom buckets -- all with updated histogram scheme that:

• supports NHCB
• ONLY encodes certain things based on schema and if it's float or native (another bit for that)?
• I would even try by using capnproto to see how it looks like (only for new records) to enable unknown fields in future.

Funny enough, even without tackling versioning a lot here, we could encode ONLY NHCB in this new record type, and then switch to it if successful for other histograms in a few later releases.

B) We could continue separate bucketing record. It's inconsistent and might cause pain in future, but it does not break compatibility, I think. I wonder only as compared to @carrieedwards approach here is there a room to encapsulate reading/writing a single package or abstraction, adding new records even is spreaded around the codebase with pools everywhere. Technically head/wal code does not need to be aware about RefHistogramCustomValues record, it could be all passed by and updated onto corresponding RefHistogramSample literally per segment. Wonder if this new pattern of splitting information to multiple records only to avoid compatibility problems is worth it, where we might want to tackle compatibility/versioning right away and keep schema clean.

I think I sill lean on A), but personally will have more time helping on this after KubeCon NA