Update PromQL from upstream #1295

Harkishen-Singh · 2022-04-12T12:50:09Z

This PR updates the PromQL module with the latest commits from upstream. It also contains the newly implemented feature for PromQL evaluation statistics.

cevian · 2022-04-28T20:13:35Z

Checks aren't passing so taking myself off the review. Please request re-review when this PR is ready and all checks pass

paulfantom · 2022-05-16T07:19:10Z

go.mod

 	google.golang.org/grpc v1.46.0
 	google.golang.org/protobuf v1.28.0
 	gopkg.in/yaml.v2 v2.4.0
 )

 // Make sure Prometheus version is pinned as Prometheus semver does not include Go APIs.
-replace github.com/prometheus/prometheus => github.com/prometheus/prometheus v1.8.2-0.20220117154355-4855a0c067e2


Do you think we can update to latest prometheus and not use git hash anymore?

If you mean @latest, I think we should stick to git hash (monthly wise). Otherwise, I have seen incompatibility with @latest mod version, which is not good, as anyone who updates the mod next might have to fix the incompatibilities.

paulfantom · 2022-05-16T07:19:57Z

pkg/api/common.go

 	"strconv"
 	"strings"
 	"time"

+	"github.com/grafana/regexp"


Why using this package instead of default golang one? I mean what are the benefits?

Well, the point is to make outputs similar to what Prometheus does. Recently, Prometheus shifted from go regex to grafana regex. I don't know the reason, will try to see the godoc APIs. But, there might be an edge case in response comparison if we shifted back to the original.

This repo is a fork of the upstream Go regexp package, with some code optimisations to make it run faster.

Source

antekresic

Looks good in general.

I'd like us to verify that these traces that are now being generated in the query engine are compatible with our setup and that we can actually use them ourselves.

Harkishen-Singh · 2022-05-23T13:50:51Z

@antekresic yes, it does. We can apply jaeger traces endpoint and get the input. We can also provide otel-collector and direct the traces there and then back to promscale.

niksajakovljevic · 2022-05-24T09:16:01Z

pkg/promql/fuzz.go

+	if warning != nil {
+		// An invalid content type is being passed, which should not happen
+		// in this context.
+		panic(warning)


Should we really panic here because of invalid contentType? I am not sure where this is being used

This is a direct copy from Prometheus

https://github.com/prometheus/prometheus/blob/957092451175e9c4b37c52b6d3c8444c1d3eebc4/promql/fuzz.go#L64

But I agree, we can replace that with a warning log.

Yeah I've noticed it's borrowed code. Just wanted to make sure we have full understanding and doing the right thing in our context.

niksajakovljevic · 2022-05-24T09:18:15Z

pkg/api/query_range.go

@@ -89,6 +89,7 @@ func queryRange(promqlConf *query.Config, queryEngine *promql.Engine, queryable

 		qry, err := queryEngine.NewRangeQuery(
 			queryable,
+			&promql.QueryOpts{EnablePerStepStats: true},


Can you explain why we want stats enabled? Do we use these stats?

This allows the stats feature to work.

promscale/pkg/promql/engine.go

Line 479 in 343bfa8

sampleStats: stats.NewQuerySamples(ng.enablePerStepStats && opts.EnablePerStepStats),

If you see how the feature works, there is a logical AND condition between

Input of -enable-feature=promql-per-step-stats that is supplied in the CLI (ng.enablePerStepStats part)

Input from the caller creating query (range & instant) (opts.EnablePerStepStats part). This one is the line you commented on

So, the feature will work only when 1 && 2 == true. 1 is based on user, 2 is Promscale enabling the feature. Hence, we pass in &promql.QueryOpts{EnablePerStepStats: true} so that the stats feature is now enabled.

why we want stats enabled?

It's a PromQL feature that we were lacking. Prometheus provides it, but we don't, as it was recently merged into Prometheus master. With this PR, we are again up to date with the master.

Note: We do not enable this, the enabling will happen only when the user passes in -enable-feature=promql-per-step-stats. We just do not want to block the feature from being enabled.

niksajakovljevic · 2022-05-24T14:16:35Z

@Harkishen-Singh Btw since you do mention that this brings in new features. Do we miss some tests for new features? I've only seen fuzz_test as a new test file, but missed to find one on statistics?

Harkishen-Singh · 2022-05-24T14:18:17Z

Test on this new feature is again borrowed from the Prometheus side in this case. That should be enough.

promscale/pkg/promql/engine_test.go

Line 761 in 343bfa8

func TestQueryStatistics(t *testing.T) {

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com> This commit updates the PromQL code upto commit 3e4bd4d9135200f6e74a2ba3ef47dba1de8656d9 This commit also includes the changes for the new PromQL stats support which is an optional feature.

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com> This commit enables the `promql-per-step-stats` feature. This is done by passing `&QueryOpts{EnablePerStepStats: true}` when creating a query. This result goes with logical AND with `-enable-feature=promql-per-step-stats` applied as Promscale starts.

Harkishen-Singh self-assigned this Apr 12, 2022

Harkishen-Singh mentioned this pull request Apr 12, 2022

Implement PromQL stats as feature options #1296

Closed

Harkishen-Singh force-pushed the update_promql_april_7 branch from a000390 to 6afe7ee Compare April 25, 2022 11:09

Harkishen-Singh marked this pull request as ready for review April 25, 2022 11:09

Harkishen-Singh requested review from niksajakovljevic and antekresic as code owners April 25, 2022 11:09

Harkishen-Singh requested a review from cevian April 25, 2022 11:10

Harkishen-Singh force-pushed the update_promql_april_7 branch from 6afe7ee to b1b8af4 Compare April 26, 2022 11:26

cevian removed their request for review April 28, 2022 20:14

Harkishen-Singh force-pushed the update_promql_april_7 branch from b1b8af4 to d85e3be Compare May 13, 2022 11:15

Harkishen-Singh requested review from paulfantom and a team as code owners May 13, 2022 11:15

Harkishen-Singh force-pushed the update_promql_april_7 branch from d85e3be to 99bf22b Compare May 13, 2022 11:16

paulfantom reviewed May 16, 2022

View reviewed changes

Harkishen-Singh force-pushed the update_promql_april_7 branch from 99bf22b to 343bfa8 Compare May 23, 2022 07:21

Harkishen-Singh requested a review from paulfantom May 23, 2022 07:21

antekresic reviewed May 23, 2022

View reviewed changes

Harkishen-Singh requested a review from antekresic May 23, 2022 13:52

niksajakovljevic reviewed May 24, 2022

View reviewed changes

Harkishen-Singh requested a review from niksajakovljevic May 24, 2022 13:49

niksajakovljevic approved these changes May 24, 2022

View reviewed changes

antekresic approved these changes May 24, 2022

View reviewed changes

Harkishen-Singh added 2 commits May 25, 2022 14:10

Update PromQL till March 30

7114d18

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com> This commit updates the PromQL code upto commit 3e4bd4d9135200f6e74a2ba3ef47dba1de8656d9 This commit also includes the changes for the new PromQL stats support which is an optional feature.

Update query APIs & use grafana/regexp to align with Prometheus.

ab791c9

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>

Harkishen-Singh force-pushed the update_promql_april_7 branch from 343bfa8 to 7ce6fa7 Compare May 25, 2022 08:42

Harkishen-Singh enabled auto-merge (rebase) May 25, 2022 08:42

Harkishen-Singh disabled auto-merge May 25, 2022 08:45

Harkishen-Singh force-pushed the update_promql_april_7 branch from 7ce6fa7 to 7e51415 Compare May 25, 2022 08:53

Harkishen-Singh enabled auto-merge (rebase) May 25, 2022 08:59

Harkishen-Singh merged commit a5ffe9d into timescale:master May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PromQL from upstream #1295

Update PromQL from upstream #1295

Harkishen-Singh commented Apr 12, 2022 •

edited

Loading

cevian commented Apr 28, 2022

paulfantom May 16, 2022

Harkishen-Singh May 23, 2022

paulfantom May 16, 2022

Harkishen-Singh May 23, 2022

Harkishen-Singh May 23, 2022

antekresic left a comment

Harkishen-Singh commented May 23, 2022

niksajakovljevic May 24, 2022

Harkishen-Singh May 24, 2022

niksajakovljevic May 24, 2022

niksajakovljevic May 24, 2022

Harkishen-Singh May 24, 2022

Harkishen-Singh May 24, 2022 •

edited

Loading

niksajakovljevic commented May 24, 2022

Harkishen-Singh commented May 24, 2022

Update PromQL from upstream #1295

Update PromQL from upstream #1295

Conversation

Harkishen-Singh commented Apr 12, 2022 • edited Loading

cevian commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antekresic left a comment

Choose a reason for hiding this comment

Harkishen-Singh commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harkishen-Singh May 24, 2022 • edited Loading

Choose a reason for hiding this comment

niksajakovljevic commented May 24, 2022

Harkishen-Singh commented May 24, 2022

Harkishen-Singh commented Apr 12, 2022 •

edited

Loading

Harkishen-Singh May 24, 2022 •

edited

Loading