Chunk 4h sparse bugfix longterm + remediation for single-point old format chunks #1129
Conversation
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
2 times, most recently
from
bf93bb6 to
422550e
Compare
also: assure chunkAtSave accounts for the full encoded chunk, not just the tsz part of it. In bigtable we were actually reporting both variants separately for each chunk being saved.
so that we can add another type that encodes without the bug
note: callers (e.g. stores) now no longer control which format is being used. it's the chunk themselves that know what format header to use.
t0 is more specific and accurate than "ts". also, put it before the byte buffer. because when want to iterate over the byte buffer, we'll most likely want to start with t0 anyway. also conceptually it seems to make sense as datapoints have timestamps >= t0
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
3 times, most recently
from
14454fc to
aaff973
Compare
|
@robert-milan IterGen.Size() now returns slightly higher values than before (because it keeps the full chunk, not just the encoded tsz.Series data like before), and the type itself has a few different properties. IterGen.Size() is only called from the cache accounting. does anything need to change there in the cache accounting? |
Dieterbe
changed the title
|
did some race condition testing today, using the method described in #1150 (comment) + running benchmarks/realistic-workload-single-tenant.sh (but with the mpo's and the vegeta rate divided by 20 otherwise my laptop couldn't hold it together - race builds are more resource intensive) , using docker-cluster which has 2min chunks, as to definitely have overlap between chunks getting saved and loaded, cache usage rising over time, etc. |
|
reading data using mixed chunk formats works well. (i tested both ways. when running master with the new format already in the database, requests fail obviously with unknown chunk format errors. but the new version displays data from both formats beautifully. data looks fine also) also visually inspected the MT dashboard, all stats look fine |
|
Didn't mention it here yet, but last week I copied the broken chunks of the affected customer to a test environment and confirmed that i could accurately reproduce the incorrect viz of their rollup data using master (which was not being properly remediated due to having so many single-point chunks) , and that it was fixed with the code in this branch. |
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
from
ba7988f to
3a99152
Compare
benchmarksmain concerns / areas of interest
running the testThere's a few things to keep into account:
so, we apply this patch: to reproduce:
new benchmarksmaster-without-remediationyou can use https://github.com/grafana/metrictank/pull/new/1129-helper-master-without-remediation https://snapshot.raintank.io/dashboard/snapshot/nUvLIrHZ256dpr7Osx6LqzjDPGvMRChC HEADhttps://snapshot.raintank.io/dashboard/snapshot/9O5c6WFMGZcLCF59dlerTsZknVZIrftq?orgId=2 HEAD but saving/reading old style chunksthe idea here is to use a patch as to save old style chunks, and read them through the new code. https://snapshot.raintank.io/dashboard/snapshot/nJF2gZ7g189DyL57dayWlE3eSRJwVJEg conclusion:
|
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
from
3a99152 to
71a5193
Compare
==> 9 Bytes gain per chunk 😎 |
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
from
915ea4c to
10c9db7
Compare
|
I just went over all api endpoints and confirmed none of them returns a |
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
from
c203181 to
45026f4
Compare
* Do away with the intermediate "Iter" type.
Now Iter is an interface, which the tsz iterators implement
This allows us to use arbitrary iterators backed by different
chunks/series encodings, at the expense of a small function
call overhead.
* put IterGen on a diet.
- no more span attribute.
it was wasteful for chunks for which we don't know the span,
and bulky for others because we can better use
its compact encoded representation, which we do now.
- generally just rely on the chunk data as is, and interpret
the data as necessary to find the span, to create iterators, etc.
this also means we no longer need the Encode method.
note: IterGen msgp serialization obviously also changes, but seems
only used by archive.Archive msgp serialization,
which is used by these metric methods:
func (m *Metric) MarshalCompressed() (*bytes.Buffer, error) {
func (m *Metric) UnmarshalCompressed(b io.Reader) error {
which are only used by these tools:
mt-whisper-importer-reader
mt-whisper-importer-writer
It doesn't seem to affect running clusters etc.
in SeriesLong, expose the .T (last timestamp) and .Finished fields and let Chunk leverage them
* no need to return pointer * clearer diff between the two constructors
by using an interval hint, we know what kind of delta we need to end up with a valid timestamp. For this to be effective this requires an interval: * >1 * that is not a divisor of 16384. but for chunks that are so long to be affected (e.g. >4h), that is always the case. Note: this can be cleaned up when we have raintank/schema#22
this helps with tracking down where ChunkWriteRequest are being created
the go behavior of unnamed fields and automatically making method and field lookups work on them is nice, but ambiguous. It seems better to be very explicit. For one thing, it allows us to easily find all the places where certain methods and fields of Series are being called, which helps with making sure we are race free. Perhaps later this can be refactored and tsz can be merged into the chunk package but that's a worry for later.
even though we don't use it anymore
no longer needed + now we have some actual validation
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
from
45026f4 to
cab1d87
Compare
I didn't notice any changes in the sizes of the structs we are using to estimate total sizes, and since Size() itself is used for that particular calculation we should not need to change anything. edited to clarify no change in size of structs vs the structs themselves |
|
ah yes, IterGen went from to lost the span, but need the intervalHint now (even for itergens that hold seriesLong data unless we want to wrap it in yet another interface, which i don't want). bummer :( |
technically, currently not needed. but might become needed once we start sending (old) chunks via our cluster protocol rather than points. rather safe than sorry.
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
2 times, most recently
from
ab6f671 to
db4d23a
Compare
and validates that what comes out, matches what went in
Dieterbe
force-pushed
the
chunk-4h-sparse-bugfix-longterm
branch
from
5c64f69 to
23b731c
Compare
|
for the record, just used benchstat to compare all go benchmarks before and after, and they're all identical |
See #1126 for problem description and short term workaround.
This is the longer term fix: introducing a new chunk format. + implemented the remediation for chunks with a single point, as alluded to in #1126
we now have:
before storing any points, we assume an initial delta of 60,
if the delta happens to be 60, dod (delta-of-delta) is 0 so we store 1 bit.
if the dod is between -63 and 64 (e.g. delta is 1 to 120s) we only need to store '10' and a 7 bit value. so 9 bits.
for other cases and more info, see the paper.
So the new format not only fixes the bug, it also saves us 32bit for the t0 and in most cases (14-9) to (14-1) (5 to 13 bits), by switching from a delta to dod for first point, shortening chunks by about 4 to 5 bytes.
it also uses a more compact end-of-stream marker, as explained in the devdocs, saving another up to 5bytes in the best case.
we then use SeriesLong everywhere in all new chunks, while old chunks can still be present in the database and chunk cache, so we accommodate for them by having each implement an iterator interface. This does mean that getting points out of chunks comes with the interface overhead, but I think it shouldn't affect us much. on query heavy instances, a bit of increased cpu usage and maybe slightly slower responses. see the new benchmarks in tsz_test.go , though they don't match the results in https://github.com/Dieterbe/dataprocessexp , this can use a bit more research.
While doing this i also cleaned up some code, specifically in chunk.Chunk and chunk.IterGen, which are now cleaner and more focused.
Btw I want to rename IterGen to Chunk and Chunk could then be called "Block" or something, but perhaps I should do that in a followup PR.