Add engine param to thanos engine constructor #228
Conversation
engine/engine.go
Outdated
| @@ -175,6 +175,10 @@ func New(opts Opts) *compatibilityEngine { | |||
| ), | |||
| } | |||
|
|
|||
| if opts.Reg != nil { | |||
| opts.EngineOpts.Reg = prometheus.WrapRegistererWithPrefix("compatibility_engine_", opts.Reg) | |||
There was a problem hiding this comment.
I am not sure if compatibility_engine_ is the best name because this is an implementation detail of how we manage to support all promql functions. Can we use thanos_engine_ or whatever the prefix is for the main engine?
There was a problem hiding this comment.
So while working with thanos-io/thanos#6234, we noticed that we would basically have 3 engines: the new engine, the new engine's compatibility prometheus engine, and the regular prometheus engine in Thanos. The metrics between the compatibility prometheus engine, and the prometheus engine run in Thanos clash when registering, so the compatibility_engine prefix is only added to the compatibility prometheus engine. And we reserve the thanos_engine prefix for metrics exported by new engine.
I wonder if there is some better way of handling this. Seems like we are running duplicative engines.
There was a problem hiding this comment.
Right, but what does compatibility_engine mean to someone who is using the library? Should they even know that there is a fallback to the Prometheus engine?
What I am suggesting is we use thanos_engine_ as the prefix so we don't have to ask users to use two metric sets to monitor essentially a single component.
There was a problem hiding this comment.
Alternatively, we can have a constructor where a user injects a QueryEngine so we don't need to instantiate one internally. This way we don't need to have a third prefix and we are transparent to the user that we use a Prometheus engine for the fallback.
There was a problem hiding this comment.
Hmm. For users of this library, I think knowing which queries hit fallback engine, might be somewhat desirable, but we already have a metric for that.
I think the latter constructor approach might be preferable here.
There was a problem hiding this comment.
👍 let's try the new constructor then and see if it fixes the problem.
There was a problem hiding this comment.
Does that mean adding this prefix at QueryEngineFactory in thanos, not here?
There was a problem hiding this comment.
If I understood correctly, let's pass an instance of the original Prometheus PromQL engine to the Thanos PromQL engine via the constructor. We can re-use the same, old one, no need to create a new one with a prefix this way.
There was a problem hiding this comment.
Since it's still early days for this project and we don't have any major versions, my suggestion would be to edit the original functions instead of creating new ones. Thoughts, @fpetkovski @saswatamcode?
engine/engine.go
Outdated
| return Create(opts, promql.NewEngine(opts.EngineOpts)) | ||
| } | ||
|
|
||
| func Create(opts Opts, engine *promql.Engine) *compatibilityEngine { |
There was a problem hiding this comment.
Does the engine param need to be a promql.Engine type? I guess we can make prom in compatibilityEngine a v1.QueryEngine type. Because when thanos creates a prometheus engine it gets type casted into v1.QueryEngine.
|
Let's edit the original function 🙂 |
PradyumnaKrishna
changed the title
Updates the compatibility engine constructor to create an engine, using the fallback engine provided in the options. Since promql-engine is in early days and doesnt have any major versions, it is finalized to use orignal function and add engine to options. Changes `prom` in `compatibilityEngine` to `v1.QueryEngine` type from `promql.Engine`. Since, prom is an fallback engine and any `QueryEngine` must work with in prom. Also, when thanos creates a engine it get type casted into `v1.QueryEngine`, that can't be passed to this constructor as an fallback engine option. Signed-off-by: Pradyumna Krishna <[email protected]>
PradyumnaKrishna
force-pushed
the
registerer-prefix
branch
from
a08ca72
to
6b01709
Compare
Prometheus and Thanos engine both uses same registerer to register the metrics. A error raises when we try to register a metric multiple time in promql engine.
To avoid them I seperated thanos promql engine metrics with prefixcompatibility_engine_.Edit: Added
enginefor fallback engine in EngineOpts. If engine is not provided then creates one otherwise uses the same.Also, change
promtype tov1.QueryEnginefrom*promql.Enginein thanosCompatibilityEngine.Related: thanos-io/thanos#6234