Load dashboards for kibana on startup US314 #116

john-gress · 2019-09-17T23:21:29Z

See user story: https://rally1.rallydev.com/#/detail/userstory/334308455668

sperry94

Just a few minor comments, but nothing serious. Did not test.

sperry94 · 2019-09-18T16:33:34Z

scripts/loadAssets.py

-search_type = "search"
+        session = requests.Session()
+        # Retry on Internal Server Error (500) and Service Unavailable (503)
+        retries = Retry(total=5, backoff_factor=0.3, status_forcelist=[ 500, 503])


do we want to retry on other errors here?

Added 408, 502, and 504.

sperry94 · 2019-09-18T16:33:45Z

scripts/exportAssets.py

+
+        session = requests.Session()
+        # Retry on Internal Server Error (500) and Service Unavailable (503)
+        retries = Retry(total=5, backoff_factor=0.3, status_forcelist=[ 500, 503])


do we want to retry on other errors here?

I added:

Request Timeout (408)

Bad Gateway (502)

Gateway timeout (504)

sperry94 · 2019-09-18T16:35:52Z

scripts/loadAssets.py

-                                          es_id,
-                                          content)
+    try:
+        url = 'http://localhost:9200/.kibana/_doc/' + es_id


we might want to grab the id from the imported file? If ever the es_id and the id from the dashboard differ, I'm not sure which id elastic search would take

The id is not saved in the json, so not available there. I think we are depending on the name of the file to have the correct id.

sperry94 · 2019-09-18T16:37:34Z

scripts/loadAssets.py

+    es_ok = check_elasticsearch_health()
+    if es_ok:
+        # Create arrays of the filenames in the resources directory
+        objects = [filename for filename in listdir(resources) if isfile(join(resources, filename))]


this iteration could get moved into the load_assets function, with the only parameter to load_assets being the path_to_files

Agreed. Changed as suggested.

Schatzman

Left some comments but just about python style, nothing that NEEDS to change 👍 - Reviewed but not tested.

Schatzman · 2019-09-18T16:28:12Z

scripts/exportAssets.py

+
+        session = requests.Session()
+        # Retry on Internal Server Error (500) and Service Unavailable (503)
+        retries = Retry(total=5, backoff_factor=0.3, status_forcelist=[ 500, 503])


Do we want to retry for 502 (bad gateway) and 504 (gateway timeout) also potentially? https://httpstatuses.com/

Schatzman · 2019-09-18T16:29:32Z

scripts/exportAssets.py

+        return False
+
+def get_search_title_json(es_type, es_title):
+    esField = es_type + '.title'


Guido Van Rossum frowns upon your camel case, python PEP8 prefers snake case for functions and variables http://www.python.org/dev/peps/pep-0008/

Guido is right. Changed to snake case.

Schatzman · 2019-09-18T16:31:26Z

scripts/exportAssets.py

+        session.mount('https://', HTTPAdapter(max_retries=retries))
+        session.mount('http://', HTTPAdapter(max_retries=retries))
+        esQuery = json.dumps(es_query)
+        headers = { 'Content-type': 'application/json' }


There is a lot of variability in this file between single and double quotes, might be good to pick one or the other and stick with them throughout for consistency. I tend to default to single quotes in python.

Single quotes it is. Fixed.

Schatzman · 2019-09-18T16:33:31Z

scripts/exportAssets.py

@@ -118,33 +162,40 @@ def main(argv):
    global OUTPUT_DIR
    global FILE_PATHS_AND_CONTENTS
    global TYPE
-    global ID
+    global TITLE


All of these globals are not "pythonic". Prefer passing as params and returning modified variables rather than modifying globals.

Schatzman · 2019-09-18T16:35:46Z

scripts/loadAssets.py

+    es_ok = check_elasticsearch_health()
+    if es_ok:
+        # Create arrays of the filenames in the resources directory
+        objects = [filename for filename in listdir(resources) if isfile(join(resources, filename))]


Ooh fancy list comprehension! ;)

Kudos to @craig-cogdill

craig-cogdill · 2019-09-18T16:23:59Z

scripts/exportAssets.py

    else:
-        print "ERROR:   Did not find any " + TYPE + " named " + ID + " in Elasticsearch."
-        sys.exit(2)
+      print "elasticsearch is not ready\n"


Should this be logger.error()?

No. Again this lets the user know their request didn't work.

craig-cogdill · 2019-09-18T16:24:10Z

scripts/exportAssets.py

+                get_all_dashboard_content_from_ES(dashboard_raw=UTIL.safe_list_read(list_ob=FILE_PATHS_AND_CONTENTS, key=get_full_path(asset_id=asset_id)))
+            export_all_files()
+        else:
+            print "ERROR:   Did not find any " + TYPE + " named " + TITLE + " in Elasticsearch."


Should this be logger.error()?

No. In this case, the error has already been logged to file in the call to get_asset. This output to the terminal lets the user know it failed. Added additional print statement to point the user to the log file for more info.

craig-cogdill · 2019-09-18T16:25:59Z

scripts/exportAssets.py

+
+        session = requests.Session()
+        # Retry on Internal Server Error (500) and Service Unavailable (503)
+        retries = Retry(total=5, backoff_factor=0.3, status_forcelist=[ 500, 503])


Space between [ 500,

Fixed. Also added 408, 502, and 504.

craig-cogdill · 2019-09-18T16:27:10Z

scripts/exportAssets.py

+        return True
+
+    except Exception as err:
+        logger.warning("[loadAssets.py] Caught HTTP exception: {0}".format(err))


[loadAssets.py] --> [exportAssets.py]

craig-cogdill · 2019-09-18T16:27:18Z

scripts/exportAssets.py

+        return True, response.text
+
+    except Exception as err:
+        logger.info("[getEs.py] Caught HTTP exception: {0}".format(err))


[getEs.py] --> [exportAssets.py]

craig-cogdill · 2019-09-18T16:27:56Z

scripts/exportAssets.py

+        if not response.ok:
+            response.raise_for_status()
+
+        logger.info("GET RETURNS: " + "\n" + UTIL.pretty_format(response.json()))


This could probably go before if not response.ok so that we see an error response as well? Or we could remove it entirely.

Moved before if not response.ok

craig-cogdill · 2019-09-18T16:31:41Z

scripts/exportAssets.py

-        success, ret = get_asset(INDEX, panel_type, panel_id)
+        es_query = get_search_id_json(panel_type, panel_id)
+        success, asset_raw = get_asset(es_query)
+        asset_id = panel_type + ":" + panel_id


This should go in the if block below

Good catch. Fixed.

craig-cogdill · 2019-09-18T16:32:27Z

scripts/loadAssets.py

+        load_assets(resources, objects)
+        set_default_index_pattern()
+    else:
+      print "elasticsearch is not ready\n"


logger.error()?

Agreed. In this case it makes sense because the script is run out of a systemd unit file.

Load dashboards for kibana on startup US314

Load dashboards for kibana on startup.

e5eafe1

sperry94 approved these changes Sep 18, 2019

View reviewed changes

Schatzman approved these changes Sep 18, 2019

View reviewed changes

craig-cogdill suggested changes Sep 18, 2019

View reviewed changes

Address code review comments.

b6e01b8

craig-cogdill approved these changes Sep 20, 2019

View reviewed changes

john-gress merged commit a42da03 into logrhythm:7.2.0_NM Sep 20, 2019

sperry94 pushed a commit that referenced this pull request Jan 30, 2020

Merge pull request #116 from john-gress/import_export_dashboards_US314

2b17fd6

Load dashboards for kibana on startup US314

Load dashboards for kibana on startup US314 #116

Load dashboards for kibana on startup US314 #116

Conversation

john-gress commented Sep 17, 2019

sperry94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schatzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment