promql string out from group_right () (...)

[tcolgate]

What did you do?

Take a promql query with a ... group_right () ...

(max(aws_ec2_spot_price_per_hour_dollars) BY (az, product, instance_type) / ON(az, product, instance_type) GROUP_RIGHT ()(max(drop_common_labels(max(aws_ec2_spot_request_bid_price_hourly_dollars{aws_tag_aws_autoscaling_groupname=~".+"}) WITHOUT (instance) / max(aws_ec2_spot_request_count) WITHOUT (instance))) BY (az, instance_type, product, aws_tag_aws_autoscaling_groupname, aws_tag_cluster))) > 1

parse that with the promql library, and print it out from there again.

(max(aws_ec2_spot_price_per_hour_dollars) BY (az, product, instance_type) / ON(az, product, instance_type) GROUP_RIGHT (max(drop_common_labels(max(aws_ec2_spot_request_bid_price_hourly_dollars{aws_tag_aws_autoscaling_groupname=~".+"}) WITHOUT (instance) / max(aws_ec2_spot_request_count) WITHOUT (instance))) BY (az, instance_type, product, aws_tag_aws_autoscaling_groupname, aws_tag_cluster))) > 1

The empty () is removed. The query is then invalid, as the subsequent expression is in parens, but is not a list

What did you expect to see?

promql output should be valid input.

What did you see instead? Under which circumstances?

promql produces invalid output

invalid output.

Environment

• System information:

  insert output of uname -srm here

• Prometheus version:

prom commit 2d203e7

[tcolgate]

I should mention that the parens around the max() aren't needed, so removing those fixes my running code. but I think the issue is still relevant.2

[grobie]

This seems to be another artifact of the lexer's early tokenizing behavior.
I don't think we should just change the printer, such query could also be
written by hand. Either we change the grammar or we fix the lexer and don't
assume all following parentheses are grouping labels lists.

For changing the grammar it'd be possible to make empty parentheses
mandatory in general or forbid any whitespace between a group modifier and
an optional label list.

We had similar issues with by/without clauses and fixed that by changing
the lexer.

[brian-brazil]

It's the printer that needs fixing here. It'd make the parser more complicated and harder to understand if we made this valid syntax.

[tcolgate]

I had a look at getting the parser to cope, and it doesn't look like doing the lookahead would be trivial. (lexing looks fine, but the parser would need a lookahead to disambiguate group_right (c) from group_right (c) (c).

[grobie]

Agreed @brian-brazil, @tcolgate and I discussed this on IRC already. It's not easy or practical to change the lexer behavior, the lookahead would be expensive and complicated. So let's change, or rather amend, the PromQL grammar specification and add that empty parentheses after GROUP_* modifiers are only optional if the following expression is not wrapped in parentheses. As this doesn't change the current behavior and only documents it, this should be fine to do in v1.x.

[tcolgate]

@grobie do you want a doc update in this PR? If so, where should i be looking to make such an update?

[grobie]

The doc update would need to happen in prometheus/docs. Although, while https://prometheus.io/docs/querying/operators/#vector-matching (in many-to-many) would be the most related place, I fear it would clutter the docs too much. Let's go without the docs change for now.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.