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

Add experimental promQL funcs #133

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

SungJin1212
Copy link
Contributor

This PR adds experimental promQL funcs.

opts.go Outdated
enableOffset bool
enableAtModifier bool
enableVectorMatching bool
enableExperimentalPromQL bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call it enableExperimentalPromQLFunctions? Same as https://prometheus.io/docs/prometheus/latest/feature_flags/#experimental-promql-functions.

It is not just functions but I think it is more accurate to mention that

walk.go Outdated
@@ -83,6 +83,10 @@ func (s *PromQLSmith) walkAggregateParam(op parser.ItemType) parser.Expr {
return s.Walk(parser.ValueTypeScalar)
case parser.COUNT_VALUES:
return &parser.StringLiteral{Val: "value"}
case parser.LIMITK, parser.LIMIT_RATIO:
if s.enableExperimentalPromQL {
Copy link
Contributor

Choose a reason for hiding this comment

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

So we pick an aggregation type randomly from enabled aggregate operators. But for now I think default aggregate operators don't include limitk and limit_ratio.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might go wrong if users enable the feature flag but don't add the operators to enabled aggregate operators

expr.Args = append(expr.Args, &parser.StringLiteral{Val: name})
cnt++
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 2 issues here:

  1. there is no randomization since s.labelNames are always the same order
  2. For refernece you can check walkLabelJoin. We try to get the underlying vector selector and infer what labels it might have and pick labels there. This increases the chance where the labels used in the query actually makes sense for the series

@SungJin1212 SungJin1212 force-pushed the Support-experimental-promQLs branch from 782da6a to 228f5f4 Compare November 6, 2024 01:47
@SungJin1212
Copy link
Contributor Author

@yeya24
I have updated the PR.

  • If enableExperimentalPromQLFunctions is true, experimental promQL functions and aggregation are appended.
  • Fix walkSortByLabel by referring to the walkLabelJoin
  • Remove if s.enableExperimentalPromQL checking in thewalk.go

@SungJin1212 SungJin1212 requested a review from yeya24 November 7, 2024 01:03
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Awesome work!

@yeya24 yeya24 merged commit 0722cc7 into cortexproject:main Nov 13, 2024
3 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.

2 participants