Making durations and number literals the same

[darshanime]

Design doc: https://docs.google.com/document/d/1LaZfknXuuRWGtQSbULoMtclQhuLUMrdwg15wMvoBvCQ/edit#

Signed-off-by: darshanime deathbullet@gmail.com

[roidelapluie]

Thank you. I expect that Beorn will want to look at this one, but you can expect some delays due to the summer period.

[LeviHarrison]

According to your design doc, the following should work: rate(prometheus_http_requests_total[0.25]), but instead I get this error: Error executing query: 1:37: parse error: strconv.Atoi: parsing "0.25": invalid syntax. Otherwise seems good!

[juliusv]

I tried predict_linear(foo[5h], 1h), but I get:

parse error: trailing commas not allowed in function call args

I also left some comments on the design doc on questions I had.

[juliusv]

One thing I'm interested in is how this will affect the printing of PromQL expressions once they have been parsed.

It would be sad if the input was:

predict_linear(foo[1h], 4h)

...and we could only serialize it out again as:

predict_linear(foo[1h], 14400)

Not sure if this is the plan already, but would it make sense to store the original tokens in the parsed AST somehow so that we can render them out again in a way that makes most sense to the user?

[beorn7]

One thing I'm interested in is how this will affect the printing of PromQL expressions once they have been parsed.
[…]

That's a good point, but perhaps we can solve that later, once the main functionality is implemented.

I think the comments above( #9138 (comment) , #9138 (comment) ) are real issues that need to be fixed.

[darshanime]

thank you, will address 👍
marking as draft to avoid notifications in the meantime...

[beorn7]

This might come from my ignorance of parser internals, but wouldn't it be easier to just get rid of the DURATION type altogether and replace it with NUMBER everywhere? And then parse each number as a duration.

[valyala]

FYI, VictoriaMetrics supports using durations instead of numeric values and vice versa in MetricsQL. See, for example, how does predict_linear(sum(rate(vm_http_requests_total[300])) offset 3600, 3h) work in VictoriaMetrics playground. It would be great if Prometheus could support this too.

[beorn7]

We discussed this during our bug scrub. I think my point above is still valid and hasn't been addressed. @darshanime are you still up to working on this?

This general topic comes up quite often, so we should finish this up one way or another.

[darshanime]

The idea here is to allow it everywhere

I skipped it originally because foo @ 3s did not "make sense" and I thought we need not make holes in the type system if we can help it. Made the changes now, agree with your point about being consistent everywhere.

Do you know of any other cases where a number cannot be replaced by a duration?

There should be no such place now, and we should be able to use duration and number interchangeably...

the documentation needs a corresponding update.

Does this mean making a change in CHANGELOG.md or someplace else too?

we need to update the frontend part as well so that the frontend doesn't put squiggly lines underneath correct syntax...

Can we do this as part of a separate PR?

[beorn7]

Do you know of any other cases where a number cannot be replaced by a duration?

There should be no such place now, and we should be able to use duration and number interchangeably...

Excellent. Thank you very much. I'll have a detailed look in the coming week, but it seems we are now where we want to be.

the documentation needs a corresponding update.

Does this mean making a change in CHANGELOG.md or someplace else too?

The CHANGELOG.md update is only created during the release, but there is "real" documentation for PromQL. Once this change is declared stable, we have to go through the whole body of PromQL documentation and make sure that numbers and durations are the same everywhere. For now, this will be an experimental change (see Julius's comment), so we only have to extend the definition of float literals and durations by something like "As of version 2.52, durations can also be represented by float values, implying the number of seconds. This is an experimental feature and might still change." or "As of version 2.52, float values can also be represented using the syntax of durations, where the duration literal is converted into a float value corresponding to the number of seconds the duration literal represents." (I guess more wordsmithing should apply here, just trying to point in the right direction. Also, an example might be good.) The file to change is https://github.com/prometheus/prometheus/blob/main/docs/querying/basics.md .

we need to update the frontend part as well so that the frontend doesn't put squiggly lines underneath correct syntax...

Can we do this as part of a separate PR?

Technically yes, but we should definitely minimize the duration of having inconsistent version of the UI code and the PromQL implementation in the backend in the main branch.

If you plan to work on the UI code yourself, it would indeed be best to add commits to this PR (but having UI code and backend code in different commits would be desired).

If you cannot work on the UI code, we should ask our React experts for help. If one of them works on this, they could create a separate PR, but we should merge both PRs at about the same time.

So the question is: Do you plan to work on the UI code yourself?

[darshanime]

we should ask our React experts for help

Yes please, that would be great!

I will update the documentation, tfr!

[Maniktherana]

Another point is that the frontend code hasn't been updated. So while rate(process_cpu_seconds_total[300]) works, the UI shows squiggly lines and red numbers because it sees it as a syntax error:

Okay so to address this, changes need to be made to @prometheus-io/lezer-promql and @prometheus-io/codemirror-promql as pointed out by @Nexucis as in this conversation. There's no React specific code that needs to be changed :)

Now while I have a broad idea of what to do, I'm unfamiliar with yacc and .grammar and they're a blocker for me atm. But so far the steps I can see are:

• Edit promql.grammar to accomodate changes made in generated_parser.y

• In the grammar file we'd have to replace Duration with a NumberDurationLiteral in the following places:

OffsetExpr {
  expr Offset Sub? Duration
}

MatrixSelector {
  expr "[" Duration "]"
}

SubqueryExpr {
  expr "[" Duration ":" ("" | Duration) "]"
}

• We may also have to update NumberLiteral and number declarations accordingly, this is where I'm most unsure:

NumberLiteral  {
("-"|"+")?~signed (number | inf | nan)
}

number {
     (std.digit+ ("." std.digit*)? | "." std.digit+) (("e" | "E") ("+" | "-")? std.digit+)? |
     "0x" (std.digit | $[a-fA-F])+
 }

Duration {
    ... 
 }

• Update tests in expression.txt accordingly

• Replace NumberLiteral with NumberDurationLiteral in @codemirror-promql to get autocomplete and parser working

Again, updating the grammar file itself is a hurdle for me and its where I'd need help. I can then work on adding tests and updating @codemirror-promql. Let me know if this is somewhat on the right path

[beorn7]

The final outcome has to be that NumberDurationLiteral is used instead of Duration or NumberLiteral everywhere. Maybe those two can disappear completely, and NumberDurationLiteral is just defined to include both former notions?

@Nexucis and @darshanime can probably help better here than I.

[juliusv]

Yeah, @Nexucis is the most knowledgeable when it comes to the grammar-related functionality, especially the completion and linting.

But what just occurred to me: For autocompletion, we will not want to treat numbers and durations the same in all places. We currently autocomplete the unit suffix for durations, but it would not make sense to do that in places where only a normal number would make sense. So I guess there'd need to be some higher-level logic that checks the context in which the NumberDurationLiteral is being typed? Like if it's the second parameter of the predict_linear() function, autocomplete it as a duration, but not in the general case?

[beorn7]

Not sure if it is worth the effort to distinguish those cases.

histogram_quantile(0.99, rate(rpc_latency_seconds[5m])) > 500ms is perfectly reasonable, but to predict that, you needed to encode a lot of query analysis logic.

[beorn7]

On the other hand, it's probably fine to only do the autocomplete where there is a duration for sure. You can still type things that the autocomplete didn't suggest, so no harm done if the autocomplete is a bit too restrictive.

[beorn7]

@Maniktherana I assume you still need some answers for the parser part. @Nexucis could you help out @Maniktherana ? He spelled out his plan above and it appears he would like to know if that's the right approach.

@darshanime as discussed, we still need an update to the docs in this PR.

[Nexucis]

Hello,

Sorry for the time it took me to reply.

• Regarding the grammar I think we can simply do that:

NumberDurationLiteral {
   Duration | NumberLiteral
}

And then replace Duration & NumberLiteral by NumberDurationLiteral everywhere it is called.

I don't think we need to update NumberLiteral and number

• The hardest work would be on the autocompletion I think. As pointed by @juliusv it doesn't make sense to autocomplete duration letter everywhere. For the moment I would advise to keep the autocompletion of the duration letter where it is today (i.e. in OffsetExpr and in SubqueryExpr)

It's possible we won't be able to autocomplete duration everywhere it is today. Let see.

• On the linter, I have written it in order to follow as close as possible the function (checkAST)[https://github.com/prometheus/prometheus/blob/main/promql/parser/parse.go#L629]. Would be cool to keep this sort of synchronisation. Just to be sure the frontend is replicating what the backend is doing.

[Nexucis]

So I guess there'd need to be some higher-level logic that checks the context in which the NumberDurationLiteral is being typed? Like if it's the second parameter of the predict_linear() function, autocomplete it as a duration, but not in the general case?

Probably we can do that in a second PR. It's indeed possible to have this kind of logic as we already do that for some use cases. I would advise to just do it later to reduce the number of changes in a single PR and also because doing this kind of logic can break the whole code if you don't do it conscientiously

[Nexucis]

@Maniktherana let me know if you need more input from me

[Maniktherana]

@Maniktherana let me know if you need more input from me

I'm tracking, I'm currently out of town however without my laptop. Will be back in a couple of days.

[beorn7]

Thank you very much @Maniktherana and @Nexucis . Looking forward to the PR for the UI.

@darshanime thank for your patience with this. I'm currently looking at the docs addition in this PR.

[beorn7]

Some wordsmithing for the comments. An also an actual correction because 0.01 != 1ms.

[darshanime]

@beorn7, addressed. tfr!

[beorn7]

Approved! Thank you very much. However, let's hold back merging for now until @Maniktherana is more or less done with the UI updates, so that we don't have an inconsistent state in main.

[beorn7]

Here an update after a bit back and forth:

The frontend changes we need here are mostly in the grammar part, so it's not really React specific, and @Maniktherana , who wanted to give it a spin, feels way outside of his comfort zone. We tried to get @Nexucis work on it, but he won't have time anytime soon.

Maybe @juliusv can help out here?

On the other hand, maybe the changes aren't that different from the one in this PR, and @darshanime might be able to do it after all, given that @Nexucis has already given some guidance above.

We could also try to find some random contributor knowledgeable with the lezer-promql stuff.

The last resort is that I do it myself. As this is not exactly my area of expertise, I'll need some time to execute it, and I have also so many things queued up that I have to get done before even getting to this.

[Maniktherana]

We tried to get @Nexucis work on it, but he won't have time anytime soon.

About this: @Nexucis and I both tried out the suggestions and that doesn't seem to be the solution. The output I was getting when trying out the following:

Regarding the grammar I think we can simply do that:

NumberDurationLiteral {
   Duration | NumberLiteral
}

The output I was getting is:

 npm run build

> @prometheus-io/lezer-promql@0.52.0-rc.1 build
> bash ./build.sh

+ npx lezer-generator src/promql.grammar -o src/parser
Overlapping tokens Duration and number used in same context (example: "0" vs "00d")
After: expr "[" (src/promql.grammar 1:1)
npm ERR! Lifecycle script `build` failed with error: 
npm ERR! Error: command failed 
npm ERR!   in workspace: @prometheus-io/lezer-promql@0.52.0-rc.1 
npm ERR!   at location: /Users/manikrana/vscode/prometheus/web/ui/module/lezer-promql