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 offset finder #681

Closed
wants to merge 2 commits into from
Closed

Conversation

arkbriar
Copy link

@arkbriar arkbriar commented Sep 10, 2024

Fixes #678

The offset finder didn't check the offset value, which incorrectly found an offset of 0 in the query below

max(max_over_time(sum(rate(stream_executor_row_count{namespace=~"default"}[30s] offset 30m))[12h2m27s:30s]))

, and it returns an error of mismatched offsets which is swallowed silently.

With the fix, the query above correctly triggered QueryRange, as showed below.

DEBU[2024-09-10T19:55:38+08:00] http://localhost:8428                         api=QueryRange query="sum(rate(stream_executor_row_count{namespace=~\"default\"}[30s]))" r="{2024-09-08 23:23:30 +0000 UTC 2024-09-10 11:25:38.141 +0000 UTC 30s}"

The request time range is [2024-09-08 23:53:30 +0000 UTC 2024-09-10 11:55:38.141] and the offset 30m was respected.

Signed-off-by: arkbriar <[email protected]>
Signed-off-by: arkbriar <[email protected]>
@arkbriar
Copy link
Author

arkbriar commented Sep 10, 2024

I suddenly realise that sum shouldn't be pushed down since merging sum won't produce a correct result. To correctly handle the sum, promxy has to merge the original time series matrix and calculate itself. However, after I looked into the query_range API, I found that it doesn't support streaming results which makes it impossible to reduce the memory footprint by merging results from servers without materialising the data (i.e., with a merged iterator).

I don't know what to do now. The PR merely makes certain queries being able to be pushed down. But it's fixing a bug upon another and becomes meaningless.

@jacksontj
Copy link
Owner

Closing this PR to take the convo back to #678 :)

@jacksontj jacksontj closed this Sep 14, 2024
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.

offset modifier causing OOM
2 participants