Replies: 3 comments
-
cc @codesome |
Beta Was this translation helpful? Give feedback.
-
Thanks for bringing it up @Dieterbe. I think we need to make Commit() to behave similar to Append(). i.e. ignore last sample if timestamp and value is same, and error out if value is different with same timestamp. But we do not want to error out on OOO sample in Commit() (yet).
It is indeed. I don't exactly have the historical context, but I happen to know that getting samples with same timestamp and different values is problematic. Same timestamp and same value is possible if some exporter is having timestamp in the exposition, hence we don't error on that. |
Beta Was this translation helpful? Give feedback.
-
Came up during #12352 , due to the fact that for histograms there is a remote possibility of getting a real error in this function. |
Beta Was this translation helpful? Give feedback.
-
I noticed a few strange things that I can think can be clarified or perhaps improved in the codebase.
Happy to file a PR, but looking for some clarity first.
Here's how things behave now:
in headAppender.Append() (uses appendable())
A) older than last TS: return OOO error, and increment OOO metric
B) same TS as last point, different value: return DuplicateSampleForTimestamp error, no metric
C) same TS as last point, same value: no error or metric. treat as valid append
in headAppender.Commit() (uses append()):
A/B/C) in all 3 of the cases, we increment the OOO metric, and none of them returns an error.
Here's my thoughts:
*) Currently, Commit() errors are used to convey io errors. Would it not make sense to have it return sample-level errors in case of duplicate and OOOjust like Append()? (either by overloading the error return value, or adding a secondary error return value)
*) for 1 C -> we know that in Commit(), the append() call will drop this sample anyway. So why even bother buffering it in the appender and trying to commit it ?
*) why are samples with same TS but different value reported (metric incremented) as OOO if it happens during Commit, but not if it happens during Append?
ErrDuplicateSampleForTimestamp
? this error is currently only used when a sample has an identical TS but different value. The use of the name 'duplicate' is a bit confusing.Another thought:
in memSeries.append(), when head is nil, but there are mmapped chunks, is there an easy/cheap way to always get the last value? (the last ts can be obtained from the mmappedChunk.maxTime). If not, this strengthens the argument that any timestamp match should be considered a "duplicate", whether or not the value matches (instead of just the case where they don't match and only in Append())
Beta Was this translation helpful? Give feedback.
All reactions