Conversation
|
Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly. |
|
Can you address the rubocop feedback, they are all pretty straightforward but if you need any help let me know and I will be happy to assist. |
majormoses
left a comment
majormoses
left a comment
There was a problem hiding this comment.
This looks pretty cool and would love to add it, I have some questions and comments that should be addressed beforehand. Once you have addressed all the comments can you please include a manual or automated testing artifact? This helps us have greater confidence that it is working as intended and in many cases the maintainers do not have subject matter expertise on the particular piece of tech and even if they do they may not have an environment where they can easily test. Even a manual test may help a maintainer understand how to write a test for it being what the intended output should look like.
|
|
||
| value = rs.fetch_row[0].to_f | ||
|
|
||
| if config[:crit] > config[:warn] |
There was a problem hiding this comment.
While I think this is a clever way to solve the problem I generally find it's not a good idea to do this kind of auto detection as you can't validate that its been configured correctly. Perhaps passing in a --threshold-type (or similar) with an option list would be better. If someone wants to check if something is greater than and the critical threshold is lower than the warning threshold we should return an unknown as we can't determine if this is intentional or a misconfiguration. Having people be explicit what they want removes the ambiguity and makes it easier to stop bad configuration from alerting people. Here is an example of passing in a list of acceptable arguments: https://github.com/sensu-plugins/sensu-plugins-redis/blob/3.0.1/lib/redis_client_options.rb#L75. Also if you set the warning threshold to be the same as the critical this will default to lower which I am not sure is the right default.
| end | ||
|
|
||
| if value * factor >= config[:crit] * factor | ||
| critical "#{col_name} #{value} is #{beyond} the CRITICAL limit of #{config[:crit]}" |
There was a problem hiding this comment.
having the tripped threshold in the message seems redundant as it will start with the severity. Also not sure we really need to change "beyond" conditionally I think a more generic word such as exceeded would cover it, if you want to say do a hash lookup based on the type of threshold passed in I am less opposed I just find that these ways of trying to auto detect things often contain subtle bugs and are hard to detect misconfiguration.
There was a problem hiding this comment.
I would argue that most people would understand "value A exceeds value B" as "value A is above value B". Looking at a monitoring dashboard or a notification from a check which I might not even have configured myself, the description should tell me unambiguous what's going on. I'll redo the code with hashes, though.
There was a problem hiding this comment.
Fair enough though we are sacrificing a bit of code readability for the sake of being grammatically correct.
|
Hey still waiting on a change but you might want to wait until #79 lands as it probably will contain a conflict that will need to be resolved. |
2 participants
Pull Request Checklist
General
Update Changelog following the conventions laid out here
Update README with any necessary configuration snippets
Binstubs are created if needed
RuboCop passes
Existing tests pass
New Plugins
Tests
Add the plugin to the README
Does it have a complete header as outlined here
Purpose
This is basically a generalization of
check-mysql-select-count.rbwhich doesn't limit the user to aSELECT Count(*)statement - instead, any value from the first column of the first result set is used. This allows to select arbitrary values, functions or variables.Additionally, depending on the value of
warningandcritical, the check checks for a upper or a lower threshold.