-
Notifications
You must be signed in to change notification settings - Fork 15
System monitor #148
base: master
Are you sure you want to change the base?
System monitor #148
Conversation
… bootstrap Watcher and Responder
2bb7410
to
26d50b9
Compare
Supper excited about this 🚀 I'll give it a look asap. |
Looks cool, always nice to see some dashboards :) I think you might be able to do without your own custom loading code though. You can take two basic approaches to loading logs: handler or agent. The handler approach is to add an additional logging stream. You already have a console and file stream:
so you would just add a third stream for elasticsearch. To do this you'd want to use a 3rd party lib as it would do a better job of keeping track of location (in case of crash), and avoiding blocking io. I dont work with python so I dont know how to assess the quality of the lib, but this seems ok: https://github.com/cmanaha/python-elasticsearch-logger The agent approach is the one you've taken already, but you can use an existing agent instead of writing your own. Filebeat (https://www.elastic.co/beats/filebeat) will keep track of files and locations, and you can use logstash if you need transforms (https://www.elastic.co/logstash). There's good discussion comparing the two approaches here (cmanaha/python-elasticsearch-logger#44) with the author |
You have potential problem with overlapping fields of different types. Elasticsearch wont accept the same field with a type different to it's mapping. So if you have a two logs like:
the latter would be rejected. I havent checked to see if there are any instances of this issue, just flagging it as potential problem. |
Starting to check this finally, sorry for taking so long :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot give you a proper review since I'm not able to run it. It may be a misconfiguration on my end or something. Maybe we can chat about it.
I've left some minor comments of what I've been able to find so far.
Also, visualizations.py seems like a view. Couldn't it be a json?
Finally, I think the best approach for this is to be run as a module or plugin on top of teos
but with no actual connection to it, so the user can opt-in specifying some runtime params in teos (or config file). Therefore I think what @yahgwai was suggesting would make more sense than just parsing from the log file (my bad here, since I suggested this approach in the first place).
I'd focus on fixing the current approach and we can update it later though.
conf_loader = ConfigLoader(DATA_DIR, CONF_FILE_NAME, DEFAULT_CONF, command_line_conf) | ||
conf = conf_loader.build_config() | ||
|
||
max_users = conf.get("DEFAULT_SLOTS") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not completely sure what max_users is used for, but you're using DEFAULT_SLOTS
, which is used to define the default number of slots a users gets for a free subscription.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhhhhh I see. Lol, I'm not sure how I reached this conclusion, but I was thinking that was the maximum number of users a watchtower supported. But I see, there's a maximum # of appointments, but not users. Will rework the graph I made with that data bit based on that
8aea710
to
2f8cb9e
Compare
2f8cb9e
to
da8c4f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a conflict with the .gitignore
version in master. Merge or rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this creates a good base to work on the online logs to Elasticsearch. A structure that I think would make sense going in that direction is:
- Changing
start_monitor
formonitord
, and starting it fromteosd
as a standalone process if enabled (trough teos conf). - Sending logs from
teosd
tomonitord
. This can be done in multiple ways, but maybe the easiest one is trough a queue (check multiple-processing and queues). - Send the data in a similar way you're doing it now but instead of pulling from a file, pulling from the queue. Notice this may simplify some of the stuff since you don't need to care about where you left it in the file anymore.
- Have data persistency in mind (what to do if the process is stopped with pending data). This may be worth leaving as an enhancement, but it is good to have it in mind while designing the code.
- Have a backup strategy. The server connection may be interrupted and the data needs to be stored locally until it can be sent again. Check the
watchtower-plugin
for an example of how to handle this (feel free to ping me about it too). As the previous point, this may be worth leaving as an enhancement.
Nice job so far!
PS:
- Code needs proper formatting (black 120)
- Looks like the code breaks if the URLs are set with http://
@@ -0,0 +1,34 @@ | |||
This is a system monitor for viewing available user slots, appointments, and other data related to Teos. Data is loaded and searched using Elasticsearch and visualized using Kibana to produce something like this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README would need some work, but I'll leave the nits for the final version.
KIBANA_HOST = localhost | ||
KIBANA_PORT = 9243 | ||
|
||
# CLOUD_ID = System_monitor:sdiohafdlkjfl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the cloud stuff should be removed.
ES_HOST = localhost | ||
ES_PORT = 9200 | ||
KIBANA_HOST = localhost | ||
KIBANA_PORT = 9243 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change for the default Kibana port
@@ -0,0 +1,14 @@ | |||
[Teos] | |||
DATA_DIR = ~/.teos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fields in the config file must match the ones in __init__py.
You may have some fields that are not included in the config file b/c you may not want them to be settable. But you should not have anything in the conf file that has no default.
visualizer.create_dashboard() | ||
|
||
if __name__ == "__main__": | ||
command_line_conf = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd params are missing. Take a look at teosd.py
to see how to add them.
results = self.index_client.delete(index) | ||
|
||
# For testing purposes... | ||
def search_logs(self, field, keyword, index): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem to be used anymore. If the only purpose is testing, maybe it could be part of a local temp file or the testing suite.
return results | ||
|
||
|
||
def get_all_logs(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method does not seem to be used anywhere.
self.kibana_endpoint = "http://{}:{}".format(kibana_host, kibana_port) | ||
self.saved_obj_endpoint = "{}/api/saved_objects/".format(self.kibana_endpoint) | ||
self.space_endpoint = "{}/api/spaces/space/".format(self.kibana_endpoint) | ||
self.headers = headers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a double variable assignment there
self.max_users = max_users | ||
|
||
def create_dashboard(self): | ||
index_pattern = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This initialisations do not seem necessary. A dict returns None
if a value is not found when calling get.
dashboard = data.get("dashboard") | ||
image_url = data.get("imageUrl") | ||
|
||
index_id = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, it does not need init.
This PR puts together a first draft of a system monitor for a Teos watchtower. It can be run once Elasticsearch and Kibana are running to produce a Kibana dashboard with several graphs displaying Teos data.
Would love some feedback if anyone has time!
I also have my eyes on the following improvements: