Conversation
ArthurSens
force-pushed
the
ct-histogram
branch
2 times, most recently
from
e0ee07d to
fa1832b
Compare
Test failure messageHere is the failure message I'm struggling to understand 🤔 |
|
Alright, only 1 test is failing now: What this test does is ingest Histogram samples with created timestamps in several different scenarios. Some of those scenarios should reject the sample and others should ingested. After that, we query the ingested samples and verify that a histogram zero sample and the normal histogram sample are retrieved as expected. The test is failing right now because the histogram zero samples are being modified after ingestion with a non-zero |
ArthurSens
force-pushed
the
ct-histogram
branch
from
28c8847 to
ffdd8d1
Compare
ArthurSens
requested review from
cstyan,
jesusvazquez and
tomwilkie
as code owners
ArthurSens
force-pushed
the
ct-histogram
branch
from
ffdd8d1 to
a75bcc1
Compare
relevant: @bwplotka's comment #13384 (comment) |
Yes, the appended histogram as well as the resulting histogram in TSDB can be modified by AppendHistogram. There's two strategies in tests around this:
If |
krajorama
left a comment
krajorama
left a comment
There was a problem hiding this comment.
first pass feedback on tests
Ah ok that makes sense then. Yes, we should always see a CounterResetHint==2 when ingesting a zero sample histogram. But that leaves me with another question: If we're scraping a histogram for the first time and this histogram has set CT during exposition. Should the zero sample also contain ResetHint==2? Theoretically there was no reset since it's the first sample ever ingested for the series |
Yep, you got it. If there's no previous sample, we encode 0 (unknown reset) because later when the chunk is read back there might be some previous sample loaded from disk for example and we cannot say counter reset or no reset with any confidence, so we encode "unknown". |
ArthurSens
force-pushed
the
ct-histogram
branch
from
a75bcc1 to
ad29b1c
Compare
|
Converted to draft while I work on the failing test in head_appender |
ArthurSens
force-pushed
the
ct-histogram
branch
from
ad29b1c to
2596498
Compare
|
This PR will conflict with #14546 once merged. I don't mind resolving the conflicts here, so it's up to you which PR gets merged first @krajorama! |
|
Converting to draft as I work through the conflicts |
ArthurSens
force-pushed
the
ct-histogram
branch
from
2596498 to
ef933cf
Compare
Many thanks for taking the hit, the OOO NH PR was months in the making. |
ArthurSens
force-pushed
the
ct-histogram
branch
from
ef933cf to
f295e9d
Compare
No problems :) |
| s := a.head.series.getByID(chunks.HeadSeriesRef(ref)) | ||
| if s == nil { | ||
| // Ensure no empty labels have gotten through. | ||
| lset = lset.WithoutEmpty() | ||
| if lset.IsEmpty() { | ||
| return 0, fmt.Errorf("empty labelset: %w", ErrInvalidSample) | ||
| } | ||
|
|
||
| if l, dup := lset.HasDuplicateLabelNames(); dup { | ||
| return 0, fmt.Errorf(`label name "%s" is not unique: %w`, l, ErrInvalidSample) | ||
| } | ||
|
|
||
| var created bool | ||
| var err error | ||
| s, created, err = a.head.getOrCreate(lset.Hash(), lset) | ||
| if err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
This part can be abstracted into a separate function, just like we do when appending floats, and we reduce duplicated code with AppendHistogram. I'd like to do it in a separate PR though
jesusvazquez
left a comment
jesusvazquez
left a comment
There was a problem hiding this comment.
Looks good! Left one question to fully understand.
| // Start fake HTTP target to that allow one scrape only. | ||
| server := httptest.NewServer( | ||
| http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| fail := true // TODO(bwplotka): Kill or use? |
There was a problem hiding this comment.
Probably not, but @Maniktherana is reworking this test case in #14738. I'm hoping to rebase this PR if that one is merged first, or just open a new pull request updating this one if his PR takes too long. Would that be ok?
| case h != nil: | ||
| zeroHistogram := &histogram.Histogram{} | ||
| s.Lock() | ||
| _, _, err := s.appendableHistogram(ct, zeroHistogram, a.headMaxt, a.minValidTime, a.oooTimeWindow, false) // OOO is not allowed for CTZeroSamples. |
There was a problem hiding this comment.
Why OOO is not allowed for CT Zero Samples?
There was a problem hiding this comment.
That's because after the first sample, every CT is OOO. I'm not sure if WBL accepts samples with duplicated timestamps, but it doesn't sound efficient to append the same 0 sample with the same timestamp on every scrape 🤔
There was a problem hiding this comment.
That's something that we can probably improve and eventually allow OOO CT.
I guess I could improve the comment here with a better explanation of why OOO CT is not allowed TODAY, if my comment doesn't make sense in the future then people might feel comfortable in changing the codebase 🤔
ArthurSens
force-pushed
the
ct-histogram
branch
2 times, most recently
from
4825787 to
3342f5c
Compare
|
It should be ready for another review :) |
| if ct > a.maxt { | ||
| a.maxt = ct | ||
| } | ||
| return 0, nil |
There was a problem hiding this comment.
Inconsistency with float handling:
| return 0, nil | |
| return storage.SeriesRef(s.ref), nil |
There was a problem hiding this comment.
Good catch! It's not only inconsistent, it would be incorrect to always return 0! The reference is used in following Append calls
| require.NoError(t, err) | ||
| } | ||
| } | ||
| require.NoError(t, a.Commit()) |
There was a problem hiding this comment.
nit: it's kind of weird that we don't test with multiple Commits() , thus the path to return out of order is never exercised. As far as I can tell that was the case for floats anyway, so maybe another PR?
There was a problem hiding this comment.
Interesting. Yeah, we haven't tested OOO well enough, that was a good call out. If that's ok I'd be happy to extend the tests here to cover OOO in another PR
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
ArthurSens
force-pushed
the
ct-histogram
branch
from
dfb511a to
95a53ef
Compare
|
Just did a quick experiment with an app exposing native histograms, I can see the first one being ingested as a 0 🥳 
|
|
Taking the liberty to merge! |
Signed-off-by: Jan Fajerski <[email protected]>
5 participants
Fixes #13384
Fundamentally it follows the same logic used in
AppendCTZeroSample(), used to append zeros for usual samples, but for histograms, we're using Go's zero value forhistogram.Histogram{}andhistogram.FloatHistogram{}The tricky thing here is that the Histogram format has CounterResetHints, which the query engine uses to detect resets. From my understanding, however, this is a no-op during the scrape and the storage layer handles the addition of counter-reset hints. This PR is only tackling the scrape part of things.
There is one test in head_appender_test that queries the appended data and counter-reset hints are verified as well