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 AlertCondition backwards compatibility #584

Merged
merged 3 commits into from
May 18, 2023

Conversation

ali-essam
Copy link
Contributor

What does this do?

Add useNewAlerts property to AlertCondition and default it to False

Why is it a good idea?

When AlertCondition is used inside of Alert.alertConditions, json serialization fails as the property is not set. When the default is set to False this will allow the AlertCondition to behave as expected when used with old grafana alerting. In case of 8.x and 9.x, this is already overriden to True in the code so the functionality will work as expected in the new alerting.

In [14]: from grafanalib import core as G

In [15]: from grafanalib import _gen as glGen

In [16]: import json

In [17]: json.dumps(G.Alert(name='name', message='message', alertConditions=[G.AlertCondition(G.Target(refI
    ...: d="A"), G.Evaluator('a', 'b'), G.TimeRange('5', '6'), 'd', 'e')]), cls=glGen.DashboardEncoder)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-17-a458c89b5490> in <module>
----> 1 json.dumps(G.Alert(name='name', message='message', alertConditions=[G.AlertCondition(G.Target(refId="A"), G.Evaluator('a', 'b'), G.TimeRange('5', '6'), 'd', 'e')]), cls=glGen.DashboardEncoder)

~/.pyenv/versions/3.8.13/lib/python3.8/json/__init__.py in dumps(obj, skipkeys, ensure_ascii, check_circular, allow_nan, cls, indent, separators, default, sort_keys, **kw)
    232     if cls is None:
    233         cls = JSONEncoder
--> 234     return cls(
    235         skipkeys=skipkeys, ensure_ascii=ensure_ascii,
    236         check_circular=check_circular, allow_nan=allow_nan, indent=indent,

~/.pyenv/versions/3.8.13/lib/python3.8/json/encoder.py in encode(self, o)
    197         # exceptions aren't as detailed.  The list call should be roughly
    198         # equivalent to the PySequence_Fast that ''.join() would do.
--> 199         chunks = self.iterencode(o, _one_shot=True)
    200         if not isinstance(chunks, (list, tuple)):
    201             chunks = list(chunks)

~/.pyenv/versions/3.8.13/lib/python3.8/json/encoder.py in iterencode(self, o, _one_shot)
    255                 self.key_separator, self.item_separator, self.sort_keys,
    256                 self.skipkeys, _one_shot)
--> 257         return _iterencode(o, 0)
    258
    259 def _make_iterencode(markers, _default, _encoder, _indent, _floatstr,

~/....venv/lib/python3.8/site-packages/grafanalib/_gen.py in default(self, obj)
     21         to_json_data = getattr(obj, 'to_json_data', None)
     22         if to_json_data:
---> 23             return to_json_data()
     24         return json.JSONEncoder.default(self, obj)
     25

~/.../.venv/lib/python3.8/site-packages/grafanalib/core.py in to_json_data(self)
   1203             'query': {
   1204                 'model': self.target.to_json_data(),
-> 1205                 'params': self.__get_query_params(),
   1206             },
   1207             'reducer': {

~/.../.venv/lib/python3.8/site-packages/grafanalib/core.py in __get_query_params(self)
   1190     def __get_query_params(self):
   1191         # Grafana 8.x alerts do not put the time range in the query params.
-> 1192         if self.useNewAlerts:
   1193             return [self.target.refId]
   1194

AttributeError: 'AlertCondition' object has no attribute 'useNewAlerts'

Context

Fixes #545

Questions

  • Is defaulting to False a safe assumption?

@ali-essam
Copy link
Contributor Author

@JamesGibo can you please review this 🙏 ?

@serboctor serboctor requested a review from JamesGibo May 18, 2023 08:57
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.
It looks good to me and I cannot see an issue with defaulting to false, as you said this is overridden when using alerts with Grafana v9+.

@JamesGibo JamesGibo merged commit 5cd3d37 into weaveworks:main May 18, 2023
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.

AlertCondition object missing useNewAlerts attribute
2 participants