From 61509fc8400be7418869343d1338beae8db2e9a7 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 5 Jan 2022 15:50:16 +0100 Subject: [PATCH 1/3] PromQL: Promote negative offset and @ modifer to stable Following the argument that breaking the invariant that PromQL does not look ahead of the evaluation time implies a breaking change, we still need to keep the feature flag around, but at least we can communicate that the feature is considered stable, and that the feature flags will be ignored from v3 on. Signed-off-by: beorn7 --- cmd/prometheus/main.go | 4 ++-- docs/feature_flags.md | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/cmd/prometheus/main.go b/cmd/prometheus/main.go index eaa97fe32..9b662249b 100644 --- a/cmd/prometheus/main.go +++ b/cmd/prometheus/main.go @@ -168,10 +168,10 @@ func (c *flagConfig) setFeatureListOptions(logger log.Logger) error { switch o { case "promql-at-modifier": c.enablePromQLAtModifier = true - level.Info(logger).Log("msg", "Experimental promql-at-modifier enabled") + level.Info(logger).Log("msg", "promql-at-modifier enabled") case "promql-negative-offset": c.enablePromQLNegativeOffset = true - level.Info(logger).Log("msg", "Experimental promql-negative-offset enabled") + level.Info(logger).Log("msg", "promql-negative-offset enabled") case "remote-write-receiver": c.web.EnableRemoteWriteReceiver = true level.Warn(logger).Log("msg", "Remote write receiver enabled via feature flag remote-write-receiver. This is DEPRECATED. Use --web.enable-remote-write-receiver.") diff --git a/docs/feature_flags.md b/docs/feature_flags.md index fd09c4088..33d822135 100644 --- a/docs/feature_flags.md +++ b/docs/feature_flags.md @@ -18,6 +18,11 @@ They may be enabled by default in future versions. The `@` modifier lets you specify the evaluation time for instant vector selectors, range vector selectors, and subqueries. More details can be found [here](querying/basics.md#modifier). +This feature is considered stable, but since it breaks the invariant that +PromQL does not look ahead of the evaluation time, it still needs to be enabled +via this feature flag. The feature will be permanently enabled (and the feature +flag ignored) upon major release v3. + ## Expand environment variables in external labels `--enable-feature=expand-external-labels` @@ -40,6 +45,11 @@ with more recent data. More details can be found [here](querying/basics.md#offset-modifier). +This feature is considered stable, but since it breaks the invariant that +PromQL does not look ahead of the evaluation time, it still needs to be enabled +via this feature flag. The feature will be permanently enabled (and the feature +flag ignored) upon major release v3. + ## Remote Write Receiver `--enable-feature=remote-write-receiver` From b39f2739e5b01560ad8299d2579f1041a0e9ae5f Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 11 Jan 2022 17:01:02 +0100 Subject: [PATCH 2/3] PromQL: Always enable negative offset and @ modifier This follows the line of argument that the invariant of not looking ahead of the query time was merely emerging behavior and not a documented stable feature. Any query that looks ahead of the query time was simply invalid before the introduction of the negative offset and the @ modifier. Signed-off-by: beorn7 --- cmd/prometheus/main.go | 16 ++++++---------- cmd/promtool/main.go | 15 +++++++++------ docs/feature_flags.md | 31 ------------------------------- docs/querying/basics.md | 10 +++------- go.mod | 2 +- go.sum | 4 ++-- promql/engine.go | 11 +++++++++-- promql/test.go | 4 +++- web/api/v1/api.go | 10 ---------- 9 files changed, 33 insertions(+), 70 deletions(-) diff --git a/cmd/prometheus/main.go b/cmd/prometheus/main.go index 9b662249b..9f4c50071 100644 --- a/cmd/prometheus/main.go +++ b/cmd/prometheus/main.go @@ -149,8 +149,6 @@ type flagConfig struct { featureList []string // These options are extracted from featureList // for ease of use. - enablePromQLAtModifier bool - enablePromQLNegativeOffset bool enableExpandExternalLabels bool enableNewSDManager bool @@ -166,12 +164,6 @@ func (c *flagConfig) setFeatureListOptions(logger log.Logger) error { opts := strings.Split(f, ",") for _, o := range opts { switch o { - case "promql-at-modifier": - c.enablePromQLAtModifier = true - level.Info(logger).Log("msg", "promql-at-modifier enabled") - case "promql-negative-offset": - c.enablePromQLNegativeOffset = true - level.Info(logger).Log("msg", "promql-negative-offset enabled") case "remote-write-receiver": c.web.EnableRemoteWriteReceiver = true level.Warn(logger).Log("msg", "Remote write receiver enabled via feature flag remote-write-receiver. This is DEPRECATED. Use --web.enable-remote-write-receiver.") @@ -195,6 +187,8 @@ func (c *flagConfig) setFeatureListOptions(logger log.Logger) error { level.Info(logger).Log("msg", "Experimental agent mode enabled.") case "": continue + case "promql-at-modifier", "promql-negative-offset": + level.Warn(logger).Log("msg", "This option for --enable-feature is now permanently enabled and therefore a no-op.", "option", o) default: level.Warn(logger).Log("msg", "Unknown option for --enable-feature", "option", o) } @@ -570,8 +564,10 @@ func main() { ActiveQueryTracker: promql.NewActiveQueryTracker(localStoragePath, cfg.queryConcurrency, log.With(logger, "component", "activeQueryTracker")), LookbackDelta: time.Duration(cfg.lookbackDelta), NoStepSubqueryIntervalFn: noStepSubqueryInterval.Get, - EnableAtModifier: cfg.enablePromQLAtModifier, - EnableNegativeOffset: cfg.enablePromQLNegativeOffset, + // EnableAtModifier and EnableNegativeOffset have to be + // always on for regular PromQL as of Prometheus v2.33. + EnableAtModifier: true, + EnableNegativeOffset: true, } queryEngine = promql.NewEngine(opts) diff --git a/cmd/promtool/main.go b/cmd/promtool/main.go index a8098bea1..bc1b2928a 100644 --- a/cmd/promtool/main.go +++ b/cmd/promtool/main.go @@ -203,17 +203,14 @@ func main() { p = &promqlPrinter{} } - var queryOpts promql.LazyLoaderOpts for _, f := range *featureList { opts := strings.Split(f, ",") for _, o := range opts { switch o { - case "promql-at-modifier": - queryOpts.EnableAtModifier = true - case "promql-negative-offset": - queryOpts.EnableNegativeOffset = true case "": continue + case "promql-at-modifier", "promql-negative-offset": + fmt.Printf(" WARNING: Option for --enable-feature is a no-op after promotion to a stable feature: %q\n", o) default: fmt.Printf(" WARNING: Unknown option for --enable-feature: %q\n", o) } @@ -258,7 +255,13 @@ func main() { os.Exit(QueryLabels(*queryLabelsServer, *queryLabelsName, *queryLabelsBegin, *queryLabelsEnd, p)) case testRulesCmd.FullCommand(): - os.Exit(RulesUnitTest(queryOpts, *testRulesFiles...)) + os.Exit(RulesUnitTest( + promql.LazyLoaderOpts{ + EnableAtModifier: true, + EnableNegativeOffset: true, + }, + *testRulesFiles...), + ) case tsdbBenchWriteCmd.FullCommand(): os.Exit(checkErr(benchmarkWrite(*benchWriteOutPath, *benchSamplesFile, *benchWriteNumMetrics, *benchWriteNumScrapes))) diff --git a/docs/feature_flags.md b/docs/feature_flags.md index 33d822135..cb5b6fa2f 100644 --- a/docs/feature_flags.md +++ b/docs/feature_flags.md @@ -11,18 +11,6 @@ Their behaviour can change in future releases which will be communicated via the You can enable them using the `--enable-feature` flag with a comma separated list of features. They may be enabled by default in future versions. -## `@` Modifier in PromQL - -`--enable-feature=promql-at-modifier` - -The `@` modifier lets you specify the evaluation time for instant vector selectors, -range vector selectors, and subqueries. More details can be found [here](querying/basics.md#modifier). - -This feature is considered stable, but since it breaks the invariant that -PromQL does not look ahead of the evaluation time, it still needs to be enabled -via this feature flag. The feature will be permanently enabled (and the feature -flag ignored) upon major release v3. - ## Expand environment variables in external labels `--enable-feature=expand-external-labels` @@ -31,25 +19,6 @@ Replace `${var}` or `$var` in the [`external_labels`](configuration/configuratio values according to the values of the current environment variables. References to undefined variables are replaced by the empty string. -## Negative offset in PromQL - -This negative offset is disabled by default since it breaks the invariant -that PromQL does not look ahead of the evaluation time for samples. - -`--enable-feature=promql-negative-offset` - -In contrast to the positive offset modifier, the negative offset modifier lets -one shift a vector selector into the future. An example in which one may want -to use a negative offset is reviewing past data and making temporal comparisons -with more recent data. - -More details can be found [here](querying/basics.md#offset-modifier). - -This feature is considered stable, but since it breaks the invariant that -PromQL does not look ahead of the evaluation time, it still needs to be enabled -via this feature flag. The feature will be permanently enabled (and the feature -flag ignored) upon major release v3. - ## Remote Write Receiver `--enable-feature=remote-write-receiver` diff --git a/docs/querying/basics.md b/docs/querying/basics.md index f4c176ed7..8a74f0df5 100644 --- a/docs/querying/basics.md +++ b/docs/querying/basics.md @@ -209,9 +209,7 @@ can be specified: rate(http_requests_total[5m] offset -1w) -This feature is enabled by setting `--enable-feature=promql-negative-offset` -flag. See [feature flags](../feature_flags.md) for more details about -this flag. +Note that this allows a query to look ahead of its evaluation time. ### @ modifier @@ -249,10 +247,6 @@ These 2 queries will produce the same result. # offset before @ http_requests_total offset 5m @ 1609746000 -This modifier is disabled by default since it breaks the invariant that PromQL -does not look ahead of the evaluation time for samples. It can be enabled by setting -`--enable-feature=promql-at-modifier` flag. See [feature flags](../feature_flags.md) for more details about this flag. - Additionally, `start()` and `end()` can also be used as values for the `@` modifier as special values. For a range query, they resolve to the start and end of the range query respectively and remain the same for all steps. @@ -262,6 +256,8 @@ For an instant query, `start()` and `end()` both resolve to the evaluation time. http_requests_total @ start() rate(http_requests_total[5m] @ end()) +Note that the `@` modifier allows a query to look ahead of its evaluation time. + ## Subquery Subquery allows you to run an instant query for a given range and resolution. The result of a subquery is a range vector. diff --git a/go.mod b/go.mod index 3a8d250a8..cdc9eae83 100644 --- a/go.mod +++ b/go.mod @@ -64,7 +64,7 @@ require ( golang.org/x/sync v0.0.0-20210220032951-036812b2e83c golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e golang.org/x/time v0.0.0-20211116232009-f0f3c7e86c11 - golang.org/x/tools v0.1.8 + golang.org/x/tools v0.1.9-0.20211209172050-90a85b2969be google.golang.org/api v0.64.0 google.golang.org/genproto v0.0.0-20211223182754-3ac035c7e7cb google.golang.org/protobuf v1.27.1 diff --git a/go.sum b/go.sum index cae70a0c0..5d8a7b229 100644 --- a/go.sum +++ b/go.sum @@ -1785,8 +1785,8 @@ golang.org/x/tools v0.1.3/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.4/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.6-0.20210726203631-07bc1bf47fb2/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= -golang.org/x/tools v0.1.8 h1:P1HhGGuLW4aAclzjtmJdf0mJOjVUZUzOTqkAkWL+l6w= -golang.org/x/tools v0.1.8/go.mod h1:nABZi5QlRsZVlzPpHl034qft6wpY4eDcsTt5AaioBiU= +golang.org/x/tools v0.1.9-0.20211209172050-90a85b2969be h1:JRBiPXZpZ1FsceyPRRme0vX394zXC3xlhqu705k9nzM= +golang.org/x/tools v0.1.9-0.20211209172050-90a85b2969be/go.mod h1:nABZi5QlRsZVlzPpHl034qft6wpY4eDcsTt5AaioBiU= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/promql/engine.go b/promql/engine.go index 9b0a32879..8fe2fe2e4 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -222,10 +222,17 @@ type EngineOpts struct { // a subquery in milliseconds if no step in range vector was specified `[30m:]`. NoStepSubqueryIntervalFn func(rangeMillis int64) int64 - // EnableAtModifier if true enables @ modifier. Disabled otherwise. + // EnableAtModifier if true enables @ modifier. Disabled otherwise. This + // is supposed to be enabled for regular PromQL (as of Prometheus v2.33) + // but the option to disable it is still provided here for those using + // the Engine outside of Prometheus. EnableAtModifier bool - // EnableNegativeOffset if true enables negative (-) offset values. Disabled otherwise. + // EnableNegativeOffset if true enables negative (-) offset + // values. Disabled otherwise. This is supposed to be enabled for + // regular PromQL (as of Prometheus v2.33) but the option to disable it + // is still provided here for those using the Engine outside of + // Prometheus. EnableNegativeOffset bool } diff --git a/promql/test.go b/promql/test.go index 19b2dd830..5f3707b28 100644 --- a/promql/test.go +++ b/promql/test.go @@ -680,7 +680,9 @@ type LazyLoader struct { // LazyLoaderOpts are options for the lazy loader. type LazyLoaderOpts struct { - // Disabled PromQL engine features. + // Both of these must be set to true for regular PromQL (as of + // Prometheus v2.33). They can still be disabled here for legacy and + // other uses. EnableAtModifier, EnableNegativeOffset bool } diff --git a/web/api/v1/api.go b/web/api/v1/api.go index e6a6daffc..79d357cdd 100644 --- a/web/api/v1/api.go +++ b/web/api/v1/api.go @@ -378,11 +378,6 @@ func (api *API) query(r *http.Request) (result apiFuncResult) { } qry, err := api.QueryEngine.NewInstantQuery(api.Queryable, r.FormValue("query"), ts) - if err == promql.ErrValidationAtModifierDisabled { - err = errors.New("@ modifier is disabled, use --enable-feature=promql-at-modifier to enable it") - } else if err == promql.ErrValidationNegativeOffsetDisabled { - err = errors.New("negative offset is disabled, use --enable-feature=promql-negative-offset to enable it") - } if err != nil { return invalidParamError(err, "query") } @@ -458,11 +453,6 @@ func (api *API) queryRange(r *http.Request) (result apiFuncResult) { } qry, err := api.QueryEngine.NewRangeQuery(api.Queryable, r.FormValue("query"), start, end, step) - if err == promql.ErrValidationAtModifierDisabled { - err = errors.New("@ modifier is disabled, use --enable-feature=promql-at-modifier to enable it") - } else if err == promql.ErrValidationNegativeOffsetDisabled { - err = errors.New("negative offset is disabled, use --enable-feature=promql-negative-offset to enable it") - } if err != nil { return apiFuncResult{nil, &apiError{errorBadData, err}, nil, nil} } From c2b80d86437fc7766d65fd4f6b7503f0dd618be7 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 11 Jan 2022 18:23:40 +0100 Subject: [PATCH 3/3] PromQL: Test negative offset in PromQL tests Signed-off-by: beorn7 --- promql/test.go | 1 + promql/testdata/at_modifier.test | 8 ++++++++ promql/testdata/selectors.test | 6 ++++++ 3 files changed, 15 insertions(+) diff --git a/promql/test.go b/promql/test.go index 5f3707b28..1da225a3d 100644 --- a/promql/test.go +++ b/promql/test.go @@ -613,6 +613,7 @@ func (t *Test) clear() { Timeout: 100 * time.Second, NoStepSubqueryIntervalFn: func(int64) int64 { return durationMilliseconds(1 * time.Minute) }, EnableAtModifier: true, + EnableNegativeOffset: true, } t.queryEngine = NewEngine(opts) diff --git a/promql/testdata/at_modifier.test b/promql/testdata/at_modifier.test index 18cd93a4b..3ba6afc49 100644 --- a/promql/testdata/at_modifier.test +++ b/promql/testdata/at_modifier.test @@ -18,6 +18,14 @@ eval instant at 10s metric offset 50s @ 100 metric{job="1"} 5 metric{job="2"} 10 +eval instant at 10s metric @ 0 offset -50s + metric{job="1"} 5 + metric{job="2"} 10 + +eval instant at 10s metric offset -50s @ 0 + metric{job="1"} 5 + metric{job="2"} 10 + eval instant at 10s -metric @ 100 {job="1"} -10 {job="2"} -20 diff --git a/promql/testdata/selectors.test b/promql/testdata/selectors.test index c4309957b..6742d83e9 100644 --- a/promql/testdata/selectors.test +++ b/promql/testdata/selectors.test @@ -29,6 +29,12 @@ eval instant at 18000s rate(http_requests{instance!="3"}[1m] offset 10000s) {job="api-server", instance="0", group="canary"} 3 {job="api-server", instance="1", group="canary"} 4 +eval instant at 4000s rate(http_requests{instance!="3"}[1m] offset -4000s) + {job="api-server", instance="0", group="production"} 1 + {job="api-server", instance="1", group="production"} 2 + {job="api-server", instance="0", group="canary"} 3 + {job="api-server", instance="1", group="canary"} 4 + eval instant at 18000s rate(http_requests[40s]) - rate(http_requests[1m] offset 10000s) {job="api-server", instance="0", group="production"} 2 {job="api-server", instance="1", group="production"} 1