'@ <timestamp>' modifier #8121

codesome · 2020-10-28T12:32:37Z

This PR implements @ <timestamp> modifier as per this design doc.

An example query:

rate(process_cpu_seconds_total[1m]) 
  and
topk(7, rate(process_cpu_seconds_total[1h] @ 1234))

which ranks based on last 1h rate and w.r.t. unix timestamp 1234 but actually plots the 1m rate.

Closes #7903

This PR is to be followed up with an easier way to represent the start, end, range of a query in PromQL so that we could do @ <end>, metric[<range>] easily.

codesome · 2020-10-28T12:54:35Z

We have this prototype running at http://104.131.10.78:9090/graph if anyone wants to try it out. (Edit: this is running a prototype which can break in unknown ways. Not in sync with the PR.)

roidelapluie · 2020-10-28T12:59:25Z

Can we use scalar, instead of creating a new timestamp type?

codesome · 2020-10-28T13:03:05Z

What do you mean by a scalar here? Does the timestamp 1603888158 count as a scalar?

roidelapluie · 2020-10-28T13:03:50Z

Also, thanks for this and the demo!!

I am asking for the timestamp/scalar because the first thing I tried is up @timestamp(up) :)

roidelapluie · 2020-10-28T13:06:11Z

oooh wait the @ is not evaluated on every step, so my idea would not work.

codesome · 2020-10-28T13:06:29Z

up @timestamp(up) means the timestamp is dependent on the evaluation time, right? So it is not actually using the fixed timestamp benefit :)

codesome · 2020-10-28T13:36:11Z

Since the above server is still gathering data, here is a peek from one of our internal servers

beorn7 · 2020-10-28T13:51:15Z

I think there are benefits in making the argument of @ dynamic.

For one, we probably want it to support an upcoming function now() to, for example, graph the top 10 fullest disk as of now. But that function's return value still won't depend on the evaluation time.

However, I can see MTD-like queries where I want it to depend on the evaluation time, e.g. where you want to compare a value at time t to the value at the beginning of the month relative to t, i.e. something like @ start_of_month(time()). (The yet-to-be-created start_of_month function would probably take time() as the default, and of course there could be more elegant ways, referring to what I wrote about fields in a fully-fledged time struct.) This would allow error-budget burn graphs, which you want to reset at the beginning of a new billing period etc.

To make the latter work nicely, we probably want an @-style start of a range in a range selector. If we allowed dynamic ranges, it could look like increase(errors_total[time()-start_of_month()]), but an @-syntax could say "from then to the time of evaluation", i.e. increase(errors_total[@start_of_month()]). But I guess I'm digressing…

The final plug for a dynamic argument of @ would be to retrieve it from recording rules or do arithmetic on it, which relates to my musings about timestamps and durations.

See https://docs.google.com/document/d/1jMeDsLvDfO92Qnry_JLAXalvMRzMSB1sBr9V7LolpYM/edit#heading=h.vmb7pe7hp12

beorn7 · 2020-10-28T13:53:04Z

To be clear: The above are just musings, not concrete suggestions what to implement in this PR. I think this is great as a demo.

codesome · 2020-10-28T14:05:05Z

Yeah this prototype is just barebone stuff required for a PoC, other helpers like now() end() etc would be based on what we decide and I don't plan implementing in this PR yet.

brian-brazil · 2020-10-28T14:12:45Z

Note that this PR constitutes a breaking change to PromQL, and thus should not be merged.

codesome · 2020-10-28T14:28:28Z

I see this as an incremental additional without breaking anything that exists right now. But anyway, should not be merged yes, just a PoC.

roidelapluie · 2020-10-28T14:47:09Z

Note that this PR constitutes a breaking change to PromQL, and thus should not be merged.

I agree with @codesome that it is not breaking anything from what I can see

brian-brazil · 2020-10-28T14:59:43Z

It breaks the invariant that has existed for 5+ years that PromQL does not look into the future, and thus that no samples beyond the evaluation time of a query may be accessed. Users could be depending on this, thus it is a breaking change.

bwplotka · 2020-10-28T15:55:53Z

I am definitely in favor of this modifier, love it! Thanks, @codesome for this prototype. What I like about this syntax is that is quite clear. Sure, there is some boilerplate but it's fully understandable and does what expected.

It breaks the invariant that has existed for 5+ years that PromQL does not look into the future, and thus that no samples beyond the evaluation time of a query may be accessed. Users could be depending on this, thus it is a breaking change.

Hm @brian-brazil if that's the case isn't this as easy as returning an error when requested @timestamps is outside or requested range? 🤔

beorn7 · 2020-10-28T16:15:58Z

Users could be depending on this, thus it is a breaking change.

That's not the definition of a breaking change.

https://xkcd.com/1172/

beorn7 · 2020-10-28T16:32:07Z

if that's the case isn't this as easy as returning an error when requested @timestamps is outside or requested range?

First of all, no, it's not the case. This is not a breaking change. Everything that works before will continue to work in exactly the same way.

Second, it is core to this feature that you @-request a timestamp from outside the queried range. For example, if you have a topk graph for the busiest CPUs over the last hour, and then you want to zoom in to look at a particular peak, you are now looking at a time interval in the past but still need to run the selecting query with the same fixed @-time. (This is BTW what most competing metrics systems get wrong with the "easy" task of displaying the top-5 busiest CPUs.)

brian-brazil · 2020-10-28T17:27:45Z

Hm @brian-brazil if that's the case isn't this as easy as returning an error when requested @timestamps is outside or requested range? thinking

That would cover it, as long as it's not after the start time it can't break anyone downstream due to this.

Everything that works before will continue to work in exactly the same way.

I disagree. Anyone who depends on the not looking into the future invariant would be broken, and this is highly likely to affect at the least both explicit and implicit caching. We'd basically be asking for all users to audit all their uses of the API to see if this would impact on them, or any of their downstream users.

Just because something isn't an explicitly enumerated behaviour doesn't mean that we can change it without causing breakage.

RichiH · 2020-10-29T12:38:15Z

Is anyone aware of any software relying this current behavior or is this a theoretical concern?

A feature flag which is on for PromCorThanos and off by default would solve this if anyone is aware of anyone needing opt-in.

brian-brazil · 2020-10-29T13:10:46Z

Being theoretical is sufficient for it to be breaking. There's stuff I'd like to break that I'd expect to have zero real impact, but can't.

In terms of concrete examples the most obvious would be Trickster, but I expect this would impact on large chunks of the userbase causing non-obvious breakage. I imagine there's tons of scripts out there running queries over "past" data and caching/storing it somewhere, presuming that the query result can't change as new data comes in.

A feature flag which is on for PromCorThanos and off by default would solve this if anyone is aware of anyone needing opt-in.

I don't think this is what you meant to write, as upgrading does not equate to opting in to breaking changes.

bwplotka · 2020-10-29T13:47:18Z

I feel we can discuss if it's breaking change or not in a different thread as this is not blocking this proposal as far as I get this, right? (: We expect the timestamp to be inside the queried range.

Second, it is core to this feature that you @-request a timestamp from outside the queried range. For example, if you have a topk graph for the busiest CPUs over the last hour, and then you want to zoom in to look at a particular peak, you are now looking at a time interval in the past but still need to run the selecting query with the same fixed @-time. (This is BTW what most competing metrics systems get wrong with the "easy" task of displaying the top-5 busiest CPUs.)

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome · 2020-11-19T10:47:43Z

I have updated this PR with the actual implementation and not a prototype anymore. I still have to fix 1-2 edge cases (the test cases failing are commented out), and have the add a flag. But otherwise, it is good for a review now.

codesome · 2020-11-19T10:50:58Z

(Also, currently it only works with a timestamp, I will be adding support for start() end() soon)

codesome · 2021-01-04T12:23:22Z

Should be ready for another review. I have added the flag and docs.

brian-brazil

I didn't do a deep re-review of the main logic, but on the other hand it didn't change.

cmd/prometheus/main.go

docs/querying/basics.md

promql/testdata/at_modifier.test

promql/engine_test.go

codesome · 2021-01-07T06:06:30Z

I have fixed all the comments. I hope it is close to merge now :)

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

brian-brazil

It's looking close. I'll want to a final full review of the core logic though, just in case I missed something or a bug slipped in during the final tweaks.

cmd/prometheus/main.go

docs/querying/basics.md

promql/functions.go

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome · 2021-01-14T12:11:10Z

(In case it was missed, the PR was ready for review after the last commit)

docs/disabled_features.md

cmd/prometheus/main.go

docs/disabled_features.md

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

brian-brazil · 2021-01-20T10:49:46Z

👍

juliusv · 2021-04-18T09:23:04Z

promql/parser/ast.go

+	// Offset is the offset used during the query execution
+	// which is calculated using the original offset, at modifier time,
+	// eval time, and subquery offsets in the AST tree.
+	Offset    time.Duration


Ooops, changing the meaning of an existing field without renaming / removing it caught me off guard and introduced a bug in PromLens where it now thinks all offsets are 0 (because Offset is now always 0 after parsing). I know we don't have API stability guarantees, but in the future I would recommend in this type of situation to do it the other way around: keep the same semantics for the existing field and instead add another one for the new meaning (e.g. AdjustedOffset or something like that).

This commit bumps the dependency on Prometheus to the commit from the v2.28.0 tag (https://github.com/prometheus/prometheus/releases/tag/v2.28.0). The reason for bumping this is to take advantage of the updates to the PromQL parser. Without this update, the prom-label-proxy cannot enforce labels on queries that use any new features, e.g. queries that include: * the last_over_time, sgn, clamp functions, added in prometheus/prometheus#8457; * the @ operator, added in prometheus/prometheus#8121; or * the group aggregation operator, added in prometheus/prometheus#7480. Currently, queries including these functions produce errors like: `parse error: unknown function with name "group"` Signed-off-by: Lucas Servén Marín <lserven@gmail.com>

This commit bumps the dependency on Prometheus to the commit from the v2.28.0 tag (https://github.com/prometheus/prometheus/releases/tag/v2.28.0). The reason for bumping this is to take advantage of the updates to the PromQL parser. Without this update, the prom-label-proxy cannot enforce labels on queries that use any new features, e.g. queries that include: * the last_over_time, sgn, clamp functions, added in prometheus/prometheus#8457; * the @ operator, added in prometheus/prometheus#8121; or * the group aggregation operator, added in prometheus/prometheus#7480. Currently, queries including these functions produce errors like: `parse error: unknown function with name "group"` In order to keep tests working, we need to pull in the latest commits from Alertmanager. This, however, does not satisfy `go mod` since v0.22.2 of Alertmanager, on which Prometheus depends, is not merged into main. As a result, we need a replace directive in the go.mod file for now. Signed-off-by: Lucas Servén Marín <lserven@gmail.com>

This commit bumps the dependency on prom-label-proxy to include prometheus-community/prom-label-proxy#71. As described in that PR, the reason for bumping this is to take advantage of the updates to the PromQL parser. Without this update, the prom-label-proxy, and thus the Observatorium API cannot enforce labels on queries that use any new features, e.g. queries that include: * the last_over_time, sgn, clamp functions, added in prometheus/prometheus#8457; * the @ operator, added in prometheus/prometheus#8121; or * the group aggregation operator, added in prometheus/prometheus#7480. Currently, queries including these functions produce errors like: `parse error: unknown function with name "group"` Signed-off-by: Lucas Servén Marín <lserven@gmail.com>

bboreham · 2021-12-14T11:53:53Z

I'm pretty sure one or two blog posts were written showing how to use this feature, but I can't find them.
Would someone post the link(s) please?

roidelapluie · 2021-12-14T12:35:05Z

https://prometheus.io/blog/2021/02/18/introducing-the-@-modifier/

codesome added 5 commits November 19, 2020 16:14

Lexer and Parser additions

467aaea

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Reduce expressions to use ConstantExpr

3ae1734

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Add PromQL test cases and fix bugs

bd33448

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Fix subqueries and add tests

83a40a8

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Extrapolate results

4ddf152

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome changed the title ~~Prototype for '@ <timestamp>' modifier~~ '@ <timestamp>' modifier Nov 19, 2020

codesome force-pushed the topk-prototype branch from fee7181 to 4ddf152 Compare November 19, 2020 10:46

brian-brazil reviewed Jan 5, 2021

View reviewed changes

codesome force-pushed the topk-prototype branch from dffd260 to f4eebd7 Compare January 6, 2021 12:42

Fix review comments

7388423

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the topk-prototype branch from f4eebd7 to 7388423 Compare January 7, 2021 07:35

brian-brazil reviewed Jan 7, 2021

View reviewed changes

cmd/prometheus/main.go Outdated Show resolved Hide resolved

cmd/prometheus/main.go Show resolved Hide resolved

cmd/prometheus/main.go Outdated Show resolved Hide resolved

docs/querying/basics.md Outdated Show resolved Hide resolved

promql/functions.go Outdated Show resolved Hide resolved

Fix review comments and add more docs

06d88e1

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

brian-brazil reviewed Jan 14, 2021

View reviewed changes

docs/disabled_features.md Outdated Show resolved Hide resolved

cmd/prometheus/main.go Outdated Show resolved Hide resolved

codesome mentioned this pull request Jan 18, 2021

PromQL template variables for start and end #8377

Closed

brian-brazil reviewed Jan 19, 2021

View reviewed changes

docs/disabled_features.md Outdated Show resolved Hide resolved

docs/disabled_features.md Outdated Show resolved Hide resolved

Fix review comments

304793a

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the topk-prototype branch from d987d37 to 304793a Compare January 20, 2021 07:51

codesome merged commit 9199fcb into prometheus:master Jan 20, 2021

codesome mentioned this pull request Jan 20, 2021

PromQL: Allow negative (i.e. "future") offset #6282

Closed

This was referenced Mar 4, 2021

introduce AT modifier prometheus/lezer-promql#23

Merged

Supporting @ Expr prometheus/codemirror-promql#104

Merged

juliusv reviewed Apr 18, 2021

View reviewed changes

squat mentioned this pull request Jun 22, 2021

go,vendor: bump Prometheus dependency prometheus-community/prom-label-proxy#71

Merged

squat mentioned this pull request Jun 23, 2021

go.mod: bump prom-label-proxy observatorium/api#145

Merged

bwplotka mentioned this pull request Mar 23, 2023

Migrate Proposal from Google Doc: topk/bottomk aggregation over time prometheus/proposals#6

Open

yu-shipit mentioned this pull request Sep 29, 2023

feat(query) Implement @modifier. filodb/FiloDB#1678

Merged

3 tasks

'@ <timestamp>' modifier #8121

'@ <timestamp>' modifier #8121

codesome commented Oct 28, 2020 •

edited

codesome commented Oct 28, 2020 •

edited

roidelapluie commented Oct 28, 2020

codesome commented Oct 28, 2020

roidelapluie commented Oct 28, 2020 •

edited

roidelapluie commented Oct 28, 2020

codesome commented Oct 28, 2020

codesome commented Oct 28, 2020

beorn7 commented Oct 28, 2020

beorn7 commented Oct 28, 2020

codesome commented Oct 28, 2020

brian-brazil commented Oct 28, 2020

codesome commented Oct 28, 2020

roidelapluie commented Oct 28, 2020

brian-brazil commented Oct 28, 2020

bwplotka commented Oct 28, 2020

beorn7 commented Oct 28, 2020

beorn7 commented Oct 28, 2020

brian-brazil commented Oct 28, 2020

RichiH commented Oct 29, 2020

brian-brazil commented Oct 29, 2020

bwplotka commented Oct 29, 2020

codesome commented Nov 19, 2020

codesome commented Nov 19, 2020

codesome commented Jan 4, 2021

brian-brazil left a comment

codesome commented Jan 7, 2021

brian-brazil left a comment

codesome commented Jan 14, 2021

brian-brazil commented Jan 20, 2021

juliusv Apr 18, 2021

bboreham commented Dec 14, 2021

roidelapluie commented Dec 14, 2021

'@ <timestamp>' modifier #8121

'@ <timestamp>' modifier #8121

Conversation

codesome commented Oct 28, 2020 • edited

codesome commented Oct 28, 2020 • edited

roidelapluie commented Oct 28, 2020

codesome commented Oct 28, 2020

roidelapluie commented Oct 28, 2020 • edited

roidelapluie commented Oct 28, 2020

codesome commented Oct 28, 2020

codesome commented Oct 28, 2020

beorn7 commented Oct 28, 2020

beorn7 commented Oct 28, 2020

codesome commented Oct 28, 2020

brian-brazil commented Oct 28, 2020

codesome commented Oct 28, 2020

roidelapluie commented Oct 28, 2020

brian-brazil commented Oct 28, 2020

bwplotka commented Oct 28, 2020

beorn7 commented Oct 28, 2020

beorn7 commented Oct 28, 2020

brian-brazil commented Oct 28, 2020

RichiH commented Oct 29, 2020

brian-brazil commented Oct 29, 2020

bwplotka commented Oct 29, 2020

codesome commented Nov 19, 2020

codesome commented Nov 19, 2020

codesome commented Jan 4, 2021

brian-brazil left a comment

Choose a reason for hiding this comment

codesome commented Jan 7, 2021

brian-brazil left a comment

Choose a reason for hiding this comment

codesome commented Jan 14, 2021

brian-brazil commented Jan 20, 2021

juliusv Apr 18, 2021

Choose a reason for hiding this comment

bboreham commented Dec 14, 2021

roidelapluie commented Dec 14, 2021

codesome commented Oct 28, 2020 •

edited

codesome commented Oct 28, 2020 •

edited

roidelapluie commented Oct 28, 2020 •

edited