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

Feature to create new dashboard in grafana #1

Merged
merged 1 commit into from
Aug 3, 2017

Conversation

rishubhjain
Copy link
Contributor

  • Create new dashboard in grafana.
  • Updated README.md

@rishubhjain rishubhjain force-pushed the feature/create_new_dashboard branch 29 times, most recently from e284965 to f8c8ad3 Compare July 31, 2017 11:57
@rishubhjain
Copy link
Contributor Author

@anivargi @r0h4n @GowthamShanmugam @cloudbehl Please review

@@ -0,0 +1,66 @@
import __builtin__
Copy link
Collaborator

Choose a reason for hiding this comment

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

folder name should be monitoring_integration

@rishubhjain rishubhjain force-pushed the feature/create_new_dashboard branch 8 times, most recently from f26873d to bfe6bac Compare August 1, 2017 16:29
@TimothyAsir
Copy link

Please link the corresponding spec no

README.rst Outdated

::

$ cp tendrl/monitoring_integration/grafana/grafana.ini /etc/grafana/.
Copy link
Collaborator

Choose a reason for hiding this comment

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

why you are maintaining grafana.ini in monitoring-integration and copying it to /etc/grafana/, Instead of that mention what is the changes in wants to do in /etc/grafana/grafana.ini

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are few changes in grafana.ini which are system independent and requires no inputs from user. So in my opinion copying the file directly is a better approach

@TimothyAsirJeyasing
Copy link
Contributor

Few changes required for packaging prospective:

  1. Use /etc/tendrl/monitoring_integration path for grafana.conf.yml
  2. Use entrypoint in setup.py to avoid using python init.py execution.
    rishubhjain@52d3c5a

@@ -0,0 +1,407 @@
##################### Grafana Configuration Example #####################
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this file to etc/grafana/grafana.ini from tendrl/monitoring_integration/grafana/grafana.ini in this repo

Also move all screenshots to etc/grafana/screenshots/ in this repo

"Development Status :: 4 - Beta"
],
zip_safe=False,
install_requires=[
Copy link
Contributor

Choose a reason for hiding this comment

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

please explain more @GowthamShanmugam

grafana.conf.yml Outdated
user: admin
password: admin

port: 3000
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are assuming that grafana service will always run on tendrl-server (with API, etcd), we dont need to allow configurable port and host

cc @Tendrl/tendrl-core , @anivargi @anmolbabu


type: graphite

host: '127.0.0.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are assuming that graphite service will always run on tendrl-server (with API, etcd), we dont need to allow configurable port and host

cc @Tendrl/tendrl-core , @anivargi @anmolbabu

basicAuth: false

isDefault: true

Copy link
Contributor

Choose a reason for hiding this comment

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

remove unused options

@@ -0,0 +1,60 @@
import os
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should be called "tendrl/monitoring_integration/grafana/utils.py" instead of connection.py

import os
import yaml
import socket
from grafana.exceptions import ConfigNotFoundException
Copy link
Contributor

Choose a reason for hiding this comment

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

from tendrl.monitoring_integration.grafana import exceptions

then use
exceptions.ConfigNotFoundException

@@ -0,0 +1,75 @@
import __builtin__
Copy link
Contributor

Choose a reason for hiding this comment

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

fix imports as per below comments on other files

def main():

dashboards = []
config = get_conf("../../grafana.conf.yml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on my comment below, you will now need to load the "/etc/tendrl/monitoring-integration/monitoring-integration.conf.yml" instead of loading ../../grafana.conf.yml and graphite.conf.yml

response_status = []

for datasource in config.datasource:
datasource_config = get_conf("../../" + str(datasource) + ".conf.yml")
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is loading graphite.conf.yml, please remove it and use /etc/monitoring-integration/monitoring-integration.conf.yml

README.rst Outdated

* Grafana instance is set-up and running on the server node.

* Graphite instance in set-up and running on the same server node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@r0h4n r0h4n left a comment

Choose a reason for hiding this comment

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

+pep8 fixes required

''' Create Datasource '''


def post_datasource(datasource_json):
Copy link
Contributor

Choose a reason for hiding this comment

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

Make post_datasource private method then (_post_datasource)

@@ -0,0 +1,96 @@
import __builtin__
Copy link
Contributor

Choose a reason for hiding this comment

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

convert methods that are used only in this file as private methods

README.rst Outdated

* Development setup

* Create a json file in tendrl/monitoring-integration/grafana/dashboards and provide json
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have the dashboards at /etc/tendrl/monitoring-integration/grafana/dashboards/

@@ -0,0 +1,42 @@
# Graphite Web Basic mod_wsgi vhost
Copy link
Contributor

Choose a reason for hiding this comment

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

etc/tendrl/monitoring-integration/graphite/graphite-web.conf.sample

sys.stdout.write('\n' + "Dashboard " + str(dashboard_json) +
" already exists" + '\n')
continue
response = dashboard.create_dashboard(dashboard_json)
Copy link
Contributor

Choose a reason for hiding this comment

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

pass dashboard dir as "/etc/tendrl/grafana/dashboards" to create dashboard

response = post_dashboard(dashboard_json)
return response

except exceptions.ConnectionFailedException:
Copy link
Contributor

Choose a reason for hiding this comment

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

exceptions from post if any? Applicable whereever python-requests(post,get etc) are used

Copy link
Contributor Author

@rishubhjain rishubhjain Aug 3, 2017

Choose a reason for hiding this comment

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

Before making a post request I am checking if the port is open and handling exceptions during this check.
Will an additional exception handling required during post requests?
In my opinion if the port is open, and if there is any error in request then it will be received as a response instead of exception

sys.stdout.write('\n' + "Dashboard " + str(dashboard_json) +
" already exists" + '\n')
continue
response = dashboard.create_dashboard(dashboard_json)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the spec, we were talking about copying the json files only first time. Further it will be skipped. Where this is implemented?

Copy link
Collaborator

Choose a reason for hiding this comment

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

dashboards = dashboard.get_all_dashboards()

 title = []

 for dashboard_json in dashboards:
    title.append(dashboard_json["uri"].split('/')[1])

 for dashboard_json in config.dashboards:
     if dashboard_json in title:
         sys.stdout.write('\n' + "Dashboard " + str(dashboard_json) +
                          " already exists" + '\n')
         continue

Copy link
Collaborator

@GowthamShanmugam GowthamShanmugam Aug 3, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

README.rst Outdated

* Grafana instance is set-up and running on the server node.

* Graphite instance in set-up and running on the same server node.
Copy link
Member

Choose a reason for hiding this comment

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

@rishubhjain /s/Graphite instance in/Graphite instance is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@r0h4n
Copy link
Contributor

r0h4n commented Aug 3, 2017

@Tendrl/tendrl-core please approve

@r0h4n r0h4n merged commit 8701e0c into Tendrl:master Aug 3, 2017
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.

7 participants