Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Add min-time to remote-out-of-order bool flag. #128

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amanycodes
Copy link

Fixes [TODO]

The remote-out-of-order flag now supports specifying a minimum time (min-time), so that out-of-order samples are only generated for timestamps no older than the defined time.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping!

This is a nice improvement, unfortunately looking closer the current implementation (yours and before) is not really introducing out-of-order as Prometheus data model understands this. My bad for not catching this during the initial PR #68

The current flow adds -60s, -5s or 0 to each sample timestamp. However in the current implementation we only send ONE sample per time series. While different samples might have different timestamps (up to 60s or remote-ooo.min-time duration in the past vs other) with our ooo option, it does NOT really exercise any OOO semantics as a single series samples are still very much in order.

With e.g. -3h offset we might exercise "too old" sample logic in the receiving backend (Prometheus simply rejects this), but it's not very interesting case to benchmark.

I think perhaps what @Vanshikav123 intended was to try shuffle order of samples within a single series, which would be interesting to exercise (Prometheus with OOO feature enabled packs out of order sample to different data structures). Unfortunately, for this, more complex implementation is needed e.g. we could gather metrics within some time frame (e.g. wait for at least two sample and shuffle that, even without min-time or offsets).

To sum, I think this feature is currently broken, so I would either remove it OR try to implement proper OOO and for that proper implementation a bool flag might be enough.

Sorry for confusion and thanks for making me realize this. Let me know if you want to either remove this and work on a proper solution 💪🏽

cc @saswatamcode for extra eyes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants