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

Fixing Errors #38

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

NickFulton98
Copy link

This pull request includes a fix for monitoring-client.py where a variable 'r' was outside of the necessary scope, as well as a fix to the monitoring README file where it was lacking a command to install a needed dependency.

Changes Done:

 - Moved positioning of file output to inside of try block to correct scope of 'r' variable.

Testing Done:

 - Testing done on batman server cluster.
Changes Done:

 - Added section explaining to install a needed dependency.

Changes Done:

 - Testing done on batman server cluster.
@cjhouser
Copy link
Collaborator

cjhouser commented Nov 22, 2020

r should be scoped to the function as try/except doesn't create its own scope.
However, there is still an issue: The print_to_logger(alerting) will run even if an exception is caught since it is executed outside the try/except. [for clarity: this is an issue with the code before the changes in this PR]
An else statement can be appended to the try/except which will run only when no exception is caught.
Additionally, keeping the logger call outside of the try block allows the try/except to focus on catching exceptions with the requests.post call.
REF

Description:

 - Fixed the alerting error that was flagged by CJHouser
@NickFulton98
Copy link
Author

@cjhouser Can you please provide a line number for the other error you are flagging? I can't seem to find a alert that happens outside the try catches for this pr. Thanks!

Description:

 - Fixed an issue where an error in the log (such as when there is an alert) causes grafana.py to crash.
@@ -20,16 +20,16 @@ def alert(data):
info["embeds"].append({'description': data})
try:
r = requests.post(url, data=json.dumps(info), headers={'Content-type': 'application/json'})
except Exception as e:
print_to_file("Alerting (discord) - {}: {}\n".format(r.status_code, r.reason))
Copy link
Collaborator

@cjhouser cjhouser Nov 23, 2020

Choose a reason for hiding this comment

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

Put line 23 in an 'else:' block following the 'except:' block. The else block will run if no exception is caught and will also keep the try/except from throwing an exception if the file write fails in print_to_file().

For reference: https://docs.python.org/3/tutorial/errors.html#handling-exceptions
Ctrl+F for 'else clause'

if monitor_config['DEFAULT']['slack']:
url = monitor_config['DEFAULT']['slack']
try:
r = requests.post(url, data=json.dumps({'text':'{}'.format(data)}), headers={'Content-type': 'application/json'})
print_to_file("Alerting (slack) - {}: {}\n".format(r.status_code, r.reason))
Copy link
Collaborator

@cjhouser cjhouser Nov 23, 2020

Choose a reason for hiding this comment

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

Same for line 30.

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.

2 participants