Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add or operator for metric expression #84

Merged
merged 9 commits into from
May 20, 2024

Conversation

groobyming
Copy link
Contributor

@groobyming groobyming commented May 14, 2024

MetricQL is an enhanced extension of PromQL, And is backward-compatible with PromQL.

Currently, MetricQL already supports using the "or" operator to connect label filters as follows:
For example, the following query selects time series with {job="app1",env="prod"} or {job="app2",env="dev"} labels:

{job="app1",env="prod" or job="app2",env="dev"}

A more detailed description of this feature can be found here MetricsQL filtering-by-multiple-or-filters

This PR is to support filtering by multiple “or” filters

@groobyming groobyming force-pushed the support_or_metric_filter branch from 6d2a28e to 76bc3d1 Compare May 14, 2024 04:13
@groobyming groobyming changed the title add |-operator for metric expression(#83) feat: add |-operator for metric expression May 14, 2024
@groobyming
Copy link
Contributor Author

@yuanbohan hi,yuanbohan.if you have time, please help to review it

@yuanbohan
Copy link
Contributor

May I ask a question:

Is this PR the beginning of total implementation of MetricsQL, or only to support OR in label matcher?

@groobyming
Copy link
Contributor Author

May I ask a question:

Is this PR the beginning of total implementation of MetricsQL, or only to support OR in label matcher?

@yuanbohan just only to support OR in label matcher

src/parser/parse.rs Outdated Show resolved Hide resolved
src/label/matcher.rs Outdated Show resolved Hide resolved
@yuanbohan
Copy link
Contributor

BTW, this PR is to support MetricsQL filtering-by-multiple-or-filters

@yuanbohan yuanbohan requested a review from waynexia May 15, 2024 02:37
src/parser/parse.rs Outdated Show resolved Hide resolved
@evenyag
Copy link
Contributor

evenyag commented May 15, 2024

@groobyming Could you please update the PR description to summarize what the PR does? There is an example: #76

@groobyming
Copy link
Contributor Author

@groobyming Could you please update the PR description to summarize what the PR does? There is an example: #76

OK

@yuanbohan yuanbohan requested a review from evenyag May 15, 2024 08:22
@groobyming groobyming force-pushed the support_or_metric_filter branch from cb6644f to 1110f9b Compare May 16, 2024 07:16
@groobyming
Copy link
Contributor Author

@yuanbohan @evenyag I extended the Matchers structure to support "or filters", if you have time, please help review new commit😀

src/label/matcher.rs Show resolved Hide resolved
src/label/matcher.rs Outdated Show resolved Hide resolved
src/label/matcher.rs Outdated Show resolved Hide resolved
src/label/matcher.rs Outdated Show resolved Hide resolved
@groobyming
Copy link
Contributor Author

@yuanbohan @evenyag
I found that when processing ordinary mather and or_matcher, it is necessary to record whether the last processed matcher or or_matcher was, otherwise there is no way to decide the next action. Refer to the following example:
a{label1="1", label2="2" or label3="3" or label4="4"}
a{label1="1" or label2="2", label3="3" or label4="4"}

When processing the label4="4" node, if there is no previous processing status, how do you know whether to append to the original {label2="2" or label3="3"}, or to create a new or_matcher and add label1=" 1" and label4="4"?

@groobyming
Copy link
Contributor Author

groobyming commented May 16, 2024

@yuanbohan @evenyag I found that when processing ordinary mather and or_matcher, it is necessary to record whether the last processed matcher or or_matcher was, otherwise there is no way to decide the next action. Refer to the following example: a{label1="1", label2="2" or label3="3" or label4="4"} a{label1="1" or label2="2", label3="3" or label4="4"}

When processing the label4="4" node, if there is no previous processing status, how do you know whether to append to the original {label2="2" or label3="3"}, or to create a new or_matcher and add label1=" 1" and label4="4"?

In general, there is still a lot of content and state to maintain when adding changes to the matchers structure.

like this

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Matchers {
    pub matchers: Vec<Matcher>,
    pub or_matchers: Vec<Vec<Matcher>>,
    pub last_filter_action: ActionType
}

#[derive(Eq, PartialEq)]
pub enum ActionType {
    AND,
    OR,
}

@groobyming
Copy link
Contributor Author

@yuanbohan @evenyag Add last_filter_action to mark the last append type, if you have time, please help review new commit😀

@groobyming groobyming force-pushed the support_or_metric_filter branch from 7ffea98 to 67e0e5a Compare May 16, 2024 12:40
@evenyag
Copy link
Contributor

evenyag commented May 16, 2024

@yuanbohan @evenyag I found that when processing ordinary mather and or_matcher, it is necessary to record whether the last processed matcher or or_matcher was, otherwise there is no way to decide the next action. Refer to the following example: a{label1="1", label2="2" or label3="3" or label4="4"} a{label1="1" or label2="2", label3="3" or label4="4"}

When processing the label4="4" node, if there is no previous processing status, how do you know whether to append to the original {label2="2" or label3="3"}, or to create a new or_matcher and add label1=" 1" and label4="4"?

I guess that we have inconsistent understandings of the "or filter" in MetricsQL.

According to the document

MetricsQL supports selecting time series, which match at least one of multiple “or” filters. Such filters must be delimited by or inside curly braces.
For example, the following query selects time series with {job="app1",env="prod"} or {job="app2",env="dev"} labels:

{job="app1",env="prod" or job="app2",env="dev"}

The number of or groups can be arbitrary. The number of ,-delimited label filters per each or group can be arbitrary. Per-group filters are applied with and operation, e.g. they select series simultaneously matching all the filters in the group.

I think or is the delimiter for filter groups.

a{label1="1", label2="2" or label3="3" or label4="4"} contains three groups:

{label1="1", label2="2"}
or
{label3="3"}
or
{label4="4"}

a{label1="1" or label2="2", label3="3" or label4="4"} indicates the following groups:

{label1="1"}
or
{label2="2", label3="3"}
or
{label4="4"}

The previous matchers group always ends with or. It should be clear and we don't need to maintain the process status.

@groobyming
Copy link
Contributor Author

groobyming commented May 16, 2024

@yuanbohan @evenyag I found that when processing ordinary mather and or_matcher, it is necessary to record whether the last processed matcher or or_matcher was, otherwise there is no way to decide the next action. Refer to the following example: a{label1="1", label2="2" or label3="3" or label4="4"} a{label1="1" or label2="2", label3="3" or label4="4"}
When processing the label4="4" node, if there is no previous processing status, how do you know whether to append to the original {label2="2" or label3="3"}, or to create a new or_matcher and add label1=" 1" and label4="4"?

I guess that we have inconsistent understandings of the "or filter" in MetricsQL.

According to the document

MetricsQL supports selecting time series, which match at least one of multiple “or” filters. Such filters must be delimited by or inside curly braces.
For example, the following query selects time series with {job="app1",env="prod"} or {job="app2",env="dev"} labels:

{job="app1",env="prod" or job="app2",env="dev"}

The number of or groups can be arbitrary. The number of ,-delimited label filters per each or group can be arbitrary. Per-group filters are applied with and operation, e.g. they select series simultaneously matching all the filters in the group.

I think or is the delimiter for filter groups.

a{label1="1", label2="2" or label3="3" or label4="4"} contains three groups:

{label1="1", label2="2"}
or
{label3="3"}
or
{label4="4"}

a{label1="1" or label2="2", label3="3" or label4="4"} indicates the following groups:

{label1="1"}
or
{label2="2", label3="3"}
or
{label4="4"}

The previous matchers group always ends with or. It should be clear and we don't need to maintain the process status.

@evenyag Indeed, I misunderstood the usage of OR. I will try to fix this problem later, Thanks 👍

@groobyming
Copy link
Contributor Author

@yuanbohan @evenyag Make some changes to fix the problem according to the suggestion 😀

src/parser/parse.rs Outdated Show resolved Hide resolved
@evenyag evenyag changed the title feat: add |-operator for metric expression feat: add or operator for metric expression May 17, 2024
@evenyag evenyag changed the title feat: add or operator for metric expression feat: add or operator for metric expression May 17, 2024
evenyag
evenyag previously approved these changes May 17, 2024
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

LGTM

src/parser/parse.rs Outdated Show resolved Hide resolved
src/parser/lex.rs Outdated Show resolved Hide resolved
@yuanbohan
Copy link
Contributor

rebase the main branch to fix the codecov failure please

@groobyming
Copy link
Contributor Author

rebase the main branch to fix the codecov failure please

Done

Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.99%. Comparing base (5bc0088) to head (d76bbaa).

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
+ Coverage   98.96%   98.99%   +0.02%     
==========================================
  Files          14       14              
  Lines        6098     6272     +174     
==========================================
+ Hits         6035     6209     +174     
  Misses         63       63              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

evenyag
evenyag previously approved these changes May 20, 2024
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

Please fix the clippy warning

@yuanbohan
Copy link
Contributor

Please fix the clippy warning

@groobyming

@groobyming
Copy link
Contributor Author

Please fix the clippy warning

@evenyag Hi, evenyag. What version of rustc are we using, my local version is "rustc 1.80.0-nightly" and when running the "cargo Clippy" command no warnings appear.
image
image

@yuanbohan
Copy link
Contributor

Please fix the clippy warning

@evenyag Hi, evenyag. What version of rustc are we using, my local version is "rustc 1.80.0-nightly" and when running the "cargo Clippy" command no warnings appear. image image

You can find the failure detail from the action: https://github.com/GreptimeTeam/promql-parser/actions/runs/9152892165/job/25161349969 , and the command is cargo clippy --all-targets -- -D warnings

@groobyming
Copy link
Contributor Author

Please fix the clippy warning

@evenyag Hi, evenyag. What version of rustc are we using, my local version is "rustc 1.80.0-nightly" and when running the "cargo Clippy" command no warnings appear. image image

You can find the failure detail from the action: https://github.com/GreptimeTeam/promql-parser/actions/runs/9152892165/job/25161349969 , and the command is cargo clippy --all-targets -- -D warnings

Ok

@groobyming
Copy link
Contributor Author

Please fix the clippy warning

@groobyming

Please fix the clippy warning

@groobyming

Please fix the clippy warning

@evenyag Hi, evenyag. What version of rustc are we using, my local version is "rustc 1.80.0-nightly" and when running the "cargo Clippy" command no warnings appear. image image

You can find the failure detail from the action: https://github.com/GreptimeTeam/promql-parser/actions/runs/9152892165/job/25161349969 , and the command is cargo clippy --all-targets -- -D warnings

Ok

fixed

@yuanbohan yuanbohan merged commit b11faca into GreptimeTeam:main May 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants