search

grafana / loki

Like Prometheus, but for logs.
https://grafana.com/loki
GNU Affero General Public License v3.0
23.98k stars 3.46k forks source link

chore: Simplify streams and metrics accumulation in frontend #14148

Open jeschkies opened 2 months ago

jeschkies commented 2 months ago

This issue comes from https://github.com/grafana/loki/pull/13881#pullrequestreview-2237990726:

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 https://github.com/grafana/loki/pull/11863 was intended to go down that path.

Let me explain why this makes sense:

It will enable us to move the joining of stats into a central logic so it's not lost on further refactoring.

  1. 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.
  2. This will allow us to define special topk or sum by accumulators in the future..
  3. 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.
jeschkies commented 2 months ago

The problem I see with this approach is that we have iterators and accumulators then. Not sure if we want to go with these two concepts.