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 Graphite subqueries #80744

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Conversation

gillesdemey
Copy link
Member

What is this feature?

This PR contains 2 notable changes to make Graphite subqueries work within alerting.

  1. Alerting was passing an incorrect array of queries, TypeScript was not complaining here because the interface of AlertQuery is compatible with DataQuery (they both have refId: string) even though these are very different data structures.
  2. The Alerting editor will track the state of its queries by listening to the onChange handler and writing the updates to its own state with a reducer. This PR adds the targetFull property to it so it can be recorded in the backend and used for rule evaluation.

Special notes for your reviewer:

I wasn't quite sure about the changes made to the Graphite editor and currently am a bit perplexed about why this does work for the panel editor.

I'd love to get some feedback if I'm on the wrong track here and if we need a different approach.

Copy link
Contributor

@bohandley bohandley left a comment

Choose a reason for hiding this comment

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

This looks like a sensible approach to me! 🚀

@@ -136,6 +136,9 @@ export const QueryWrapper = ({
}

const showVizualisation = data.state !== LoadingState.NotStarted;
// ⚠️ the query editors want the entire array of queries passed as "DataQuery" NOT "AlertQuery"
// TypeScript isn't complaining here because the interfaces just happen to be compatible
const editorQueries = cloneDeep(queries.map((query) => query.model));
Copy link
Member Author

@gillesdemey gillesdemey Jan 18, 2024

Choose a reason for hiding this comment

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

Leaving this here for posterity; some editors mutate the queries prop they receive and those are managed by an immutable state and a reducer on the Alerting side – so we clone them before passing them forward.

Copy link
Contributor

@konrad147 konrad147 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gillesdemey gillesdemey added this to the 10.2.x milestone Jan 18, 2024
@gillesdemey gillesdemey merged commit 8a4bd85 into main Jan 18, 2024
30 checks passed
@gillesdemey gillesdemey deleted the alerting/graphite-subqueries-fix branch January 18, 2024 14:15

This comment was marked as resolved.

This comment was marked as resolved.

@grafana-delivery-bot grafana-delivery-bot bot modified the milestones: 10.2.x, 10.4.x Jan 18, 2024
@gillesdemey gillesdemey modified the milestones: 10.4.x, 10.2.x Jan 18, 2024
grafana-delivery-bot bot pushed a commit that referenced this pull request Jan 18, 2024
gillesdemey added a commit that referenced this pull request Jan 18, 2024
Alerting: Fix Graphite subqueries (#80744)

(cherry picked from commit 8a4bd85)

Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
@gillesdemey gillesdemey added the backport v9.4.x Mark PR for automatic backport to v9.4.x label Jan 22, 2024
Copy link
Contributor

The backport to v9.4.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-80744-to-v9.4.x origin/v9.4.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 8a4bd85efdd4101c481244b62d70e03e315d820c

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-80744-to-v9.4.x
# Create the PR body template
PR_BODY=$(gh pr view 80744 --json body --template 'Backport 8a4bd85efdd4101c481244b62d70e03e315d820c from #80744{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "[v9.4.x] Alerting: Fix Graphite subqueries" --body-file - --label "type/bug" --label "datasource/Graphite" --label "area/alerting" --label "area/frontend" --label "add to changelog" --label "backport" --base v9.4.x --milestone 9.4.x --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-80744-to-v9.4.x

# Create a pull request where the `base` branch is `v9.4.x` and the `compare`/`head` branch is `backport-80744-to-v9.4.x`.

# Remove the local backport branch
git switch main
git branch -D backport-80744-to-v9.4.x

@grafana-delivery-bot grafana-delivery-bot bot added the backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. label Jan 22, 2024
gillesdemey added a commit that referenced this pull request Jan 22, 2024
gillesdemey added a commit that referenced this pull request Jan 22, 2024
Alerting: Fix Graphite subqueries (#80744)

(cherry picked from commit 8a4bd85)
@zserge zserge modified the milestones: 10.2.x, 10.2.4 Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/alerting Grafana Alerting area/frontend backport v9.4.x Mark PR for automatic backport to v9.4.x backport v10.2.x backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. datasource/Graphite type/bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants