Skip to content

Commit

Permalink
PromQL: Always enable negative offset and @ modifier
Browse files Browse the repository at this point in the history 
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 <beorn@grafana.com>
  • Loading branch information
@beorn7
beorn7 committed Jan 11, 2022
1 parent 61509fc commit a532649
Show file tree
Hide file tree
Showing 13 changed files with 31 additions and 268 deletions.
12 changes: 2 additions & 10 deletions cmd/prometheus/main.go
View file
Expand Up @@ -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

Expand All @@ -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.")
Expand All @@ -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)
}
Expand Down Expand Up @@ -570,8 +564,6 @@ 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,
}

queryEngine = promql.NewEngine(opts)
Expand Down
10 changes: 3 additions & 7 deletions cmd/promtool/main.go
View file
Expand Up @@ -57,7 +57,6 @@ import (
"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/model/rulefmt"
"github.com/prometheus/prometheus/notifier"
"github.com/prometheus/prometheus/promql"
"github.com/prometheus/prometheus/scrape"
)

Expand Down Expand Up @@ -203,17 +202,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)
}
Expand Down Expand Up @@ -258,7 +254,7 @@ func main() {
os.Exit(QueryLabels(*queryLabelsServer, *queryLabelsName, *queryLabelsBegin, *queryLabelsEnd, p))

case testRulesCmd.FullCommand():
os.Exit(RulesUnitTest(queryOpts, *testRulesFiles...))
os.Exit(RulesUnitTest(*testRulesFiles...))

case tsdbBenchWriteCmd.FullCommand():
os.Exit(checkErr(benchmarkWrite(*benchWriteOutPath, *benchSamplesFile, *benchWriteNumMetrics, *benchWriteNumScrapes)))
Expand Down
12 changes: 6 additions & 6 deletions cmd/promtool/unittest.go
View file
Expand Up @@ -39,11 +39,11 @@ import (

// RulesUnitTest does unit testing of rules based on the unit testing files provided.
// More info about the file format can be found in the docs.
func RulesUnitTest(queryOpts promql.LazyLoaderOpts, files ...string) int {
func RulesUnitTest(files ...string) int {
failed := false

for _, f := range files {
if errs := ruleUnitTest(f, queryOpts); errs != nil {
if errs := ruleUnitTest(f); errs != nil {
fmt.Fprintln(os.Stderr, " FAILED:")
for _, e := range errs {
fmt.Fprintln(os.Stderr, e.Error())
Expand All @@ -61,7 +61,7 @@ func RulesUnitTest(queryOpts promql.LazyLoaderOpts, files ...string) int {
return successExitCode
}

func ruleUnitTest(filename string, queryOpts promql.LazyLoaderOpts) []error {
func ruleUnitTest(filename string) []error {
fmt.Println("Unit Testing: ", filename)

b, err := ioutil.ReadFile(filename)
Expand Down Expand Up @@ -96,7 +96,7 @@ func ruleUnitTest(filename string, queryOpts promql.LazyLoaderOpts) []error {
// Testing.
var errs []error
for _, t := range unitTestInp.Tests {
ers := t.test(evalInterval, groupOrderMap, queryOpts, unitTestInp.RuleFiles...)
ers := t.test(evalInterval, groupOrderMap, unitTestInp.RuleFiles...)
if ers != nil {
errs = append(errs, ers...)
}
Expand Down Expand Up @@ -152,9 +152,9 @@ type testGroup struct {
}

// test performs the unit tests.
func (tg *testGroup) test(evalInterval time.Duration, groupOrderMap map[string]int, queryOpts promql.LazyLoaderOpts, ruleFiles ...string) []error {
func (tg *testGroup) test(evalInterval time.Duration, groupOrderMap map[string]int, ruleFiles ...string) []error {
// Setup testing suite.
suite, err := promql.NewLazyLoader(nil, tg.seriesLoadingString(), queryOpts)
suite, err := promql.NewLazyLoader(nil, tg.seriesLoadingString())
if err != nil {
return []error{err}
}
Expand Down
35 changes: 6 additions & 29 deletions cmd/promtool/unittest_test.go
View file
Expand Up @@ -15,19 +15,16 @@ package main

import (
"testing"

"github.com/prometheus/prometheus/promql"
)

func TestRulesUnitTest(t *testing.T) {
type args struct {
files []string
}
tests := []struct {
name string
args args
queryOpts promql.LazyLoaderOpts
want int
name string
args args
want int
}{
{
name: "Passing Unit Tests",
Expand Down Expand Up @@ -79,43 +76,23 @@ func TestRulesUnitTest(t *testing.T) {
want: 1,
},
{
name: "Disabled feature (@ modifier)",
args: args{
files: []string{"./testdata/at-modifier-test.yml"},
},
want: 1,
},
{
name: "Enabled feature (@ modifier)",
name: "@ modifier always works",
args: args{
files: []string{"./testdata/at-modifier-test.yml"},
},
queryOpts: promql.LazyLoaderOpts{
EnableAtModifier: true,
},
want: 0,
},
{
name: "Disabled feature (negative offset)",
name: "Negative offset always works",
args: args{
files: []string{"./testdata/negative-offset-test.yml"},
},
want: 1,
},
{
name: "Enabled feature (negative offset)",
args: args{
files: []string{"./testdata/negative-offset-test.yml"},
},
queryOpts: promql.LazyLoaderOpts{
EnableNegativeOffset: true,
},
want: 0,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := RulesUnitTest(tt.queryOpts, tt.args.files...); got != tt.want {
if got := RulesUnitTest(tt.args.files...); got != tt.want {
t.Errorf("RulesUnitTest() = %v, want %v", got, tt.want)
}
})
Expand Down
31 changes: 0 additions & 31 deletions docs/feature_flags.md
View file
Expand Up @@ -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`
Expand All @@ -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`
Expand Down
10 changes: 3 additions & 7 deletions docs/querying/basics.md
View file
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Expand Up @@ -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=
Expand Down
70 changes: 0 additions & 70 deletions promql/engine.go
View file
Expand Up @@ -221,12 +221,6 @@ type EngineOpts struct {
// NoStepSubqueryIntervalFn is the default evaluation interval of
// a subquery in milliseconds if no step in range vector was specified `[30m:<step>]`.
NoStepSubqueryIntervalFn func(rangeMillis int64) int64

// EnableAtModifier if true enables @ modifier. Disabled otherwise.
EnableAtModifier bool

// EnableNegativeOffset if true enables negative (-) offset values. Disabled otherwise.
EnableNegativeOffset bool
}

// Engine handles the lifetime of queries from beginning to end.
Expand All @@ -241,8 +235,6 @@ type Engine struct {
queryLoggerLock sync.RWMutex
lookbackDelta time.Duration
noStepSubqueryIntervalFn func(rangeMillis int64) int64
enableAtModifier bool
enableNegativeOffset bool
}

// NewEngine returns a new engine.
Expand Down Expand Up @@ -323,8 +315,6 @@ func NewEngine(opts EngineOpts) *Engine {
activeQueryTracker: opts.ActiveQueryTracker,
lookbackDelta: opts.LookbackDelta,
noStepSubqueryIntervalFn: opts.NoStepSubqueryIntervalFn,
enableAtModifier: opts.EnableAtModifier,
enableNegativeOffset: opts.EnableNegativeOffset,
}
}

Expand Down Expand Up @@ -386,10 +376,6 @@ func (ng *Engine) NewRangeQuery(q storage.Queryable, qs string, start, end time.
}

func (ng *Engine) newQuery(q storage.Queryable, expr parser.Expr, start, end time.Time, interval time.Duration) (*query, error) {
if err := ng.validateOpts(expr); err != nil {
return nil, err
}

es := &parser.EvalStmt{
Expr: PreprocessExpr(expr, start, end),
Start: start,
Expand All @@ -405,62 +391,6 @@ func (ng *Engine) newQuery(q storage.Queryable, expr parser.Expr, start, end tim
return qry, nil
}

var (
ErrValidationAtModifierDisabled = errors.New("@ modifier is disabled")
ErrValidationNegativeOffsetDisabled = errors.New("negative offset is disabled")
)

func (ng *Engine) validateOpts(expr parser.Expr) error {
if ng.enableAtModifier && ng.enableNegativeOffset {
return nil
}

var atModifierUsed, negativeOffsetUsed bool

var validationErr error
parser.Inspect(expr, func(node parser.Node, path []parser.Node) error {
switch n := node.(type) {
case *parser.VectorSelector:
if n.Timestamp != nil || n.StartOrEnd == parser.START || n.StartOrEnd == parser.END {
atModifierUsed = true
}
if n.OriginalOffset < 0 {
negativeOffsetUsed = true
}

case *parser.MatrixSelector:
vs := n.VectorSelector.(*parser.VectorSelector)
if vs.Timestamp != nil || vs.StartOrEnd == parser.START || vs.StartOrEnd == parser.END {
atModifierUsed = true
}
if vs.OriginalOffset < 0 {
negativeOffsetUsed = true
}

case *parser.SubqueryExpr:
if n.Timestamp != nil || n.StartOrEnd == parser.START || n.StartOrEnd == parser.END {
atModifierUsed = true
}
if n.OriginalOffset < 0 {
negativeOffsetUsed = true
}
}

if atModifierUsed && !ng.enableAtModifier {
validationErr = ErrValidationAtModifierDisabled
return validationErr
}
if negativeOffsetUsed && !ng.enableNegativeOffset {
validationErr = ErrValidationNegativeOffsetDisabled
return validationErr
}

return nil
})

return validationErr
}

func (ng *Engine) newTestQuery(f func(context.Context) error) Query {
qry := &query{
q: "test statement",
Expand Down

0 comments on commit a532649

Please sign in to comment.