Accept promql.Engine interface in v1.NewAPI()#10050
Accept promql.Engine interface in v1.NewAPI()#10050roidelapluie merged 1 commit intoprometheus:mainfrom colega:accept-promql-engine-interface-in-api
promql.Engine interface in v1.NewAPI()#10050Conversation
Instead of requesting a concrete type. This would allow other implementations that use the same API to replace or wrap the engine implementation while maintaining the same API. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
|
I am not sure we are doing ourserlves a favor by accepting this. This is the Prometheus API, meant to be used with PromQL only. |
Yes, PromQL, but not necessarily |
|
I'm fine renaming the interface to any other name, My main reason to wrap this right now, @roidelapluie is to be able to return other errors here: Lines 453 to 458 in 6c75771 In order to avoid referring to prometheus-specific fields when this API implementation is being used by Cortex/Thanos/etc. I have considered a more complex approach by injecting an error mapper into the API, and even though it would be cleaner for the downstream implementations, it would be unnecessarily overengineered (IMO) for Prometheus itself. But we can still discuss that option too. |
roidelapluie
left a comment
There was a problem hiding this comment.
Thanks for explaining your point. The code change is small, seems fine to me.
|
Thank you! |
Instead of requesting a concrete type. This would allow other implementations that use the same API to replace or wrap the engine implementation while maintaining the same API.
Signed-off-by: Oleg Zaytsev mail@olegzaytsev.com