Skip to content

Conversation

@felixbarny
Copy link
Member

@felixbarny felixbarny commented Nov 26, 2025

Changes the PROMQL command from being a processing command that should be nested under the TS source command to be a standalone source command.

Example:

PROMQL k8s step 1m (sum(avg_over_time(network.cost[1m])))

@felixbarny felixbarny self-assigned this Nov 26, 2025
@felixbarny felixbarny added >non-issue :StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL labels Nov 26, 2025
@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v9.3.0 labels Nov 26, 2025
@felixbarny
Copy link
Member Author

@stratoula fyi about this grammar change for PromQL

@felixbarny felixbarny marked this pull request as ready for review November 27, 2025 08:52
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Comment on lines -16 to -21
// Simple unquoted identifier for parameter names and values
PROMQL_UNQUOTED_IDENTIFIER
: [a-z0-9][a-z0-9_]* // Starts with letter/digit, followed by letters/digits/underscores
| [_@][a-z0-9_]+ // OR starts with _/@ followed by at least one alphanumeric/underscore
;

Copy link
Member Author

@felixbarny felixbarny Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: I removed this in favor of UNQUOTED_SOURCE as the tokens would overlap otherwise. Alphanumeric tokens would always be interpreted as a PROMQL_UNQUOTED_IDENTIFIER, and never "fall though" to UNQUOTED_SOURCE.

This means a slightly different character set is now allowed for param key/values but that seems fine.

@dnhatn
Copy link
Member

dnhatn commented Nov 29, 2025

@felixbarny I tested with PROMQL hosts step 1m (sum(avg_over_time(cpu[1m]))) and failed with the error: line 1:22: missing {QUOTED_STRING, NAMED_OR_POSITIONAL_PARAM, QUOTED_IDENTIFIER, PROMQL_UNQUOTED_IDENTIFIER} at '('. I think we should update the grammar? Let me know if you need help.

@felixbarny
Copy link
Member Author

felixbarny commented Nov 29, 2025

Hm, that's strange. How did you test that?

When adding this test in PromqlParserTests, it succeeds for me:

    public void testNhat() {
        PromqlCommand promqlCommand = parse("PROMQL hosts step 1m (sum(avg_over_time(cpu[1m])))");
        assertThat(promqlCommand.step().value(), equalTo(Duration.ofMinutes(1)));
        List<UnresolvedRelation> unresolvedRelations = promqlCommand.collect(UnresolvedRelation.class);
        assertThat(unresolvedRelations, hasSize(1));
        assertThat(unresolvedRelations.getFirst().indexPattern().indexPattern(), equalTo("hosts"));
    }

There are also similar csv test cases in k8s-timeseries*.csv-spec that are working for me. I'll still need to sort out some issues with the bwc tests related to modifying the queries to include remote clusters.
Update: I finally got all tests to pass

@felixbarny
Copy link
Member Author

PROMQL_UNQUOTED_IDENTIFIER

Maybe you were using an old version of this branch? I’ve removed this token in favor of UNQUOTED_SOURCE as highlighted above: #138687 (review)

@dnhatn
Copy link
Member

dnhatn commented Nov 29, 2025

Yes, it works with the latest revision - thanks Felix. Just to confirm, do we want PromQL to be used only as a source command, or as both a source and a processing command?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue :StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL Team:StorageEngine v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants