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

Alerting: Fix evaluation interval validation #56115

Merged
merged 7 commits into from Oct 7, 2022

Conversation

konrad147
Copy link
Contributor

@konrad147 konrad147 commented Sep 30, 2022

What this PR does / why we need it:
This PR fixes evaluation interval validation

Which issue(s) this PR fixes:
Fixes #55357, #55649

Special notes for your reviewer:
WIP: Prometheus validation regex doesn't play well in JS

@grafanabot
Copy link
Contributor

@gillesdemey
Copy link
Member

gillesdemey commented Sep 30, 2022

I had some time to play around with a parser that I think could work to parse this type of duration, it's different from the traditional time.ParseDuration because

  1. It does not support floating point numbers
  2. No negative durations supported
  3. Prometheus supports d, w, y and the default Go parser does not

https://gist.github.com/gillesdemey/6c251dc4f810d8e60ec2958eb9aa7225

@gillesdemey gillesdemey marked this pull request as ready for review October 4, 2022 12:04
@gillesdemey gillesdemey requested a review from a team as a code owner October 4, 2022 12:04
@gillesdemey
Copy link
Member

I've appended some changes that should correctly validate:

  • If interval and for are valid "prometheus duration" strings (such as 1d30m10s)
  • Compare interval and for with eachother
  • Compare interval with the global min_interval

Copy link
Member

@gillesdemey gillesdemey left a comment

Choose a reason for hiding this comment

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

LGTM but my review is slightly biased :)

@grafanabot
Copy link
Contributor

@gillesdemey gillesdemey added add to changelog backport v8.5.x Mark PR for automatic backport to v8.5.x backport v9.2.x Mark PR for automatic backport to v9.2.x labels Oct 4, 2022
@gillesdemey gillesdemey added this to the 9.1.8 milestone Oct 4, 2022
@konrad147 konrad147 added backport v9.0.x backport v9.1.x Bot will automatically open backport PR labels Oct 6, 2022
@gillesdemey gillesdemey requested a review from a team October 6, 2022 09:00
// 0 is a special value meaning for equals evaluation interval
if (millisFor === 0) {
if (Number(value) === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid it's not enough. 0s is also valid for value. Actually, it's what we get from the API when the user set for=0

Copy link
Member

Choose a reason for hiding this comment

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

Let's make sure we also support 0 as that is a format that Prometheus accepts

Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉
I added a non-blocking comment 👀

const diff = durationToMilliseconds(duration);
if (diff < MIN_TIME_RANGE_STEP_S * 1000) {
try {
const duration = parsePrometheusDuration(value);
Copy link
Member

Choose a reason for hiding this comment

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

I would name this const withmillisEvery or millisEvaluateEvery to be consistent with the For, and I think it would be more clear

return durationToMilliseconds(parseDuration(duration));
}

export function isValidPrometheusDuration(duration: string): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

👏

@@ -15,7 +15,10 @@ interface RuleConfigStatusProps {
export function RuleConfigStatus({ rule }: RuleConfigStatusProps) {
const styles = useStyles2(getStyles);

const { exceedsLimit } = checkEvaluationIntervalGlobalLimit(rule.group.interval);
const { exceedsLimit } = useMemo(
Copy link
Member

Choose a reason for hiding this comment

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

💡

@grafanabot
Copy link
Contributor

// a strict golang time.ParseDuration compatible format
if (!alertGroupEvaluateEvery || !isValidPrometheusDuration(alertGroupEvaluateEvery)) {
// config.unifiedAlerting.minInterval should be Prometheus-compatible duration
// However, Go's gtime library has issues with parsing y,w,d
Copy link
Member

Choose a reason for hiding this comment

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

Technically it's our own grafana-sdk-go's gtime package :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, so why can't we fix it? 😄

Copy link
Member

Choose a reason for hiding this comment

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

I think that might introduce some unwanted side-effects :D

@gillesdemey gillesdemey merged commit d945091 into main Oct 7, 2022
@gillesdemey gillesdemey deleted the alerting/fix-evaluation-interval-validation branch October 7, 2022 13:01
@grafanabot
Copy link
Contributor

The backport to v8.5.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-56115-to-v8.5.x origin/v8.5.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x d945091ff14a5d66c6b34fe75e72ef5befa19be8
# Push it to GitHub
git push --set-upstream origin backport-56115-to-v8.5.x
git switch main
# Remove the local backport branch
git branch -D backport-56115-to-v8.5.x

Then, create a pull request where the base branch is v8.5.x and the compare/head branch is backport-56115-to-v8.5.x.

@grafanabot
Copy link
Contributor

The backport to v9.0.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-56115-to-v9.0.x origin/v9.0.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x d945091ff14a5d66c6b34fe75e72ef5befa19be8
# Push it to GitHub
git push --set-upstream origin backport-56115-to-v9.0.x
git switch main
# Remove the local backport branch
git branch -D backport-56115-to-v9.0.x

Then, create a pull request where the base branch is v9.0.x and the compare/head branch is backport-56115-to-v9.0.x.

@grafanabot
Copy link
Contributor

The backport to v9.1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-56115-to-v9.1.x origin/v9.1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x d945091ff14a5d66c6b34fe75e72ef5befa19be8
# Push it to GitHub
git push --set-upstream origin backport-56115-to-v9.1.x
git switch main
# Remove the local backport branch
git branch -D backport-56115-to-v9.1.x

Then, create a pull request where the base branch is v9.1.x and the compare/head branch is backport-56115-to-v9.1.x.

grafanabot pushed a commit that referenced this pull request Oct 7, 2022
Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
(cherry picked from commit d945091)
konrad147 added a commit that referenced this pull request Oct 10, 2022
Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
(cherry picked from commit d945091)
konrad147 added a commit that referenced this pull request Oct 10, 2022
Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
(cherry picked from commit d945091)
konrad147 added a commit that referenced this pull request Oct 10, 2022
Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
konrad147 added a commit that referenced this pull request Oct 10, 2022
Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend backport v8.5.x Mark PR for automatic backport to v8.5.x backport v9.0.x backport v9.1.x Bot will automatically open backport PR backport v9.2.x Mark PR for automatic backport to v9.2.x enterprise-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI Bug, Alert Creation: "Evaluate every:" with non number input blocks the whole page
4 participants