47

Overall this is quite a nice module. Here are a few usability issues/nitpicks, though: When running the script, there is no easy way to stop it. CTRL+C does not work, I have to manually kill the process. This is probably because of how threading.Thread handles it, but I'm not sure. It would be nice if it was slightly easier to add a different handler than ...


32

This is more of a design / algorithm / architecture review than of the code. There are some major issues the other answers didn't address. First of all, wasting network bandwidth running a speed-test on repeat seems like a bad idea. It will fill up your connection every hour (by default), so it hurts you personally if you happen to be doing something on ...


22

I'd like to say just one (yet IMO very important) thing: You're developing bad habits early! The point of classes is to eliminate (truly) global state and to manage it within classes and objects. You've really abused the use of global which is making your code harder to digest. There are cases where it's fine to use globals, where the generality of passing ...


11

I'll proceed top to bottom. mail_error Which would you rather read: x.body = e.message or mail.body = exception.message? Don't try to save a few characters on naming — your fellow programmers will like you better. In particular, x has a connotation of being a floating point number, which this isn't. scanning_all_servers and server_status Net::SSH#start ...


11

Argument handling $defaultmail and $defaultip are poorly named — there is a naming inconsistency: $host ||= $defaultip; … $mailto ||= $defaultmail; I would also advise against introducing $defaultmail and $defaultip as variables at all. Either just write them in directly: my $host = (shift @ARGV) || '8.8.8.8'; # Google's nameserver my $...


8

I’ll start with a meta comment: Python (and production code) emphasis readability. There are several parts of your code which are quick difficult to read, or work out what they’re for (more details below). In production code, we don’t just want speed and reliability – we also want code that somebody else can debug in a year, who’s never seen it before. Try ...


7

Want to start of with I am new to workflows as well. For the most part your code is solid. You should see a couple of suggestions that would actually improve performance especially if you are running this against 100's of computers. Grouping logic This is more of a compliment. I thought, from what I read, that -ThrottleLimit would be a simple answer to ...


7

I think you need to think your use cases through a bit. What should this do during an outage? Do you want to enqueue a new tweet if a previous one hasn't been sent yet? Do you want to enqueue a special message for this, like "outage detected at hh:mm, resolved at hh:mm" (adjusted when sending after the outage)? Should it tweet at all? What exactly should ...


6

You could replace your get_str_from_file function with the standard read_to_string function: For instance like so: use std::fs; if fs::read_to_string(STATUS).unwrap().trim() == CHARGING_STR) { return; //return should be equivalent to std::process::exit(0) } This is your implementation: fn get_str_from_file(file_path: &str) -> String { let ...


6

This looks pretty good. Some suggestions: [[ is preferred over [ in Bash. This script could benefit from set -o errexit -o nounset -o pipefail. You'll need to set cpu_cores=0 before referring to it in the loop, but that's just best practice. Dividing by one isn't going to do much :) The quotes are a cute feature, but I'd much rather see a summary of the ...


5

This definitely looks like a real project that someone can use or expand. Have you put it on GitHub? The symbolic constants defined in TempI.h are following the C Standard. Some general considerations, while the most common numbers of cores in a CPU chip are currently 1, 2 or 4, there can also be larger powers of 2 such as 8, 16 or 32. Rather than using a ...


5

Overall - good initial post. Recommend to include the corresponding header file first. If heartbeat.h relies on any include file, then it (the .h file) should include them. View this from a user's perspective of the functions in the .h file. The user should not need to know .h perquisites, let the .h file include them itself. By including the .h file ...


5

Here's a solution using threads: import sys import os import platform import subprocess import Queue import threading def worker_func(pingArgs, pending, done): try: while True: # Get the next address to ping. address = pending.get_nowait() ping = subprocess.Popen(ping_args + [address], stdout ...


5

A design pattern often used, is to have a non generic base type and a generic type derived from it or a type implementing a non generic as well as a generic interface. An example is public abstract class Comparer<T> : System.Collections.Generic.IComparer<T>, System.Collections.IComparer You can see the full implementation here public ...


4

It's not a complete answer, but your code would be more clear (and easy to maintain) if you won't duplicate code: import sys import os import platform import subprocess plat = platform.system() scriptDir = sys.path[0] hosts = os.path.join(scriptDir, 'hosts.txt') hostsFile = open(hosts, "r") lines = hostsFile.readlines() for line in lines: line = line....


4

When the server does not exists it seems node throws an EIO error. Now you need to make a decision, you can either reject the promise or resolve with false. From an usage point of view I would say I would prefer to resolve it false unless I need to know why the server is unavailable. I am assuming that you just want to know if it is available or not, ...


4

Your use of $locate as a regular expression for grep is sloppy. If $locate is 2.3.45.25, for example, a hop through 70.233.45.251 would be considered successful. I suggest using grep -F " $locate " — with -F to interpret the argument as a fixed string rather than as a regex, and with spaces before and after the IP address.


4

Although unrelated to the code, I'll mention that for a CPU to overheat, especially a dual-core CPU, is not usual except with very high ambient temps. I'd suggest removing the heat sink and re-applying thermal paste. Any number of youtube videos can provide step-by-step instructions. Moving on to the code: terminal semicolons aren't needed configuration ...


4

Unwrapping options is probably fine for this simple little app that only you’re using, but it dumps out a panic if the result is None. I like your use of expect better. That can give a nicer error message to the user. Either way, it’d be good to learn the more idiomatic way of dealing with options. match x { None => foo, Some => bar } The ...


3

Instead of storing the output of the traceroute | grep pipeline in a variable and checking that it's not empty, it would be better to work with the exit code of the pipeline directly: if traceroute -n -w 2 "$target" | grep -q "$locate"; then echo "$locate found in path. Still running on primary" exit $nagiosStateOK else echo "$locate not found ...


3

You need to quote your variables: traceResult=$(traceroute -n -w 2 "$target" | grep "$locate") ref: Security implications of forgetting to quote a variable in bash/POSIX shells


3

As from your comment: Its a shared block used in other "nagios" scripts. # Nagios exit codes. These are used to determine state nagiosStateOK=0 nagiosStateWarning=1 nagiosStateCritical=2 nagiosStateUnknown=3 You should put shared code with other scripts rather into an included source file like source nagios_shared_stuff.inc or . ...


3

Don't write custom scripts for common sysadmin tasks. Using the proper tools will get the job done better, and the solution will be more maintainable — especially by any colleagues you might have. Using monit, for example, this would just be a matter of writing a simple configuration file. set logfile /root/process_monitor.log check process myproc with ...


3

Thanks for incorporating the various suggestions and posting a new code review. This is looking really excellent. Some nits and answers to your concerns: creating a function for curling is good. "$@" is the right choice as well. But it would be nice if the stuff within squiggly brackets was indented like: function _curl { curl --fail -u $USERNAME:$...


3

Naming The elephant in the room: names like var1 and var2 really hinder the readibility. var1 probably means has_packet_loss, var2 -> latency. Naming is hard, but it also is a fundamental property of readable code. Ternary You got repetition in if var1 == 0: # has_packet_loss Status.configure(background="Green") else: Status.configure(background="...


3

Great idea! That makes it possible to get information about hanging / lacking GUIs. That kind of (probably) valuable information are only available via customer feedback otherwise :). It would be interesting to know if you already use that ThreadMonitor, if you already evaluated the log files and if you got some interesting information out of it ;). I have ...


3

Looks good to me. Kudos for the "... or die" idiom. More informative output would be "unable to run df on $s" or "unable to run svmon on $s". You don't have to do tail or grep on the far end, as a line of perl could do that. But it is a perfectly clean and sensible approach, which I advocate keeping. If you wind up with many such monitoring commands, you ...


3

Kudos to you! This is a nice little script, easy to read and to understand. However, there is no reason to reach for expr. Most shells (including dash or even busybox sh) can interpret arithmetic expressions on their own: WARNING=$(($total / 5)) CRITICAL=$(($total / 10)) Also, in [ ] expressions, I'd use quotes around variables, to make sure that they don'...


3

I’d agree with all of l0b0’s answer – including the suggestion of using grep to process /proc/cpuinfo. An alternate way to count all CPU cores using an AWK one-liner would be: cpu_cores=$(awk '/cpu cores/ { num_cores += $4} END { print num_cores}' /proc/cpuinfo) This would also remove the usage of Bash arrays, resulting in the script being POSIX-compatible ...


3

I just have a couple of minor points to add to Olivier Jacot-Descombes' excellent answer. public override bool Faulty() { if (LogicalOperator == eLogicalOperator.Equal) return Value == Threshold; else if (LogicalOperator == eLogicalOperator.GreaterOrEqualThan) return Value >= Threshold; else if (...


Only top voted, non community-wiki answers of a minimum length are eligible