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: Propagate headers/warnings/stats from quantile downstreams #13881

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

benclive
Copy link
Contributor

What this PR does / why we need it:
Passes headers from Downstreams back to the caller

  • This caused a bug where the cache header was injected in downstreams after sharding, but wasn't passed back up to the query frontend, so the results were never cached.
  • I copied the implementation from the StreamAccumulator in the same file - so this also passes back warnings & statistics if they are present.
  • I added a test to confirm.

While debugging, I added a Span for when the CacheGenHeader is added, and you can see the span is triggered, and the header injected, in the Downstream call but never passed all the way up to the ResultsCache.Do because of this bug:
image

Trace from my local instance showing that 3/4 intervals now hit the cache:
image

@benclive benclive requested a review from a team as a code owner August 14, 2024 09:53
Copy link
Contributor

@jeschkies jeschkies left a comment

Choose a reason for hiding this comment

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

I see now what I did wrong. The stats, warnings etc are joined in Downstream here but my accumulator is returning only one result.

I don't know how adventurous your are but the current state has a little code smell. I think the join of the stats should be moved out of Downstream. Furthermore, if you follow the code for the streams you'll see that we run iter.NewSortEntryIterator on the results. However, NewStreamAccumulator(params) returns only one result. So the sort iterator is not really required.

I suggest to change the Accumulator.Result return type to logqlmodel.Result instead of a slice. And then move the logic of the merge for the BufferedAccumulator. I'd do this as a follow up. The big work is that this would require moving the logic of NewMergeLastOverTimeStepEvaluator, NewMergeFirstOverTimeStepEvaluator and NewConcatStepEvaluator into new Accumulators.

Stepping back a little, my refactoring from #11863 was intended to go down that path.

Let me explain why this makes sense:

  1. It will enable us to move the joining of stats into a central logic so it's not lost on further refactoring.
  2. Accumulate and drop samples as they arrive. E.g. last_over_time does not have to buffer all the results anymore. Same with sum by in the future. This should reduce the memory footprint a little.
  3. This will allow us to define special topk or sum by accumulators in the future..
  4. The previous point will then enable us to print and investigate a query plan without running the step evaluators.

TLDR:

  1. We should land this and verify that it works.
  2. Replace BufferedAccumulator with ConcatAccumulator, LastOverTimeAccumulator and FirstOverTimeAccumulator.

@benclive benclive merged commit a0c7598 into main Aug 29, 2024
63 checks passed
@benclive benclive deleted the propagate-headers-from-quantile-downstreams branch August 29, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants