-
Notifications
You must be signed in to change notification settings - Fork 820
Enable @ modifier with result cache handling #3744
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
Conversation
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
234da3a
to
23b3f25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Should we keep |
It is disabled by default in Prometheus because it has no in-built caching and so that users of PromQL can enable when they have fixed the caching to use this modifier. Since we handle the caching inside Cortex itself, I don't think we need to have a flag for that and just enable it by default. |
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
After some offline discussions, I have added a flag for @ modifier. |
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! I left a nit post-merge comment.
} | ||
expr, err := parser.ParseExpr(query) | ||
if err != nil { | ||
// We are being pessimistic in such cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A warning log may be useful here. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will open another PR for the warning.
What this PR does:
Enables the newly added @ modifier in PromQL with cache handling for it.
If the time in @ is > query end time, then we don't cache the result. If @ time is <= end, then we cache, since even if the start and end were actually in the future we would end up caching the result anyway.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]