Thor 7/monitor network security #133
Conversation
There was a problem hiding this comment.
Dear Contributor,
welcome to the Thronode bot project and thanks for your contribution!
Congratulations for easy and smooth onboarding in the project.
I left some suggestions regarding the style, feel free to open discussion about it.
Hint regarding testing: it's always good to run the bot in DEBUG=True,False on testnet and DEBUG=False on chaosnet to make sure there is no crash or anything.
Thanks for fixing the typos.
bot/constants/globals.py
Outdated
|
|
||
|
|
||
| class NetworkHealthStatus(Enum): | ||
| INEFFICIENT = "Inefficient" | ||
| OVERBONDED = "Overbonded" | ||
| OPTIMAL = "Optimal" | ||
| UNDBERBONDED = "Underbonded" | ||
| INSECURE = "Insecure" |
There was a problem hiding this comment.
Good idea with using enum in this case. Maybe constants.messages file would be a more appropriate place for this piece of code?
bot/constants/messages.py
Outdated
| HEALTH_LEGEND = f'\n*Node health*:\n{HEALTH_EMOJIS[True]} - *healthy*\n{HEALTH_EMOJIS[False]} - *unhealthy*\n' \ | ||
| f'{HEALTH_EMOJIS[None]} - *unknown*\n' | ||
|
|
||
| NETWORK_HEALTH_CURATION = "The network is safe and efficient again! ✅" |
There was a problem hiding this comment.
This name was a little bit mysterious for me at first. Is something like NETWORK_HEALTHY_AGAIN easier to understand?
| def NETWORK_HEALTH_WARNING(network_health_status: NetworkHealthStatus) -> str: | ||
| severity = "🤒" | ||
| if network_health_status is NetworkHealthStatus.INSECURE: | ||
| severity = "💀" | ||
| elif network_health_status is NetworkHealthStatus.INEFFICIENT: | ||
| severity = "🦥" | ||
|
|
||
| return f"Network health is not optimal: {network_health_status.value} {severity}" |
There was a problem hiding this comment.
Wouldn't it be simpler to just use a dictionary? See constants/globals.py:30 STATUS_EMOJIS
There was a problem hiding this comment.
We came to a consensus and decided no
bot/jobs/thornodes_jobs.py
Outdated
| """ | ||
| Job notifying the user if network security has changed | ||
| """ |
There was a problem hiding this comment.
We are trying to move away from commenting stuff too much when the function's operation is clear from the method name, if there are any old unnecessary comments in the code feel free to remove them (@mr-rooftop correct me if I'm wrong)
bot/jobs/thornodes_jobs.py
Outdated
| context.bot_data["network_health_status"] = network_health_status | ||
| return None | ||
|
|
||
| if network_health_status is not context.bot_data["network_health_status"]: |
There was a problem hiding this comment.
I'm pretty sure that we should compare here using != because we want to compare the values, not the objects itself
test/unit_tests/_trial_temp/test.log
Outdated
| @@ -0,0 +1 @@ | |||
| 2021-01-12 09:56:14+0100 [-] Log opened. | |||
| @@ -14,6 +14,9 @@ def setup_bot_data(dispatcher): | |||
| dispatcher.job_queue.run_repeating(check_other_nodes_syncing_job, | |||
| interval=syncing_checks_interval_in_seconds) | |||
|
|
|||
| dispatcher.job_queue.run_repeating(check_network_security_job, | |||
| interval=3600) | |||
There was a problem hiding this comment.
just for consistency, remove pls syncing_checks_interval_in_seconds or assign seconds in the same way here
Description
Add new Job (hourly interval)
trigger notification on network health change
Network health is important in terms of security and efficiency, therefore the user should be notified.
Thor-7 JIRA Task
Fixes (issue)
Monitor Network State
Improvements
Trying to introduce best practices w/ enums and string file instead of hardcoded strings.
How Has This Been Tested?
Changing the health state of the network and checking if the the returned message matches the expected ones.
Checklist:
If you have any comments regarding one of these points just write it down.