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

fix: AlertRulev9 JSON schema is not correct #544

Merged
merged 2 commits into from May 18, 2023

Conversation

OscarVanL
Copy link
Contributor

There are a couple of mistakes in the Grafana 9 Alerts-As-Code AlertRulev9 JSON output.

Firstly, the absence of grafana_alert means that uploading the output JSON fails with a validation error: {"message":"unexpected backend type (grafana) for payload type (lotex)","traceID":""}, which specifically is this error in Grafana.

Secondly, the fields noDataState and execErrState were renamed to no_data_state and exec_err_state in v9, meaning these settings were not applied correctly.

With these changes my alerts upload successfully and appear to be correct, but if I spot further mistakes I'll make follow-up PRs.

@OscarVanL OscarVanL changed the title fix: AlertRulev9 JSON schems is not correct fix: AlertRulev9 JSON schema is not correct Nov 11, 2022
@JamesGibo
Copy link
Collaborator

Thanks for the PR, I will give this a test with my alert rules as I have not seen this issue, but then I am not uploading via the API.

@OscarVanL
Copy link
Contributor Author

Interesting, how are you uploading the alerts? By provisioning?

@JamesGibo
Copy link
Collaborator

Interesting, how are you uploading the alerts? By provisioning?

Yeah I use the file based provisioning.

@OscarVanL
Copy link
Contributor Author

This looks to be the format that Grafana uses internally, if you open Inspect Element and go to the Alerting tab of Grafana, then search for the request to /api/ruler/grafana/api/v1/rules?subtype=cortex, and look at the response body, you can see the JSON structure. For me (Grafana 9.2.3), it matches the structure that this PR uses.

Out of curiosity, if you find one of the alerts you uploaded using file-based provisioning inside this response, what structure does it use?

@hugohaa
Copy link

hugohaa commented Dec 19, 2022

Just a FYI I also tested this PR in our project and it's working perfectly. Was having a hard time figuring out what was happening. Thanks!

@OscarVanL
Copy link
Contributor Author

Thanks @hugohaa! Is there any possibility I could get a second look at this PR @JamesGibo?

@ran-ka
Copy link
Contributor

ran-ka commented Dec 27, 2022

We are also using the API to upload our alerts, and had to merge this PR for it to work. Thanks @OscarVanL

@KacperLegowski
Copy link

Are there plans to merge this PR? :)

@JDuskey
Copy link

JDuskey commented Mar 6, 2023

This looks good, and can confirm it fixes errors when pushing alerts to the unified alerting API.

Copy link
Collaborator

@JamesGibo JamesGibo left a comment

Choose a reason for hiding this comment

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

Looks good to me, I have not tested the updated JSON format, but will test it before publishing a release

@JamesGibo JamesGibo merged commit 37224f9 into weaveworks:main May 18, 2023
@soheil-ta
Copy link

@JamesGibo Is there any plan to release the latest version including this fix?
The latest version is 0.7.0 from Nov 2022 which does not include the fix.

@urosht
Copy link

urosht commented Aug 26, 2023

+1 here, any idea on the next release for this, @JamesGibo?

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.

None yet

8 participants