Query: Switch Multiple Engines

[PradyumnaKrishna]

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

Thanos query has two engine, prometheus (default) and thanos. A single engine runs through thanos query command at a time, and have to re run the command to switch between.

This PR aims to add a functionality to run multiple engines at once and switch between them using query UI.

QueryEngineFactory is added that creates PromQL engine on demand and returns a engine using method GetXEngine(), where X is a engine name. Two engines are added GetPrometheusEngine and GetThanosEngine.

UI Changes

Verification

Tests are passing, tested locally with both query params.

[saswatamcode]

Thanks for awesome work! This already looks really good!

Some suggestions! Let's also keep a flag for default engine preference and update tests/ui. 🙂

[saswatamcode]

Do we need to define this separately? I think we'd just be able to use engineOpts directly! 🙂

[saswatamcode]

Let's pass in the default engine preference from the flag here too!

[saswatamcode]

Maybe using an enum instead of a string here would be better?

Something like,

enum EngineType {
  THANOS = 0;
  PROMETHEUS = 1;
}

[PradyumnaKrishna]

I am not sure how to handle default case with this?

[saswatamcode]

Why not? Would be the same as erroring out in the HTTP API, no? 🙂

[PradyumnaKrishna]

For enums, the default value is the first defined enum value, which must be 0. THANOS in this case.

https://protobuf.dev/programming-guides/proto3/#default

[saswatamcode]

Ah yes, it is. I was talking about how to handle default in gRPC API switch case

[PradyumnaKrishna]

the enum by default choose prometheus if not provided. where in string a empty string is form and we can use defaultEngine value if empty.

Does that mean to add a default engineType in enum?

[saswatamcode]

I think erroring out here would be better, rather than just implicitly using an engine.

[saswatamcode]

Ditto, let's error out with an ApiError in default case, where param is not empty, but also doesn't match valid values.

[saswatamcode]

If this is empty, let's fallback to engine specified via the flag.

[GiedriusS]

Maybe let's call this prometheusEngine? Perhaps we could even put this collection of engines behind another struct like:

type QueryEngineFactory struct {
...
}

func (f *QueryEngineFactory) GetPrometheusEngine(...) v1.QueryEngine {
}

func (f *QueryEngineFactory) GetThanosEngine(...) v1.QueryEngine {
}

With this, we wouldn't even have to create those engines in advance. We could create them on the first call to GetXEngine 🤔

[saswatamcode]

Thanks for awesome work, and for adding the UI changes as well!

Some suggestions! I think you can also mark this as "Ready for review".

[saswatamcode]

Suggested change

	de PromqlEngineType,
	default PromqlEngineType,

[saswatamcode]

Suggested change

	defaultEngine: de,
	defaultEngine: defaultEngine,

[saswatamcode]

Once thanos-io/promql-engine#228 goes in, let's change this to something like,

Suggested change

	f.thanosEngine = engine.New(engine.Opts{EngineOpts: f.engineOpts})
	f.thanosEngine = engine.New(engine.Opts{EngineOpts: f.engineOpts, Engine: f.GetPrometheusEngine()})

[saswatamcode]

Do we need to export this?

[saswatamcode]

Right, I think we can get the default engine from the flag API call i.e flagRes here, and pass it to Panel, where we render the dropdown accordingly, instead of hardcoding to prometheus.

[saswatamcode]

Maybe having this select item in Panel instead of PanelList would be a better idea, similar to the Use Deduplication option?
The reason for this is that we might want to compare the results of two engines on the same screen, with different query panels. So I think having the dropdown on every query panel would be amazing.

[saswatamcode]

Actually, do we need a completely separate component just for single select input? 🙂

Maybe it would just be easier to add this <Input> to Panel? We have an example of this in GraphControl

thanos/pkg/ui/react-app/src/pages/graph/GraphControls.tsx

Line 158 in 96fd6db

<Input

[saswatamcode]

I think we can skip this completely and just render it, instead of having a dedicated component, as mentioned above. 🙂

[saswatamcode]

Why not? Would be the same as erroring out in the HTTP API, no? 🙂

[GiedriusS]

LGTM 🙇 🎖️

[GiedriusS]

I think you'll need to merge (or rebase) the newest origin/main into your branch to make the CI pass.

[saswatamcode]

Awesome work!