Query: Enable promql-experimental-functions #8191
Conversation
Signed-off-by: Sujal Gupta <[email protected]>
Signed-off-by: Sujal Gupta <[email protected]>
Signed-off-by: Sujal Gupta <[email protected]>
Adjust the default get-config timeout to match the default get-config interval. Signed-off-by: Michael Hoffmann <[email protected]> Signed-off-by: Sujal Gupta <[email protected]>
Signed-off-by: Sujal Gupta <[email protected]>
Signed-off-by: Sujal Gupta <[email protected]>
| // Set global experimental functions flag | ||
| parser.EnableExperimentalFunctions = enablePromQLExperimentalFunctions | ||
|
|
There was a problem hiding this comment.
Just a Question: As I was working with this in engine (re: thanos-io/promql-engine#547) wonder if its ok to have this flag set logic in thanos side or we should keep parser related logic limited to engine.
There was a problem hiding this comment.
Perhaps, adding a new field in engine struct like EnableExperimentalFunctions might be a good way if want to continue with later 👀
https://github.com/thanos-io/promql-engine/blob/main/engine/engine.go#L57C1-L84C2 and then when we do engine.New we can use this logic there (parser.EnableExperimentalFunctions=EnableExperimentalFunctions)
There was a problem hiding this comment.
If it sets the global in parser package, wouldn't that reflect everywhere (including in the thanos engine)? I think this is an ok pattern to follow. Would having extra option on the engine struct help other cases? 🙂
There was a problem hiding this comment.
Ah right!
Current looks to be right then
We can resolve this!
| @@ -135,7 +136,7 @@ func registerQuery(app *extkingpin.App) { | |||
|
|
|||
| activeQueryDir := cmd.Flag("query.active-query-path", "Directory to log currently active queries in the queries.active file.").Default("").String() | |||
|
|
|||
| featureList := cmd.Flag("enable-feature", "Comma separated experimental feature names to enable.The current list of features is empty.").Hidden().Default("").Strings() | |||
| featureList := cmd.Flag("enable-feature", "Comma separated experimental feature names to enable. The current list of features is: promql-experimental-functions (enables experimental PromQL functions).").Hidden().Default("").Strings() | |||
There was a problem hiding this comment.
Does this flag still need to be hidden, I wonder
| // Set global experimental functions flag | ||
| parser.EnableExperimentalFunctions = enablePromQLExperimentalFunctions | ||
|
|
There was a problem hiding this comment.
If it sets the global in parser package, wouldn't that reflect everywhere (including in the thanos engine)? I think this is an ok pattern to follow. Would having extra option on the engine struct help other cases? 🙂
| @@ -317,6 +318,7 @@ func registerQuery(app *extkingpin.App) { | |||
| *enableTargetPartialResponse, | |||
| *enableMetricMetadataPartialResponse, | |||
| *enableExemplarPartialResponse, | |||
| false, | |||
There was a problem hiding this comment.
if --enable-feature=promql-experimental-functions is set, this will still take it as false.
maybe we can intercept by checking from here if the set value is promql-experimental-functions and pass true/false based on that.
| DefaultTenant string | ||
| TenantCertField string | ||
| EnableXFunctions bool | ||
| EnableQueryExperimentalFunctions bool |
There was a problem hiding this comment.
we can use this field at cmd/thanos/query_frontend.go now
e.g. https://github.com/thanos-io/thanos/blob/main/cmd/thanos/query_frontend.go#L161
Changes
Fixes #7640
In
thanos/query.goAdded entry for the same in
pkg/api/query/engine.go,pkg/queryfrontend/config.go, andtest/e2e/e2ethanos/services.goVerification