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.

Sign up for GitHub

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

Jump to bottom

Fix rendering AlertRulev9 json for possible use in Alerting provisioning API #568

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix rendering AlertRulev9 json for possible use in Alerting provisioning API #568

wants to merge 4 commits into from

Conversation

ilia-ilin-jiji
Copy link

@ilia-ilin-jiji ilia-ilin-jiji commented Jan 27, 2023 •
edited
Loading

What does this do?

This PR fix generates the correct json for HTTP API requests.

  1. Add link alert with dashboard and panel. In AlertRulev9 class exist params panel_id and dashboard_uid but didn't use in json generating.
  2. Fix defaults for labels and annotations params because are work incorrectly. If I change param in one alert is changed in all
>>> import attr
>>> @attr.s
... class X:
...     d = attr.ib(default={})
...
>>> a = X()
>>> b = X()
>>> a.d['test']='test'
>>> b
X(d={'test': 'test'})
>>>
  1. Add DataSource class for correct generating targets in alert_rule, because API needs this struct:
{
	"datasourceUid": "uid",
	"model": {
		"datasource": {
			"type": "prometheus",
			"uid": "uid"
		}
	}
}

Why is it a good idea?

Context

Questions

KacperLegowski
KacperLegowski reviewed Feb 6, 2023
View reviewed changes
grafanalib/core.py
@@ -1633,7 +1653,9 @@ def to_json_data(self):
"annotations": self.annotations,
"data": data,
"noDataState": self.noDataAlertState,
"execErrState": self.errorAlertState
"execErrState": self.errorAlertState,
Copy link

Choose a reason for hiding this comment

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

Are you sure that this model is correct? According to this API: https://editor.swagger.io/?url=https://raw.githubusercontent.com/grafana/grafana/main/pkg/services/ngalert/api/tooling/post.json looks that grafana_alert object is required for grafana managed alerts 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please could you remove this part from the PR

@JDuskey
Copy link

JDuskey commented Mar 6, 2023

#544 Has the correct implementation of this fix. @KacperLegowski is right that alerts need a nested grafana_alert object.

JamesGibo
JamesGibo reviewed May 18, 2023
View reviewed changes
grafanalib/core.py
Comment on lines +1596 to +1597
folderUid = attr.ib(default=None, validator=attr.validators.optional(instance_of(str)))
ruleGroup = attr.ib(default=None, validator=attr.validators.optional(instance_of(str)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add document string for these

JamesGibo
JamesGibo requested changes May 18, 2023
View reviewed changes
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.

Thanks for the PR, there looks like there are some really useful improvements in the PR.
Would you be bale to-do the following:

  • rebase on master
  • Remove the incorrect
  • Add documentation strings for the additional fields you have added

Gnappuraz
Gnappuraz reviewed Jun 1, 2023
View reviewed changes
Copy link

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

I would be happy to help out on this. I'm currently trying to use the lib to implement alert provisioning via the new API but I'm struggling because of this and other issues.

grafanalib/core.py
@@ -1633,7 +1653,9 @@ def to_json_data(self):
"annotations": self.annotations,
"data": data,
"noDataState": self.noDataAlertState,
"execErrState": self.errorAlertState
"execErrState": self.errorAlertState,
Copy link

Choose a reason for hiding this comment

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

Suggested change
"execErrState": self.errorAlertState,

grafanalib/core.py
@@ -1574,13 +1586,15 @@ class AlertRulev9(object):
:param timeRangeTo: Time range interpolation data finish at
:param uid: Alert UID should be unique
:param dashboard_uid: Dashboard UID that should be use for linking on alert message
:param panel_id: Panel ID that should should be use for linking on alert message
:param panel_id: Panel ID that should be use for linking on alert message
Copy link

Choose a reason for hiding this comment

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

Suggested change
:param panel_id: Panel ID that should be use for linking on alert message
:param panel_id: Panel ID that should be used for linking on alert message
:param annotations: An array of additional annotations, can be used for alert message
:param labels: An array of labels, can be used with Notification Policies
:param folderUid: Folder ID that should be use for linking on alert message
:param ruleGroup: A group for alerts that are evaluated together

Gnappuraz
Gnappuraz reviewed Jun 1, 2023
View reviewed changes
Copy link

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

I would be happy to help out on this. I'm currently trying to use the lib to implement alert provisioning via the new API but I'm struggling because of this and other issues.

Gnappuraz
Gnappuraz reviewed Jun 1, 2023
View reviewed changes
Copy link

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

I would be happy to help out on this. I'm currently trying to use the lib to implement alert provisioning via the new API but I'm struggling because of this and other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KacperLegowski KacperLegowski KacperLegowski left review comments

@Gnappuraz Gnappuraz Gnappuraz left review comments

@JamesGibo JamesGibo JamesGibo requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ilia-ilin-jiji @JDuskey @Gnappuraz @KacperLegowski @JamesGibo @bkmz