Logql/parallel binop

[owen-d]

This PR does a few things:

1. Runs both legs of a binary operation in parallel.
2. Introduces the Clone() method for our Exprs to ensure we run into mutability bugs less
3. Uses (2) in our sharding code to prevent a mutability bug, in this case one which prevented binary operations from being sharded. This bug has existed since 2020 😭 and of course I'm the original author 😱
4. Increases the Downstreamer concurrency. Now that we have the LimitedRoundTripper, this structure is largely used to just prevent goroutine explosions via malicious queries, so it feels safe to increase this as it no longer needs to limit access to our tenant queues.

Running this in one of our clusters resulted in sharded binary operations running ~10x faster 🎉

[kavirajk]

LGTM! happy all my ratio queries (exp1)/(exp2) is going to run faster now :)

Left one minor clarification though.

[kavirajk]

Q: Failing to understand why it got increased (even after reading below comment on Downstreamer())

[cyriltovena]

Any way we can line it up with the max_query_parallelism ? Because 128 is half the biggest tenant we have at 256 so he will get limited by this.

[cyriltovena]

Or may be cap it higher ?

[owen-d]

This was historically used to limit how many downstream queries (query-frontend -> querier) could be dispatched in parallel by a single time splitted logql query. Now that this part is controlled by the LimitedRoundTripper instead, we only want to use the Downstreamer concurrency to prevent us from creating unbounded goroutines. Increasing the limit to 128 still seems reasonable to that effect but is also high enough to not pre-limit anything the LimitedRoundTripper would limit anyway. Basically, this is a crude attempt to prevent us from blowing up goroutines due to malicious queries without introducing a bottleneck in our query path.

[kavirajk]

Thanks for clarifying Owen. 👍

Two things:

1. Can we add this

 Increasing the limit to 128 still seems reasonable to that effect but is also high enough to not pre-limit anything the LimitedRoundTripper would limit anyway. 

as a comment to the const itself?

2. Also +1 to have this value same as max_query_parallelism. Because having max_query_parallelism as 256 will be allowed happily by LimitedRoundTripper but still can get limited by this Downstreamer.?

[owen-d]

Any way we can line it up with the max_query_parallelism ? Because 128 is half the biggest tenant we have at 256 so he will get limited by this.

Yes, we could, but this is also post-splitted code, meaning that each split would need to schedule more than 128 queries for this to limit it. We may ultimately want to thread it into the MaxQueryParallelism code, but I felt that was overengineering for the moment and could be done in another PR if needed.

[cyriltovena]

Sounds good !