PromQL: Promote negative offset and @ modifer to stable

[beorn7]

I hereby propose to promote the negative offset and the @ modifer to stable with v2.33. This PR can be used to discuss it. If we decide to postpone the promotion, we can keep it around and merge it once we are ready to do so.

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.

That's what this PR implements.

However, personally I still don't consider these features breaking changes. The invariant is only broken for queries that were invalid queries before the feature existed. I see how the new features are problematic for implementers of caching proxies, but there was never an explicit stability guarantee that PromQL would never have features that allow accessing samples newer than the query time. When we start to consider emerging behavioral patterns as stability guarantees, we could declare many new features or even improvements and bug fixes as breaking changes, breaking the invariant that they surely change some behavior. It's a question where to draw the line. That's ultimately a pragmatic decision without a precise answer. We probably all agree that the obligatory XKCD comic draws the line at the wrong level, but this case is more problematic. I would like to find out where you all would draw the line in this case. Perhaps we can after all just drop the feature flag and enable the features by default already now.

[codesome]

LGTM

[juliusv]

I'm for just promoting it and removing the feature flag.

…

On Thu, Jan 6, 2022, 11:20 Ganesh Vernekar ***@***.***> wrote: ***@***.**** approved this pull request. LGTM — Reply to this email directly, view it on GitHub <#10121 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEDLGEOSDPS2DASDF6BKQDUUVUHDANCNFSM5LKEAJQA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because your review was requested.Message ID: ***@***.***>

[roidelapluie]

I now also think if we have not a list of invariants documented, this is not a breaking change. Especially because it does not change existing queries results. This is an extra fonctionality at most .

[beorn7]

Thanks for the feedback. Since so far, you all agreed that the negative offset and the @ modifier should be a regular "always on" feature even in 2.x, I'll amend this PR to do so later today. But I'll wait until Wednesday to see if there are objections.

[beorn7]

I pushed an additional commit that permanently enables negative offset and the @ modifier. If we decide that's the way to go, we leave the commit in. If we decide to keep those features behind a feature flag until v3.x, we simply remove the commit from the PR again.

Please let me know what you think. It would be cool to make the call by Wednesday, when I plan to cut the release candidate. (And since it is a release candidate only, we can still change our mind before the final release.)

[pstibrany]

I pushed an additional commit that permanently enables negative offset and the @ modifier. If we decide that's the way to go, we leave the commit in. If we decide to keep those features behind a feature flag until v3.x, we simply remove the commit from the PR again.

In Cortex we would prefer to be able to disable negative offsets (via field in EngineOpts struct) until we explore how to handle them properly with regards to caching. We also currently expose CLI flag to disable @-modifier, but we could remove that flag.

[beorn7]

So what does everyone think about leaving the various Opts structs in place, but not exposing them in prometheus and promtool?

[beorn7]

To extend on the latter: Those Opts structs and their handling would be "dead code" from the Prometheus perspective, but once we introduce the next feature flag for PromQL, it would come in handy to have all the plumbing in place already.

Since it also helps Cortex, I tend to do it that way (with some doc comments so that devs understand why it's there and aren't tempted to rip it out as "dead code").

If there is no other feedback within the next hours, I will amend this PR again to implement that idea.

[roidelapluie]

Side note: if we enable them by default, they would become de facto part of the PromQL compatibility suite, so cloud providers and downstream projects will need to support that to be "Prometheus Compatible".

[roidelapluie]

So what does everyone think about leaving the various Opts structs in place, but not exposing them in prometheus and promtool?

SGTM

[beorn7]

OK, done. This now has the plumbing for the options in place, but in the prometheus and promtool binaries, both features are always enabled.

[roidelapluie]

Extra nits:

• Now those features are undocumented.
• They should be always on in promql tests, too. Is it the case?

[beorn7]

The features are documented:

• https://github.com/prometheus/prometheus/blob/beorn7/promql/docs/querying/basics.md#-modifier
• https://github.com/prometheus/prometheus/blob/beorn7/promql/docs/querying/basics.md#offset-modifier

About the tests: You can pick for tests if they should be enabled or not, and both is still tested, which is important because we should still test the behavior when they are disabled as we are offering that behavior to users of the engine as a library.

[beorn7]

Oops, perhaps you were thinking about the PromQL tests (in testdata/...), which I ignored so far.

In fact, they already had EnableAtModifier enabled always, but not EnableNegativeOffset (simply because none of those tests used that). I will enable the latter, too, and add a test case with a negative offset.

[juliusv]

Looks good, thanks! 👍

For keeping the old options around, we just shouldn't do that for too long, as we shouldn't incentivize anyone using the PromQL engine code in a way that is not PromQL comaptible anymore. Maybe we can keep it for the same time frame as compliant and compatible marks are meant to be valid for: "Both compliant and compatible marks are valid for 2 minor releases or 12 weeks, whichever is longer." (from https://prometheus.io/blog/2021/05/03/introducing-prometheus-conformance-program/)

[beorn7]

I tend to agree. To make things easier for cache implementers, perhaps we can provide a way to find out which time range a query covers rather than just tagging features that potentially access a time range ahead of the query time.

In any case, that's for the future. I'll merge this now and will work on the changelog.