Merge ~whereisrysmind/charm-grafana:bugs/1875691 into charm-grafana:master

Proposed by Ryan Farrell
Status: Rejected
Rejected by: Haw Loeung
Proposed branch: ~whereisrysmind/charm-grafana:bugs/1875691
Merge into: charm-grafana:master
Diff against target: 431 lines (+239/-19) (has conflicts)
4 files modified
actions/import-dashboard (+37/-9)
lib/charms/layer/grafana.py (+130/-1)
reactive/grafana.py (+71/-9)
tests/unit/requirements.txt (+1/-0)
Conflict in lib/charms/layer/grafana.py
Conflict in reactive/grafana.py
Reviewer Review Type Date Requested Status
Xav Paice (community) Needs Information
Peter Sabaini (community) Needs Fixing
Drew Freiberger (community) Approve
BootStack Reviewers mr tracking; do not claim Pending
BootStack Reviewers Pending
BootStack Reviewers Pending
Review via email: mp+384918@code.launchpad.net

Commit message

Standardize process for updating dashboards

Grafana dashboards were being POSTed to Grafana in two different places
in the code depending on whether they were from built-in templates, the
charm's run action, or the dashboards relation. Only for the built-in
templates were dashboards being checked against existing dashboards for
differences prior to importing them. In all cases Grafana would not
honor updates to exiting dashboards due to missing "overwrite=True"
flag.

- This commit moves the intelligent diff logic from reactive/grafana.py
to its own function in layer/grafana.py, and leverages import_dashboard
anywhere where dashboards are being imported.

- The reactive handler to import_dashboards will now iterate over all
requests from the dashboards endpoint regardless of whether they have
a response. This ensures that updates will be attempted to existing
dashboards.

- All POSTs to add or change the dashboard now use "overwrite=True" to
ensure Grafana will overwrite them.

- The import_dashboards action now returns more useful
failure information.

Partially Resolves: LP1875691
Resolves: LP1876371

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
Drew Freiberger (afreiberger) wrote :

A couple of concerns in-line.

review: Needs Fixing
ad6e920... by Ryan Farrell

Fixed import_dashboard reactve flag

The reactive flag endpoint.dashboards.requests is only true for new
requests, and does not trigger true for updated requests. This commit
changes import_dashboards to use endpoint.dashboards.has_requests in
order to ensure dashboard updates will continually attempt to update
existing requests. It will still run comparison of the dashboard data
against existing dashboards in grafana so as to avoid repeated,
unnecessary POSTs.

1d2dac8... by Ryan Farrell

Improved check_if_dashboard_update_required logic

This commit changes logic around checking existing dashboards for a
match with a new dashboard to be imported. It now only matches with the
dashboard titles for a list of uids. If the uid list is empty, the
dashboard is new and no further checking is required. If the list is
more than one item long, it will log a warning that only one dashboard
will be used for comparison and update; this should not happen because
grafana doesnt allow dashboards with duplicate titles.

Additional changes:
- Implements a check for a dashboard title when importing, and returns a
failure to import instead of applying the name "Untitled".
- Fixes a syntax error when logging success on the import_dashboards
action.

Revision history for this message
Drew Freiberger (afreiberger) wrote :

My concerns have been addressed and the change lgtm. I've not done testing, but code looks sound.

Thanks for the contribution, Ryan!

review: Approve
dd6ae02... by Ryan Farrell

Added helper function to get application name

Helper function works by searching through the relation data, as seen
from the perspective of grafana to match the request id with the
originating unit. It splits the related unit name and returns the
application name so that the source application can be inserted into the
dashboard title.

df35a35... by Ryan Farrell

Added try/except to get source application

Revision history for this message
Ryan Farrell (whereisrysmind) wrote :

Added a couple more commits which include a new helper function to assist in modifying the dashboard title from "[juju] blah" to "[juju - $source - application] blah". This improved scalability of the dashboards relation when grafana is related to multiple application instances stemming from the same charm.
I tested this in my lab to confirm it works as expected for cross-model and cross-controller relations, which will enforce unique application names when the relation offers are being accepted.

Revision history for this message
Peter Sabaini (peter-sabaini) wrote :

There are some conflicts that have crept into the commit and would need fixing.

review: Needs Fixing
Revision history for this message
James Hebden (ec0) wrote :

Looks good to me once the branch is rebased from HEAD

Revision history for this message
Xav Paice (xavpaice) wrote :

this change is a lot of commits, that may have some conflict around https://code.launchpad.net/~peter-sabaini/charm-grafana/+git/grafana-charm/+merge/386564

Currently there's a lot of merge conflicts to resolve, and because a lot of parallel work has been editing similar items I think it's important that the original author reviews if this change is even still needed.

review: Needs Information

Unmerged commits

df35a35... by Ryan Farrell

Added try/except to get source application

dd6ae02... by Ryan Farrell

Added helper function to get application name

Helper function works by searching through the relation data, as seen
from the perspective of grafana to match the request id with the
originating unit. It splits the related unit name and returns the
application name so that the source application can be inserted into the
dashboard title.

1d2dac8... by Ryan Farrell

Improved check_if_dashboard_update_required logic

This commit changes logic around checking existing dashboards for a
match with a new dashboard to be imported. It now only matches with the
dashboard titles for a list of uids. If the uid list is empty, the
dashboard is new and no further checking is required. If the list is
more than one item long, it will log a warning that only one dashboard
will be used for comparison and update; this should not happen because
grafana doesnt allow dashboards with duplicate titles.

Additional changes:
- Implements a check for a dashboard title when importing, and returns a
failure to import instead of applying the name "Untitled".
- Fixes a syntax error when logging success on the import_dashboards
action.

ad6e920... by Ryan Farrell

Fixed import_dashboard reactve flag

The reactive flag endpoint.dashboards.requests is only true for new
requests, and does not trigger true for updated requests. This commit
changes import_dashboards to use endpoint.dashboards.has_requests in
order to ensure dashboard updates will continually attempt to update
existing requests. It will still run comparison of the dashboard data
against existing dashboards in grafana so as to avoid repeated,
unnecessary POSTs.

32d92e2... by Ryan Farrell

Fixed import_dashboard title issue

Changed the import_dashboard invocation to let the dashboard title be
automatically parsed from the dashboard json, instead of passed in as
the request.name. This fixed an issue where the dashboards comparison
with whats present in grafana would never actually find a dashboard to
compare with, causing repeated imports.

d60dc7a... by Ryan Farrell

Removed use of helpers.data_changed

Helpers.data_changed did not get reset when relations were removed and
re-added, leading to a situaion where relation data would no longer be
shared over the relation. This commit removes this check and relies on
the already existing grafana dashboard comparson instead.

466c3e0... by Ryan Farrell

Added catch for json.loads errors

b48bac4... by Ryan Farrell

Fixed key cleanup comparing dashboards for update

Previously, the code comparing new and existing dashboards would remove
keys expected to be different from both dictionaries. However, new
dashboards passed via the dashboards relation may already be missing
these keys. This commit checks for the existence of those keys before
attempting to delete them.

7c4708a... by Ryan Farrell

Added failure messages for import dashboard action

0d5c53e... by Ryan Farrell

Implemented dashboard change check during import

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/actions/import-dashboard b/actions/import-dashboard
2index 62c28c5..94728ed 100755
3--- a/actions/import-dashboard
4+++ b/actions/import-dashboard
5@@ -6,6 +6,7 @@ import base64
6 import traceback
7 import os
8 from charmhelpers.core.hookenv import (
9+ ERROR,
10 config,
11 action_fail,
12 action_set,
13@@ -16,6 +17,22 @@ from charmhelpers.core.hookenv import (
14 from charms.layer.grafana import import_dashboard
15
16
17+def import_dashboard_and_log_results(dashboard):
18+ """Helper function which runs import dashboard and reports the results.
19+
20+ :param dashboard: dictionary containing the dashboard
21+ :returns: tuple -- (bool, string), same as the import_dashboard result
22+ """
23+ success, reason = import_dashboard(dashboard)
24+ name = dashboard['dashboard'].get('title')
25+ if not success:
26+ log("Posting Dashboard '{}' failed with error: {}".format(name, reason),
27+ ERROR, )
28+ else:
29+ log("Dashboard '{}' import success".format(name))
30+ return success, reason
31+
32+
33 def import_file(path):
34 with open(path, 'r') as f:
35 d = f.read()
36@@ -24,7 +41,7 @@ def import_file(path):
37 except json.JSONDecodeError as e:
38 action_fail('Fail to json decode the dashboard: {}'.format(e.msg))
39 return False
40- return import_dashboard(j)
41+ return import_dashboard_and_log_results(j)
42
43
44 if action_get('dashboard'):
45@@ -37,22 +54,33 @@ if action_get('dashboard'):
46 except Exception:
47 action_fail('Unhandled exception, check logs')
48 log(traceback.format_exc())
49- import_dashboard(d)
50+ result, message = import_dashboard_and_log_results(d)
51+ if not result:
52+ action_fail("Fail to import the dashboard: {}".format(message))
53+
54
55 if action_get('location'):
56 path = action_get('location')
57 if not os.path.exists(path):
58 action_fail('Provided path does not exist on unit: {}'.format(path))
59 elif os.path.isfile(path):
60- import_file(path)
61+ result, message = import_file(path)
62+ if not result:
63+ action_fail("Fail to import the dashboard: {}".format(message))
64 elif os.path.isdir(path):
65- f_count = 0
66+ f_messages = []
67 s_count = 0
68 for f in os.listdir(path):
69- if import_file(os.path.join(path, f)):
70+ result, message = import_file(os.path.join(path, f))
71+ if result:
72 s_count += 1
73 else:
74- f_count += 1
75- action_set({'summary': 'Found {}. Imported: {}. Failed: {}'.format(s_count+f_count, s_count, f_count)})
76- if f_count > 0:
77- action_fail('Number of dashboards that failed to impor: {}'.format(f_count))
78+ f_messages.append(message)
79+ action_set({'summary': 'Found {}. Imported: {}. Failed: {}'
80+ .format(s_count+len(f_messages),
81+ s_count,
82+ len(f_messages))})
83+ if len(f_messages) > 0:
84+ action_set({'details': '{}'.format(f_messages)})
85+ action_fail('Number of dashboards that failed to import: {}'
86+ .format(len(f_messages)))
87diff --git a/lib/charms/layer/grafana.py b/lib/charms/layer/grafana.py
88index 1ddd431..2758dde 100644
89--- a/lib/charms/layer/grafana.py
90+++ b/lib/charms/layer/grafana.py
91@@ -6,8 +6,12 @@ import subprocess
92
93 import requests
94
95+from jsondiff import diff
96+
97 from charmhelpers.core import unitdata
98 from charmhelpers.core.hookenv import (
99+ DEBUG,
100+ WARNING,
101 config,
102 log,
103 )
104@@ -20,8 +24,106 @@ def get_admin_password():
105 )
106
107
108-def import_dashboard(dashboard, name=None):
109+def get_current_dashboards(port, passwd):
110+ """Returns list of available dashboards.
111+
112+ https://grafana.com/docs/grafana/latest/http_api/folder_dashboard_search/
113+ """
114+ dash_req = requests.get(
115+ "http://127.0.0.1:{}/api/search?type=dash-db".format(port),
116+ auth=('admin', passwd),
117+ )
118+ return dash_req.json() if dash_req.status_code == 200 else []
119+
120+
121+def get_current_dashboard_json(uid, port, passwd):
122+ """Returns the dashboard revision information (fake rev if not found).
123+
124+ https://grafana.com/docs/grafana/latest/http_api/dashboard/#get-dashboard-by-uid
125+ """
126+ # Dashboard attributes that will be removed from the stored version (likely to
127+ # be different) before comparing it to the template. Value is not important.
128+ default_dashboard = {key: 0 for key in ["version", "id", "uid"]}
129+ if not uid:
130+ return default_dashboard
131+
132+ dash_req = requests.get(
133+ "http://127.0.0.1:{}/api/dashboards/uid/{}".format(port, uid),
134+ auth=('admin', passwd),
135+ )
136+ if dash_req.status_code != 200:
137+ return default_dashboard
138+
139+ try:
140+ return json.loads(dash_req.text)["dashboard"]
141+ except json.decoder.JSONDecodeError:
142+ return default_dashboard
143+
144+
145+def check_if_dashboard_update_required(dashboard, name, port, passwd):
146+ """Check if the dashboard contained exists already in Grafana. If it's new or changed,
147+ then return True, otherwise return False.
148+
149+ :param dashboard: Describes the dashboard schema
150+ :type dashboard: dict
151+ :param name: Title of the dashboard
152+ :type name: str
153+ :param port: Grafana application port
154+ :param passwd: Grafana admin password
155+ :type passwd: str
156+
157+ :returns: Whether the Grafana dashboard needs to be updated
158+ :rtype: bool
159+ """
160+ new = dashboard["dashboard"]
161+ current_dashboards = get_current_dashboards(port, passwd)
162+ uids = [dash['uid'] for dash in current_dashboards
163+ if dash['title'] == new['title']]
164+
165+ if len(uids) == 0:
166+ # the dashboard is new
167+ return True
168+ elif len(uids) > 1:
169+ # this should never happen because grafana doesnt allow duplicate titles
170+ # On the off chance it does occur, log a warning
171+ log("Ambiguous entries exist for a dashboard titled '{}'. \
172+ UID '{}' will be used & updated".format(new['title'], uids[0]), WARNING)
173+
174+ # check the rest of the fields to verify if update is required.
175+ curr = get_current_dashboard_json(
176+ uids[0],
177+ port,
178+ passwd,
179+ )
180+ # Remove known differences between template and already stored dashboard
181+ # Common: 'version' (template defaults to 1)
182+ # Common: 'id' (template sets it null for Grafana to generate it. If the dashboard
183+ # already exists in Grafana, "curr" will have an integer stored)
184+ # Stored dashboard: 'uid' (it is also generated by Grafana)
185+ # Dashboards passed in from templates may by missing these keys already
186+ for key in ["version", "id"]:
187+ if key in new.keys():
188+ del new[key]
189+ del curr["uid"], curr["id"], curr["version"]
190+
191+ dashboard_changed = diff(new, curr)
192+ if not dashboard_changed:
193+ return False
194+ else:
195+ log(
196+ "dashboard differences[{}], dashboard title[{}]".format(
197+ dashboard_changed,
198+ name,
199+ ),
200+ DEBUG,
201+ )
202+ return True
203+
204+
205+def import_dashboard(dashboard):
206+ name = dashboard['dashboard'].get('title', None)
207 if name is None:
208+<<<<<<< lib/charms/layer/grafana.py
209 name = dashboard["dashboard"].get("title") or "Untitled"
210 headers = {"Content-Type": "application/json"}
211 import_url = "http://localhost:{}/api/dashboards/db".format(config("port"))
212@@ -39,6 +141,33 @@ def import_dashboard(dashboard, name=None):
213 False,
214 'Error importing dashboard "{}": {}'.format(name, r.text),
215 )
216+=======
217+ return (False, 'Dashboard title cannot be empty')
218+ port = config('port')
219+ headers = {'Content-Type': 'application/json'}
220+ import_url = 'http://localhost:{}/api/dashboards/db'.format(port)
221+ passwd = get_admin_password()
222+ if passwd is None:
223+ return (False, 'Unable to retrieve grafana password.')
224+ if not check_if_dashboard_update_required(dashboard, name, port, passwd):
225+ log(
226+ "Skipping Dashboard: already up to date: '{}'".format(name)
227+ )
228+ return (True, '')
229+
230+ # overwrite flag must true for grafana to update existing dashboards
231+ dashboard['overwrite'] = True
232+ log("POSTing Dashboard: {}".format(name))
233+
234+ api_auth = ('admin', passwd)
235+ r = requests.post(import_url, auth=api_auth, headers=headers,
236+ data=json.dumps(dashboard))
237+ if r.status_code == 200:
238+ return (True, None)
239+ else:
240+ return (False, "Error importing dashboard '{}': {}".format(name,
241+ r.text))
242+>>>>>>> lib/charms/layer/grafana.py
243
244
245 def get_cmd_output(cmd, shell=False):
246diff --git a/reactive/grafana.py b/reactive/grafana.py
247index d12a2c6..de62892 100644
248--- a/reactive/grafana.py
249+++ b/reactive/grafana.py
250@@ -38,8 +38,6 @@ from charms.reactive.helpers import (
251
252 from jinja2 import Environment, FileSystemLoader, exceptions
253
254-from jsondiff import diff
255-
256 import requests
257
258 import six
259@@ -460,13 +458,17 @@ def wipe_nrpe_checks():
260 @when("grafana.admin_password.set")
261 def configure_sources(relation):
262 sources = relation.datasources()
263- gf_adminpasswd = get_admin_password()
264 for ds in sources:
265 hookenv.log("Found datasource: {}".format(str(ds)))
266 # Ensure datasource is configured
267 check_datasource(ds)
268+<<<<<<< reactive/grafana.py
269 if ds["type"] == "prometheus":
270 generate_prometheus_dashboards(gf_adminpasswd, ds)
271+=======
272+ if ds['type'] == 'prometheus':
273+ generate_prometheus_dashboards(ds)
274+>>>>>>> reactive/grafana.py
275
276
277 @hook("grafana-source-relation-departed")
278@@ -610,6 +612,7 @@ def render_custom(source, context, **parameters):
279 return template.render(context)
280
281
282+<<<<<<< reactive/grafana.py
283 def get_current_dashboards(port, passwd):
284 """Returns list of available dashboards.
285
286@@ -650,6 +653,9 @@ def get_current_dashboard_json(uid, port, passwd):
287 def check_and_add_dashboard(
288 filename, context, prom_metrics, dash_to_uid, port, gf_adminpasswd
289 ):
290+=======
291+def check_and_add_dashboard(filename, context, prom_metrics):
292+>>>>>>> reactive/grafana.py
293 """Verifies if the rendered dashboard from template is new or do nothing.
294
295 * Renders a template with context gathered from
296@@ -687,6 +693,7 @@ def check_and_add_dashboard(
297 return
298
299 dashboard_json = json.loads(dashboard_str)
300+<<<<<<< reactive/grafana.py
301 # before uploading the dashboard, we should check that it doesn't already
302 # exist with same data lp#1858490
303 new = dashboard_json["dashboard"]
304@@ -734,11 +741,19 @@ def check_and_add_dashboard(
305 "Posting template {} failed with error: {}".format(
306 filename, r.text
307 ),
308+=======
309+ success, reason = import_dashboard(dashboard_json)
310+ if success:
311+ hookenv.log("Using Dashboard Template: {}".format(filename))
312+ else:
313+ hookenv.log(
314+ "Posting template '{}' failed with error: {}".format(filename, reason),
315+>>>>>>> reactive/grafana.py
316 hookenv.ERROR,
317 )
318
319
320-def generate_prometheus_dashboards(gf_adminpasswd, ds):
321+def generate_prometheus_dashboards(ds):
322 # prometheus_host = ds
323 ds_name = "{} - {}".format(ds["service_name"], ds["description"])
324 config = hookenv.config()
325@@ -755,6 +770,7 @@ def generate_prometheus_dashboards(gf_adminpasswd, ds):
326 )
327 return
328
329+<<<<<<< reactive/grafana.py
330 current_dashboards = get_current_dashboards(config["port"], gf_adminpasswd)
331 dash_to_uid = {
332 dash["title"]: dash["uid"]
333@@ -771,6 +787,16 @@ def generate_prometheus_dashboards(gf_adminpasswd, ds):
334 "bonds_enabled": "bond_status" in prom_metrics,
335 "conntrack_enabled": "nf_conntrack_max" in prom_metrics,
336 }
337+=======
338+ prom_metrics = response.json()['data']
339+ templates_dir = 'templates/dashboards/prometheus'
340+ context = {'datasource': ds_name,
341+ 'external_network': config['external_network'],
342+ 'bcache_enabled': "bcache_cache_hit_ratio" in prom_metrics,
343+ 'bonds_enabled': "bond_status" in prom_metrics,
344+ 'conntrack_enabled': "nf_conntrack_max" in prom_metrics,
345+ }
346+>>>>>>> reactive/grafana.py
347 # we ignore some phrases picked up by the regex because they're not real
348 # metric names. Let's add them to the list of metrics to make life easy.
349 ignore_metrics = [
350@@ -796,9 +822,6 @@ def generate_prometheus_dashboards(gf_adminpasswd, ds):
351 filename,
352 context,
353 prom_metrics,
354- dash_to_uid,
355- config["port"],
356- gf_adminpasswd,
357 )
358
359
360@@ -989,16 +1012,49 @@ def hpwgen(passwd, salt):
361 return hpasswd
362
363
364+<<<<<<< reactive/grafana.py
365 @when("grafana.started", "endpoint.dashboards.requests")
366+=======
367+def _get_source_application_from_request_id(request_id):
368+ """Helper function steps through the relation data until it finds the unit
369+ that originated the given request_id to return the source application name.
370+ """
371+ try:
372+ dashboards_relations_info = hookenv.relations().get("dashboards")
373+ for rel_id in dashboards_relations_info:
374+ for related_unit in filter(
375+ lambda x: x.split("/")[0] != hookenv.application_name(),
376+ dashboards_relations_info[rel_id]):
377+ application = related_unit.split("/")[0]
378+ for unit_key in \
379+ dashboards_relations_info[rel_id][related_unit].keys():
380+ if request_id in unit_key:
381+ return application
382+ except Exception as e:
383+ hookenv.log("Unable to parse source application for Request {}: {}".format(request_id, e),
384+ hookenv.WARNING)
385+ return None
386+
387+@when('grafana.started',
388+ 'endpoint.dashboards.has_requests')
389+>>>>>>> reactive/grafana.py
390 def import_dashboards(dashboards):
391- for request in dashboards.new_requests:
392+ for request in dashboards.all_requests:
393+ application = _get_source_application_from_request_id(request._id)
394 try:
395 dash = request.dashboard
396 if isinstance(request.dashboard, str):
397 dash = json.loads(request.dashboard)
398- success, reason = import_dashboard(dash, request.name)
399+ hookenv.log('Importing dashboard: {}'.format(request.name))
400+ if application is not None:
401+ # Update the dashboard title
402+ dash["dashboard"]["title"] = dash["dashboard"]["title"]\
403+ .replace("[juju]", "[juju - {}]".format(application))
404+
405+ success, reason = import_dashboard(dash)
406 except json.decoder.JSONDecodeError as error:
407 success, reason = False, str(error)
408+<<<<<<< reactive/grafana.py
409
410 hookenv.log(
411 "import_dashboards via grafana-dashboards: {} [{}]".format(
412@@ -1006,4 +1062,10 @@ def import_dashboards(dashboards):
413 ),
414 hookenv.DEBUG,
415 )
416+=======
417+ hookenv.log("import_dashboards via grafana-dashboards: {} [{}]"
418+ .format(success, reason),
419+ hookenv.DEBUG
420+ )
421+>>>>>>> reactive/grafana.py
422 request.respond(success, reason)
423diff --git a/tests/unit/requirements.txt b/tests/unit/requirements.txt
424index 29f9318..b8e5360 100644
425--- a/tests/unit/requirements.txt
426+++ b/tests/unit/requirements.txt
427@@ -4,3 +4,4 @@ mock
428 pytest
429 pytest-cov
430 requests
431+jsondiff

Subscribers

People subscribed via source and target branches

to all changes: